Skip to content

Commit

Permalink
[Core, Rust Server, ASP.NET Core] Fix Codegen Operation Scope Consist…
Browse files Browse the repository at this point in the history
…ency (#3495)

Fix Codegen Operation Scope Consistency

- Filter scopes based on operation

- Partially revert #1984 to not rely on custom attributes as to whether scopes exist

- Fix filtering global authentication schemes
  • Loading branch information
richardwhiuk authored Nov 8, 2019
1 parent 5b44418 commit de162f7
Show file tree
Hide file tree
Showing 16 changed files with 971 additions and 450 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

package org.openapitools.codegen;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Locale;
Expand All @@ -38,7 +39,7 @@ public class CodegenSecurity {
// Oauth specific
public String flow, authorizationUrl, tokenUrl;
public List<Map<String, Object>> scopes;
public Boolean isCode, isPassword, isApplication, isImplicit, hasScopes;
public Boolean isCode, isPassword, isApplication, isImplicit;

@Override
public String toString() {
Expand Down Expand Up @@ -100,4 +101,47 @@ public int hashCode() {
isImplicit,
scopes);
}

// Return a copy of the security object, filtering out any scopes from the passed-in list.
public CodegenSecurity filterByScopeNames(List<String> filterScopes) {
CodegenSecurity filteredSecurity = new CodegenSecurity();
// Copy all fields except the scopes.
filteredSecurity.name = name;
filteredSecurity.type = type;
filteredSecurity.hasMore = false;
filteredSecurity.isBasic = isBasic;
filteredSecurity.isBasicBasic = isBasicBasic;
filteredSecurity.isBasicBearer = isBasicBearer;
filteredSecurity.isApiKey = isApiKey;
filteredSecurity.isOAuth = isOAuth;
filteredSecurity.keyParamName = keyParamName;
filteredSecurity.isCode = isCode;
filteredSecurity.isImplicit = isImplicit;
filteredSecurity.isApplication = isApplication;
filteredSecurity.isPassword = isPassword;
filteredSecurity.isKeyInCookie = isKeyInCookie;
filteredSecurity.isKeyInHeader = isKeyInHeader;
filteredSecurity.isKeyInQuery = isKeyInQuery;
filteredSecurity.flow = flow;
filteredSecurity.tokenUrl = tokenUrl;
filteredSecurity.authorizationUrl = authorizationUrl;
// It is not possible to deep copy the extensions, as we have no idea what types they are.
// So the filtered method *will* refer to the original extensions, if any.
filteredSecurity.vendorExtensions = new HashMap<String, Object>(vendorExtensions);
List<Map<String, Object>> returnedScopes = new ArrayList<Map<String, Object>>();
Map<String, Object> lastScope = null;
for (String filterScopeName : filterScopes) {
for (Map<String, Object> scope : scopes) {
String name = (String) scope.get("scope");
if (filterScopeName.equals(name)) {
returnedScopes.add(scope);
lastScope = scope;
break;
}
}
}
filteredSecurity.scopes = returnedScopes;

return filteredSecurity;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -1057,55 +1057,21 @@ private void processOperation(String resourcePath, String httpMethod, Operation
}

Map<String, SecurityScheme> authMethods = getAuthMethods(securities, securitySchemes);
if (authMethods == null || authMethods.isEmpty()) {
authMethods = getAuthMethods(globalSecurities, securitySchemes);
}

if (authMethods != null && !authMethods.isEmpty()) {
codegenOperation.authMethods = config.fromSecurity(authMethods);
List<Map<String, Object>> scopes = new ArrayList<Map<String, Object>>();
if (codegenOperation.authMethods != null) {
for (CodegenSecurity security : codegenOperation.authMethods) {
if (security != null && security.isBasicBearer != null && security.isBasicBearer &&
securities != null) {
for (SecurityRequirement req : securities) {
if (req == null) continue;
for (String key : req.keySet()) {
if (security.name != null && key.equals(security.name)) {
int count = 0;
for (String sc : req.get(key)) {
Map<String, Object> scope = new HashMap<String, Object>();
scope.put("scope", sc);
scope.put("description", "");
count++;
if (req.get(key) != null && count < req.get(key).size()) {
scope.put("hasMore", "true");
} else {
scope.put("hasMore", null);
}
scopes.add(scope);
}
//end this inner for
break;
}
}

}
security.hasScopes = scopes.size() > 0;
security.scopes = scopes;
}
}
}
List<CodegenSecurity> fullAuthMethods = config.fromSecurity(authMethods);
codegenOperation.authMethods = filterAuthMethods(fullAuthMethods, securities);
codegenOperation.hasAuthMethods = true;
}
} else {
authMethods = getAuthMethods(globalSecurities, securitySchemes);

/* TODO need to revise the logic below
Map<String, SecurityScheme> securitySchemeMap = openAPI.getComponents().getSecuritySchemes();
if (securitySchemeMap != null && !securitySchemeMap.isEmpty()) {
codegenOperation.authMethods = config.fromSecurity(securitySchemeMap);
codegenOperation.hasAuthMethods = true;
if (authMethods != null && !authMethods.isEmpty()) {
List<CodegenSecurity> fullAuthMethods = config.fromSecurity(authMethods);
codegenOperation.authMethods = filterAuthMethods(fullAuthMethods, globalSecurities);
codegenOperation.hasAuthMethods = true;
}
}
*/

} catch (Exception ex) {
String msg = "Could not process operation:\n" //
+ " Tag: " + tag + "\n"//
Expand Down Expand Up @@ -1311,6 +1277,40 @@ private static OAuthFlow cloneOAuthFlow(OAuthFlow originFlow, List<String> opera
.scopes(newScopes);
}

private List<CodegenSecurity> filterAuthMethods(List<CodegenSecurity> authMethods, List<SecurityRequirement> securities) {
if (securities == null || securities.isEmpty() || authMethods == null) {
return authMethods;
}

List<CodegenSecurity> result = new ArrayList<CodegenSecurity>();

for (CodegenSecurity security : authMethods) {
boolean filtered = false;
if (security != null && security.scopes != null) {
for (SecurityRequirement requirement : securities) {
List<String> opScopes = requirement.get(security.name);
if (opScopes != null) {
// We have operation-level scopes for this method, so filter the auth method to
// describe the operation auth method with only the scopes that it requires.
// We have to create a new auth method instance because the original object must
// not be modified.
CodegenSecurity opSecurity = security.filterByScopeNames(opScopes);

This comment has been minimized.

Copy link
@jasonculverhouse

jasonculverhouse Nov 22, 2019

Contributor

you need to set opSecurity.hasMore = security.hasMore or else the list will not have the hasMore set correctly

result.add(opSecurity);
filtered = true;
break;
}
}
}

// If we didn't get a filtered version, then we can keep the original auth method.
if (!filtered) {
result.add(security);
}
}

return result;
}

private boolean hasOAuthMethods(List<CodegenSecurity> authMethods) {
for (CodegenSecurity cs : authMethods) {
if (Boolean.TRUE.equals(cs.isOAuth)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,15 @@ namespace {{apiPackage}}
/// <param name="{{paramName}}">{{description}}</param>{{/allParams}}{{#responses}}
/// <response code="{{code}}">{{message}}</response>{{/responses}}
[{{httpMethod}}]
[Route("{{{basePathWithoutHost}}}{{{path}}}")]{{#hasAuthMethods}}{{#authMethods}}{{#isApiKey}}
[Authorize(Policy = "{{name}}")]{{/isApiKey}}{{#isBasicBearer}}
[Authorize{{#hasScopes}}(Roles = "{{#scopes}}{{scope}}{{#hasMore}},{{/hasMore}}{{/scopes}}"){{/hasScopes}}]{{/isBasicBearer}}{{/authMethods}}{{/hasAuthMethods}}
[Route("{{{basePathWithoutHost}}}{{{path}}}")]
{{#authMethods}}
{{#isApiKey}}
[Authorize(Policy = "{{name}}")]
{{/isApiKey}}
{{#isBasicBearer}}
[Authorize{{#scopes}}{{#-first}}(Roles = "{{/-first}}{{scope}}{{^-last}},{{/-last}}{{#-last}}"){{/-last}}{{/scopes}}]
{{/isBasicBearer}}
{{/authMethods}}
[ValidateModelState]{{#useSwashbuckle}}
[SwaggerOperation("{{operationId}}")]{{#responses}}{{#dataType}}
[SwaggerResponse(statusCode: {{code}}, type: typeof({{&dataType}}), description: "{{message}}")]{{/dataType}}{{^dataType}}{{/dataType}}{{/responses}}{{/useSwashbuckle}}{{^useSwashbuckle}}{{#responses}}{{#dataType}}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,20 @@ paths:
responses:
'200':
description: 'OK'
/readonly_auth_scheme:
get:
security:
- authScheme: ["test.read"]
responses:
200:
description: Check that limiting to a single required auth scheme works
/multiple_auth_scheme:
get:
security:
- authScheme: ["test.read", "test.write"]
responses:
200:
description: Check that limiting to multiple required auth schemes works
/responses_with_headers:
get:
responses:
Expand All @@ -123,7 +137,18 @@ paths:
Failure-Info:
schema:
type: String

components:
securitySchemes:
authScheme:
type: oauth2
flows:
authorizationCode:
authorizationUrl: 'http://example.org'
tokenUrl: 'http://example.org'
scopes:
test.read: Allowed to read state.
test.write: Allowed to change state.
schemas:
UuidObject:
description: Test a model containing a UUID
Expand Down
Original file line number Diff line number Diff line change
@@ -1 +1 @@
4.1.1-SNAPSHOT
4.1.3-SNAPSHOT
Loading

0 comments on commit de162f7

Please sign in to comment.