Implement fallback to anonymous role permissions#3141
Implement fallback to anonymous role permissions#3141KobeCnext wants to merge 1 commit intoAzure:mainfrom
Conversation
Added fallback to anonymous role's permissions if the role is not found.
There was a problem hiding this comment.
Pull request overview
This PR attempts to fix an issue where authenticated users cannot filter on anonymous-accessible entities by implementing a fallback to anonymous role permissions in the AreRoleAndOperationDefinedForEntity method. When a role doesn't have explicit permissions for an entity, the system now checks if the anonymous role has permissions and grants access accordingly.
Changes:
- Added fallback logic to check anonymous role permissions when the requested role lacks permissions
- Added explanatory comments describing the fallback behavior
| } | ||
|
|
||
| return false; | ||
| return false; |
There was a problem hiding this comment.
Critical syntax error: the closing brace on line 143 closes the outer if block prematurely, making the return statement on line 145 unreachable and causing a compilation error. The return statement should be inside the method body. Remove the extra closing brace on line 143.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| } | ||
|
|
||
| return false; | ||
| return false; |
There was a problem hiding this comment.
Indentation error: the return statement has no indentation, which violates C# formatting conventions. It should be indented with 4 spaces to align with the method body.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| if (valueOfEntityToRole.RoleToOperationMap.TryGetValue(ROLE_ANONYMOUS, out RoleMetadata? anonymousRoleMetadata)) | ||
| { | ||
| if (anonymousRoleMetadata!.OperationToColumnMap.ContainsKey(operation)) | ||
| { | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
The fallback to anonymous role permissions in AreRoleAndOperationDefinedForEntity should be accompanied by a similar fallback in AreColumnsAllowedForOperation. When a filter references fields on an anonymous-accessible entity, the field-level authorization check (called after entity-level check in GraphQLFilterParsers.cs line 158) should also fall back to anonymous role's column permissions. Without this, authenticated users may still get "Access forbidden to a field referenced in the filter" errors even after this entity-level fix.
| // If the role is not found or doesn't define the operation, | ||
| // fall back to the anonymous role's permissions | ||
| // since anonymous access implies the entity is publicly accessible. | ||
| if (valueOfEntityToRole.RoleToOperationMap.TryGetValue(ROLE_ANONYMOUS, out RoleMetadata? anonymousRoleMetadata)) | ||
| { | ||
| if (anonymousRoleMetadata!.OperationToColumnMap.ContainsKey(operation)) | ||
| { | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Security concern: This fallback allows authenticated users to access any entity that has anonymous permissions, even if those authenticated users don't have explicit permissions for their role. While this may be intentional for GraphQL (since lines 92-93 in CosmosMutationEngine.cs and 126-127 in SqlMutationEngine.cs suggest anonymous permissions are inherited), the logic in AreRoleAndOperationDefinedForEntity should be documented with clear explanation of the security model. Consider: (1) Should this fallback apply to all operations or only Read? (2) Does this align with REST API authorization behavior? (3) Should this behavior be configurable?
| if (valueOfEntityToRole.RoleToOperationMap.TryGetValue(ROLE_ANONYMOUS, out RoleMetadata? anonymousRoleMetadata)) | ||
| { | ||
| if (anonymousRoleMetadata!.OperationToColumnMap.ContainsKey(operation)) | ||
| { | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
This new fallback behavior lacks test coverage. No unit tests in AuthorizationResolverUnitTests.cs verify that AreRoleAndOperationDefinedForEntity returns true when the requested role lacks permissions but anonymous role has them. Additionally, no integration tests verify the specific scenario described in the PR (authenticated user with filter on anonymous entity). Test cases should cover: (1) Entity with anonymous permissions, authenticated role without permissions, (2) Various operations (Read, Create, Update, Delete), (3) Integration test matching the PR's sample request.
| // If the role is not found or doesn't define the operation, | ||
| // fall back to the anonymous role's permissions | ||
| // since anonymous access implies the entity is publicly accessible. | ||
| if (valueOfEntityToRole.RoleToOperationMap.TryGetValue(ROLE_ANONYMOUS, out RoleMetadata? anonymousRoleMetadata)) | ||
| { | ||
| if (anonymousRoleMetadata!.OperationToColumnMap.ContainsKey(operation)) | ||
| { | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
This fallback logic contradicts the existing permission model. Lines 391-397 in this file show that anonymous permissions are copied to authenticated role ONLY when authenticated role is not explicitly defined in the configuration. The new fallback allows ANY role (even explicitly defined roles with different permissions) to use anonymous permissions as a fallback. This means: (1) A role explicitly configured with restricted permissions could bypass those restrictions by using anonymous fallback, (2) The behavior is inconsistent with the startup-time permission setup. The fallback should either check if the role is "authenticated" specifically, or verify that the role was not explicitly defined in the configuration.
Fix filtering by Authorized users on Anonymous accessible Entities
Why make this change?
Logged in users could not add a filter on an anonymous Entity
What is this change?
Allow access to anonymous entities for logged in users in a specific 'filter' scenario
How was this tested?
Manual test
Sample Request(s)
This request, when made with a Authenticated user failed with:
If you remove the filter section ,it works.
After my fix this query works as expected.