Implement fallback to anonymous role permissions #3141
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -128,9 +128,21 @@ public bool AreRoleAndOperationDefinedForEntity(string entityIdentifier, string | |
| return true; | ||
| } | ||
| } | ||
|
|
||
| // 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; | ||
| } | ||
| } | ||
|
Comment on lines
+132
to
+141
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?
Comment on lines
+135
to
+141
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.
Comment on lines
+132
to
+141
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. |
||
| } | ||
| } | ||
|
|
||
| return false; | ||
|
Comment on lines
+143
to
+145
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.
Author
There was a problem hiding this comment. @copilot open a new pull request to apply changes based on this feedback
Collaborator
There was a problem hiding this comment. @KobeCnext, thanks for this fix. Copilot will not operate on PRs that merge into a cross repository. Could you please fix these comments? 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.
Author
There was a problem hiding this comment. @copilot open a new pull request to apply changes based on this feedback |
||
| } | ||
|
|
||
| public bool IsStoredProcedureExecutionPermitted(string entityName, string roleName, SupportedHttpVerb httpVerb) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.