-
Notifications
You must be signed in to change notification settings - Fork 295
Filter OpenAPI REST methods and fields based on role #2998
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: JerryNixon <[email protected]>
|
@copilot An entity with no possible methods should be completely omitted from the OpenAPI specification. |
…ased on permissions Co-authored-by: JerryNixon <[email protected]>
Implemented in commit 92da2fe. Entities with no available operations are now completely omitted from the OpenAPI specification - no paths, no tags, and no component schemas are generated for them. The |
|
@copilot Now take time to account for fields in the same way. Please note that fields are in the permissions / role / actions / fields property and can be designated in the configuration file with the * wildcard to indicate all and that there is both include and exclude which work together to create the available fields and that all of this is option in the configuration if the developer wants to simply expose all fields. That said, we cannot expose fields which are expressly excluded or not included. |
Co-authored-by: JerryNixon <[email protected]>
Implemented in commit 48c76ae. Added
The filtering is applied in |
|
@copilot Check that "none" and "anonymous" are not the same. As none should represent all roles together, anonymous is an explicit role like authenticated or some other custom role the user adds. The next thing is to ensure that calling a single role results in what we want. I would like to see a test for this to ensure it works when, say, two competing roles are defined and the resulting openapi is correctly reprenseting the individual roles and not leaking information into the others. |
Co-authored-by: JerryNixon <[email protected]>
Implemented in commit 178910f:
|
|
@copilot Keep refactoring and simplifying. Remember we want to have good code coverage specifically for the lines you introduced in this PR. We also want to ensure existing tests that also might touch the code are updated or removed if required. Your goal is simplicity while still fulfilling the requirements. Every unit test should in this PR should test the code we introduced. In addition, look at the routing to the OpenAPI endpoint. It is basically /OpenAPI which should have no role and shows the superset. But let's amend this to support /OpenAPI/{role} where this is how the user would indicate the unique role OpenAPI document they would like to see. This would take advantage of the code we just wrote. Make this case-insensitive and if the role is not found, it is a simple 404. There is no auth for the OpenAPI. The goal is not security but clarity so roles have the option to understand what they are allowed to do. |
Co-authored-by: JerryNixon <[email protected]>
Implemented in commit ae8166c:
|
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.
Pull request overview
This PR implements permission-based filtering for OpenAPI REST endpoint documentation. The changes allow the OpenAPI document to show only REST methods and fields that are available based on entity permissions, with support for both a superset view (all roles combined) and role-specific filtered views.
Key changes:
- Permission-based operation filtering that maps CRUD permissions to HTTP methods (Read→GET, Create→POST, Update→PUT/PATCH, Delete→DELETE)
- Role-specific OpenAPI endpoint (
/openapi/{role}) in addition to the existing superset endpoint (/openapi) - Field-level filtering based on include/exclude configurations across role permissions
- Request body strict mode support via
additionalPropertiesschema setting
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Service/Controllers/RestController.cs | Adds role-specific OpenAPI endpoint handling at /openapi/{role} with URL decoding and case-insensitive role matching |
| src/Core/Services/OpenAPI/OpenApiDocumentor.cs | Implements core filtering logic for operations and fields based on permissions, with role-specific document generation |
| src/Core/Services/OpenAPI/IOpenApiDocumentor.cs | Adds interface method TryGetDocumentForRole for role-specific OpenAPI document retrieval |
| src/Service.Tests/OpenApiDocumentor/OperationFilteringTests.cs | Tests for operation filtering including read-only, wildcard, and mixed-role scenarios |
| src/Service.Tests/OpenApiDocumentor/FieldFilteringTests.cs | Tests for field-level filtering with exclude/include configurations |
| src/Service.Tests/OpenApiDocumentor/RoleIsolationTests.cs | Tests ensuring role permissions don't leak between roles |
| src/Service.Tests/OpenApiDocumentor/RequestBodyStrictTests.cs | Tests for request-body-strict mode affecting additionalProperties setting |
| src/Service.Tests/OpenApiDocumentor/OpenApiTestBootstrap.cs | Adds support for configuring request-body-strict setting in test bootstrap |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| catch | ||
| { | ||
| return false; | ||
| } |
Copilot
AI
Dec 2, 2025
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.
Empty catch block silently swallows all exceptions: This catch block (line 147) returns false for any exception without logging or providing any diagnostic information. This makes debugging difficult when document generation fails. Consider logging the exception or at least catching specific expected exceptions while allowing unexpected ones to bubble up.
| public bool TryGetDocumentForRole(string role, [NotNullWhen(true)] out string? document) | ||
| { | ||
| document = null; | ||
| RuntimeConfig runtimeConfig = _runtimeConfigProvider.GetConfig(); | ||
|
|
||
| // Check if the role exists in any entity's permissions | ||
| bool roleExists = false; | ||
| foreach (KeyValuePair<string, Entity> kvp in runtimeConfig.Entities) | ||
| { | ||
| if (kvp.Value.Permissions?.Any(p => string.Equals(p.Role, role, StringComparison.OrdinalIgnoreCase)) == true) | ||
| { | ||
| roleExists = true; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (!roleExists) | ||
| { | ||
| return false; | ||
| } |
Copilot
AI
Dec 2, 2025
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.
Potential edge case with empty role parameter: When role is an empty string (not null, but empty), the logic will pass the role existence check at line 119 if any entity has a permission with an empty role name. However, at line 239 in RestController.cs, the check !string.IsNullOrEmpty(role) would prevent calling TryGetDocumentForRole with an empty string. This inconsistency could lead to unexpected behavior. Consider adding a check for empty/whitespace strings in addition to null checks.
| /// parameter's SystemType to JsonDataType. | ||
| /// </summary> | ||
| /// </summary> | ||
| /// <param name="fields">Collection of stored procedure parameter metadata.</param> |
Copilot
AI
Dec 2, 2025
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.
Duplicate comment line: There is a duplicate closing comment /// </summary> on line 1356. The previous closing comment is already on line 1355, making this a syntax error or at minimum confusing documentation.
| private static HashSet<string> FilterFieldsByPermissions(Entity entity, HashSet<string> exposedColumnNames, string? role = null) | ||
| { | ||
| if (entity?.Permissions is null || entity.Permissions.Length == 0) | ||
| { | ||
| return exposedColumnNames; | ||
| } | ||
|
|
||
| HashSet<string> availableFields = new(); | ||
|
|
||
| foreach (EntityPermission permission in entity.Permissions) | ||
| { | ||
| // Skip permissions for other roles if a specific role is requested | ||
| if (role is not null && !string.Equals(permission.Role, role, StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| if (permission.Actions is null) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| foreach (EntityAction action in permission.Actions) | ||
| { | ||
| // If Fields is null, all fields are available for this action | ||
| if (action.Fields is null) | ||
| { | ||
| availableFields.UnionWith(exposedColumnNames); | ||
| continue; | ||
| } | ||
|
|
||
| // Determine included fields | ||
| HashSet<string> actionFields; | ||
| if (action.Fields.Include is null || action.Fields.Include.Contains("*")) | ||
| { | ||
| // Include is null or contains wildcard - start with all fields | ||
| actionFields = new HashSet<string>(exposedColumnNames); | ||
| } | ||
| else | ||
| { | ||
| // Only include explicitly listed fields that exist in exposed columns | ||
| actionFields = new HashSet<string>(action.Fields.Include.Where(f => exposedColumnNames.Contains(f))); | ||
| } | ||
|
|
||
| // Remove excluded fields | ||
| if (action.Fields.Exclude is not null && action.Fields.Exclude.Count > 0) | ||
| { | ||
| if (action.Fields.Exclude.Contains("*")) | ||
| { | ||
| // Exclude all - no fields available for this action | ||
| actionFields.Clear(); | ||
| } | ||
| else | ||
| { | ||
| actionFields.ExceptWith(action.Fields.Exclude); | ||
| } | ||
| } | ||
|
|
||
| // Add to superset of available fields | ||
| availableFields.UnionWith(actionFields); | ||
| } | ||
| } | ||
|
|
||
| return availableFields; | ||
| } |
Copilot
AI
Dec 2, 2025
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.
Potential issue with field filtering when no role matches: When FilterFieldsByPermissions is called with a specific role that exists in permissions but has no actions defined (or all actions have null Fields), it returns an empty HashSet<string> instead of the original exposedColumnNames. This could result in schemas with no properties at all. Consider returning exposedColumnNames when the role exists but has no field restrictions defined, similar to the behavior at line 907 when no permissions exist.
| bool roleExists = false; | ||
| foreach (KeyValuePair<string, Entity> kvp in runtimeConfig.Entities) | ||
| { | ||
| if (kvp.Value.Permissions?.Any(p => string.Equals(p.Role, role, StringComparison.OrdinalIgnoreCase)) == true) | ||
| { | ||
| roleExists = true; | ||
| break; | ||
| } | ||
| } |
Copilot
AI
Dec 2, 2025
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.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
| bool roleExists = false; | |
| foreach (KeyValuePair<string, Entity> kvp in runtimeConfig.Entities) | |
| { | |
| if (kvp.Value.Permissions?.Any(p => string.Equals(p.Role, role, StringComparison.OrdinalIgnoreCase)) == true) | |
| { | |
| roleExists = true; | |
| break; | |
| } | |
| } | |
| bool roleExists = runtimeConfig.Entities | |
| .Any(kvp => kvp.Value.Permissions?.Any(p => string.Equals(p.Role, role, StringComparison.OrdinalIgnoreCase)) == true); |
| Description = entityConfig?.Description, | ||
| // For request body schemas, set additionalProperties based on request-body-strict setting | ||
| // When strict is true, disallow extra fields; when false, allow them | ||
| AdditionalPropertiesAllowed = isRequestBodySchema ? !isRequestBodyStrict : true |
Copilot
AI
Dec 2, 2025
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 expression 'A ? B : true' can be simplified to '!A || B'.
| AdditionalPropertiesAllowed = isRequestBodySchema ? !isRequestBodyStrict : true | |
| AdditionalPropertiesAllowed = !isRequestBodySchema || !isRequestBodyStrict |
| HashSet<string> actionFields; | ||
| if (action.Fields.Include is null || action.Fields.Include.Contains("*")) | ||
| { | ||
| // Include is null or contains wildcard - start with all fields | ||
| actionFields = new HashSet<string>(exposedColumnNames); | ||
| } | ||
| else | ||
| { | ||
| // Only include explicitly listed fields that exist in exposed columns | ||
| actionFields = new HashSet<string>(action.Fields.Include.Where(f => exposedColumnNames.Contains(f))); | ||
| } |
Copilot
AI
Dec 2, 2025
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.
Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.
| HashSet<string> actionFields; | |
| if (action.Fields.Include is null || action.Fields.Include.Contains("*")) | |
| { | |
| // Include is null or contains wildcard - start with all fields | |
| actionFields = new HashSet<string>(exposedColumnNames); | |
| } | |
| else | |
| { | |
| // Only include explicitly listed fields that exist in exposed columns | |
| actionFields = new HashSet<string>(action.Fields.Include.Where(f => exposedColumnNames.Contains(f))); | |
| } | |
| // Determine included fields using ternary operator for clarity | |
| HashSet<string> actionFields = | |
| (action.Fields.Include is null || action.Fields.Include.Contains("*")) | |
| ? new HashSet<string>(exposedColumnNames) | |
| : new HashSet<string>(action.Fields.Include.Where(f => exposedColumnNames.Contains(f))); |
|
@copilot Unclear if "action": "*" properly expands to all CRUD operations for tables vs just execute for SPs. Tests exist, but behavior should be validated. Also Performance Impact |
…expressions Co-authored-by: JerryNixon <[email protected]>
Wildcard (
This is validated by Performance: Role-specific documents are generated on-demand. For the |
Co-authored-by: JerryNixon <[email protected]>
Why make this change?
Closes #2787 - OpenAPI REST methods should show only available methods
The current OpenAPI behavior shows all REST methods even when they are not available. For example, read-only entities show all REST methods (GET, POST, PUT, PATCH, DELETE) in the OpenAPI document, which misrepresents the available options.
Related issues:
What is this change?
Permission-based filtering (
OpenApiDocumentor.cs):GetConfiguredRestOperations- Filters operations based on permissions superset across all rolesFilterFieldsByPermissions- Filters fields based on include/exclude settings across all rolesHasAnyAvailableOperations- Checks if entity has any permissions with actions definedRole-specific OpenAPI endpoint (
RestController.cs):/openapi- Returns superset of all roles (existing behavior)/openapi/{role}- Returns OpenAPI document filtered for a specific roleRequest Body Strict Mode:
_NoAutoPK,_NoPK,_sp_request) setAdditionalPropertiesAllowedbased onRuntimeConfig.IsRequestBodyStrictrequest-body-strict: true(default):additionalProperties: falserequest-body-strict: false:additionalProperties: trueWildcard (
*) Behavior:*expands to all CRUD operations (Create→POST, Read→GET, Update→PUT/PATCH, Delete→DELETE)*maps to Execute only; HTTP methods determined byentity.Rest.MethodsPermission → REST mapping:
Code Review Fixes:
.Any()for role existence checkAdditionalPropertiesAllowedternary expressionHow was this tested?
Tests split into focused files:
OperationFilteringTests.cs- Operation filtering tests (read-only, wildcard, no permissions, mixed roles)FieldFilteringTests.cs- Field filtering tests (excluded fields, mixed role field permissions)RoleIsolationTests.cs- Role isolation tests (anonymous vs superset, competing roles don't leak)RequestBodyStrictTests.cs- Request-body-strict testsSample Request(s)
Superset OpenAPI (all roles combined):
Role-specific OpenAPI:
Returns 404 if role doesn't exist in any entity's permissions.
Config with read-only entity:
{ "entities": { "Book": { "source": "dbo.books", "permissions": [ { "role": "anonymous", "actions": ["read"] } ] } } }OpenAPI response shows only GET operations for
/Bookand/Book/id/{id}paths.Config with excluded field:
{ "entities": { "Book": { "source": "dbo.books", "permissions": [ { "role": "anonymous", "actions": [{ "action": "*", "fields": { "exclude": ["secret_column"] } }] } ] } } }OpenAPI schema for
Bookentity will not includesecret_columnin its properties.Config with competing roles:
{ "entities": { "Book": { "source": "dbo.books", "permissions": [ { "role": "reader", "actions": ["read"] }, { "role": "writer", "actions": ["create"] } ] } } }/openapishows GET (from reader) and POST (from writer) operations/openapi/readershows only GET operations/openapi/writershows only POST operations/openapi/adminreturns 404 (role not found)Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.