Extend MinimalPermissionsPlugin by introducing the schemeName property #1391
Conversation
|
Thanks! We'll review it asap |
|
We're going to do a release tomorrow. To avoid adding last minute changes, we'll wait with reviewing/merging until after the release, later this week, ok? |
no worries, whenever is fine. |
There was a problem hiding this comment.
Pull Request Overview
This PR extends the MinimalPermissionsPlugin by introducing a schemeName property that allows users to specify which security scheme definition to use when determining minimal permissions for API calls.
- Adds
schemeNameconfiguration property to target specific OAuth2 security schemes - Updates permission checking logic to filter by the specified scheme name
- Enhances reporting to include scheme name information when available
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| schemas/v1.2.0/minimalpermissionsplugin.schema.json | Adds schemeName property definition to the JSON schema |
| DevProxy.Plugins/Reporting/MinimalPermissionsPluginReport.cs | Updates report classes to include and display scheme name information |
| DevProxy.Plugins/Reporting/MinimalPermissionsPlugin.cs | Integrates schemeName configuration into the plugin logic and logging |
| DevProxy.Plugins/Extensions/OpenApiDocumentExtensions.cs | Modifies OAuth2 scheme filtering to support scheme name targeting |
| return CheckMinimalPermissions(openApiDocument, requests, logger, null); | ||
| } | ||
|
|
||
| public static ApiPermissionsInfo CheckMinimalPermissions(this OpenApiDocument openApiDocument, IEnumerable<RequestLog> requests, ILogger logger, string? schemeName) |
There was a problem hiding this comment.
Since we're adding an optional parameter, couldn't we add it to the original signature rather than introducing a new method, similarly to what we've done with GetEffectiveScopes?
There was a problem hiding this comment.
Left unchanged.
If we look at the parameter a bit closely, it's not an optional. There are 2 separate methods as it helps to avoid an ambiguity during refactoring. No change comming here as the extra method is going away in a few days.
| var schemes = openApiDocument.Components.SecuritySchemes | ||
| .Where(s => s.Value.Type == SecuritySchemeType.OAuth2); | ||
|
|
||
| if (!string.IsNullOrWhiteSpace(schemeName)) |
There was a problem hiding this comment.
Why not add the name check to the .Where query above?
| Results = [.. results], | ||
| UnmatchedRequests = [.. unmatchedRequests], | ||
| Errors = [.. errors] | ||
| Errors = [.. errors], |
There was a problem hiding this comment.
Let's remove this dangling comma
| .AppendLine(); | ||
|
|
||
| .AppendLine() | ||
| if (!string.IsNullOrWhiteSpace(apiResult.SchemeName)) |
There was a problem hiding this comment.
Rather than introducing a whole separate section for just one name, let's add it to the ### Minimal permissions heading, like ### Minimal permissions for xyz scheme
Co-authored-by: Copilot <[email protected]>
#1023