From 67b376633220bb2c1122a5408862b8e68cefc65c Mon Sep 17 00:00:00 2001 From: Mike Wilkes Date: Fri, 5 Apr 2019 06:49:29 -0400 Subject: [PATCH] [Issue#392] Correct issue with OAuth scopes not propogated correctly (#1982) If a path defined security to an OAuth type, and defined scopes, the scopes from the components definition were still being used, rather than the (most likely shorter) list of specific scopes for the path. This copies all the component security information over EXCEPT for the scopes. The scopes to be included are determined by the path's security settings. NOTE: Modified the petstore.yaml file so the GET operations only have read:pets scope and utilized the Kotlin server sample to verify output. Sample output updated only for this scenario --- .../codegen/DefaultGenerator.java | 68 ++++++++++++- .../codegen/java/JavaClientCodegenTest.java | 67 ++++++++++--- .../src/test/resources/3_0/issue392.yaml | 99 +++++++++++++++++++ .../src/test/resources/3_0/petstore.yaml | 2 - .../kotlin/org/openapitools/api/PetApi.kt | 4 +- .../kotlin/org/openapitools/model/Category.kt | 24 ----- 6 files changed, 216 insertions(+), 48 deletions(-) create mode 100644 modules/openapi-generator/src/test/resources/3_0/issue392.yaml delete mode 100644 samples/server/petstore/kotlin-springboot/src/main/kotlin/org/openapitools/model/Category.kt diff --git a/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultGenerator.java b/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultGenerator.java index 8a566e21990..2438781a0b7 100644 --- a/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultGenerator.java +++ b/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultGenerator.java @@ -29,8 +29,7 @@ import io.swagger.v3.oas.models.info.Info; import io.swagger.v3.oas.models.info.License; import io.swagger.v3.oas.models.media.Schema; import io.swagger.v3.oas.models.parameters.Parameter; -import io.swagger.v3.oas.models.security.SecurityRequirement; -import io.swagger.v3.oas.models.security.SecurityScheme; +import io.swagger.v3.oas.models.security.*; import io.swagger.v3.oas.models.tags.Tag; import org.apache.commons.io.IOUtils; import org.apache.commons.lang3.ObjectUtils; @@ -1177,16 +1176,77 @@ public class DefaultGenerator extends AbstractGenerator implements Generator { } final Map authMethods = new HashMap<>(); for (SecurityRequirement requirement : securities) { - for (String key : requirement.keySet()) { + for (Map.Entry> entry : requirement.entrySet()) { + final String key = entry.getKey(); SecurityScheme securityScheme = securitySchemes.get(key); if (securityScheme != null) { - authMethods.put(key, securityScheme); + + if (securityScheme.getType().equals(SecurityScheme.Type.OAUTH2)) { + OAuthFlows oautUpdatedFlows = new OAuthFlows(); + oautUpdatedFlows.extensions(securityScheme.getFlows().getExtensions()); + + SecurityScheme oauthUpdatedScheme = new SecurityScheme() + .type(securityScheme.getType()) + .description(securityScheme.getDescription()) + .name(securityScheme.getName()) + .$ref(securityScheme.get$ref()) + .in(securityScheme.getIn()) + .scheme(securityScheme.getScheme()) + .bearerFormat(securityScheme.getBearerFormat()) + .openIdConnectUrl(securityScheme.getOpenIdConnectUrl()) + .extensions(securityScheme.getExtensions()) + .flows(oautUpdatedFlows); + + // Ensure inserted AuthMethod only contains scopes of actual operation, and not all of them defined in the Security Component + // have to iterate through and create new SecurityScheme objects with the scopes 'fixed/updated' + final OAuthFlows securitySchemeFlows = securityScheme.getFlows(); + + + if (securitySchemeFlows.getAuthorizationCode() != null) { + OAuthFlow updatedFlow = cloneOAuthFlow(securitySchemeFlows.getAuthorizationCode(), entry.getValue()); + + oautUpdatedFlows.setAuthorizationCode(updatedFlow); + } + if (securitySchemeFlows.getImplicit() != null) { + OAuthFlow updatedFlow = cloneOAuthFlow(securitySchemeFlows.getImplicit(), entry.getValue()); + + oautUpdatedFlows.setImplicit(updatedFlow); + } + if (securitySchemeFlows.getPassword() != null) { + OAuthFlow updatedFlow = cloneOAuthFlow(securitySchemeFlows.getPassword(), entry.getValue()); + + oautUpdatedFlows.setPassword(updatedFlow); + } + if (securitySchemeFlows.getClientCredentials() != null) { + OAuthFlow updatedFlow = cloneOAuthFlow(securitySchemeFlows.getClientCredentials(), entry.getValue()); + + oautUpdatedFlows.setClientCredentials(updatedFlow); + } + + authMethods.put(key, oauthUpdatedScheme); + } else { + authMethods.put(key, securityScheme); + } } } } return authMethods; } + private static OAuthFlow cloneOAuthFlow(OAuthFlow originFlow, List operationScopes) { + Scopes newScopes = new Scopes(); + for (String operationScope : operationScopes) { + newScopes.put(operationScope, originFlow.getScopes().get(operationScope)); + } + + return new OAuthFlow() + .authorizationUrl(originFlow.getAuthorizationUrl()) + .tokenUrl(originFlow.getTokenUrl()) + .refreshUrl(originFlow.getRefreshUrl()) + .extensions(originFlow.getExtensions()) + .scopes(newScopes); + } + private boolean hasOAuthMethods(List authMethods) { for (CodegenSecurity cs : authMethods) { if (cs.isOAuth) { diff --git a/modules/openapi-generator/src/test/java/org/openapitools/codegen/java/JavaClientCodegenTest.java b/modules/openapi-generator/src/test/java/org/openapitools/codegen/java/JavaClientCodegenTest.java index f93da79fba3..68c556e5b09 100644 --- a/modules/openapi-generator/src/test/java/org/openapitools/codegen/java/JavaClientCodegenTest.java +++ b/modules/openapi-generator/src/test/java/org/openapitools/codegen/java/JavaClientCodegenTest.java @@ -36,6 +36,9 @@ import java.nio.file.Files; import java.util.*; import java.util.stream.Collectors; +import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertTrue; + public class JavaClientCodegenTest { @Test @@ -47,7 +50,7 @@ public class JavaClientCodegenTest { RequestBody body1 = new RequestBody(); body1.setDescription("A list of ids"); body1.setContent(new Content().addMediaType("application/json", new MediaType().schema(new ArraySchema().items(new StringSchema())))); - CodegenParameter codegenParameter1 = codegen.fromRequestBody(body1 , new HashSet(), null); + CodegenParameter codegenParameter1 = codegen.fromRequestBody(body1, new HashSet(), null); Assert.assertEquals(codegenParameter1.description, "A list of ids"); Assert.assertEquals(codegenParameter1.dataType, "List"); Assert.assertEquals(codegenParameter1.baseType, "List"); @@ -55,7 +58,7 @@ public class JavaClientCodegenTest { RequestBody body2 = new RequestBody(); body2.setDescription("A list of list of values"); body2.setContent(new Content().addMediaType("application/json", new MediaType().schema(new ArraySchema().items(new ArraySchema().items(new IntegerSchema()))))); - CodegenParameter codegenParameter2 = codegen.fromRequestBody(body2 , new HashSet(), null); + CodegenParameter codegenParameter2 = codegen.fromRequestBody(body2, new HashSet(), null); Assert.assertEquals(codegenParameter2.description, "A list of list of values"); Assert.assertEquals(codegenParameter2.dataType, "List>"); Assert.assertEquals(codegenParameter2.baseType, "List"); @@ -67,7 +70,7 @@ public class JavaClientCodegenTest { point.addProperties("message", new StringSchema()); point.addProperties("x", new IntegerSchema().format(SchemaTypeUtil.INTEGER32_FORMAT)); point.addProperties("y", new IntegerSchema().format(SchemaTypeUtil.INTEGER32_FORMAT)); - CodegenParameter codegenParameter3 = codegen.fromRequestBody(body3 , new HashSet(), null); + CodegenParameter codegenParameter3 = codegen.fromRequestBody(body3, new HashSet(), null); Assert.assertEquals(codegenParameter3.description, "A list of points"); Assert.assertEquals(codegenParameter3.dataType, "List"); Assert.assertEquals(codegenParameter3.baseType, "List"); @@ -84,7 +87,7 @@ public class JavaClientCodegenTest { } @Test - public void testParametersAreCorrectlyOrderedWhenUsingRetrofit(){ + public void testParametersAreCorrectlyOrderedWhenUsingRetrofit() { JavaClientCodegen javaClientCodegen = new JavaClientCodegen(); javaClientCodegen.setLibrary(JavaClientCodegen.RETROFIT_2); @@ -102,9 +105,9 @@ public class JavaClientCodegenTest { javaClientCodegen.postProcessOperationsWithModels(objs, Collections.emptyList()); Assert.assertEquals(Arrays.asList(pathParam1, pathParam2, queryParamRequired, queryParamOptional), codegenOperation.allParams); - Assert.assertTrue(pathParam1.hasMore); - Assert.assertTrue(pathParam2.hasMore); - Assert.assertTrue(queryParamRequired.hasMore); + assertTrue(pathParam1.hasMore); + assertTrue(pathParam2.hasMore); + assertTrue(queryParamRequired.hasMore); Assert.assertFalse(queryParamOptional.hasMore); } @@ -149,7 +152,7 @@ public class JavaClientCodegenTest { codegen.additionalProperties().put(CodegenConstants.HIDE_GENERATION_TIMESTAMP, "true"); codegen.additionalProperties().put(CodegenConstants.MODEL_PACKAGE, "xyz.yyyyy.zzzzzzz.mmmmm.model"); codegen.additionalProperties().put(CodegenConstants.API_PACKAGE, "xyz.yyyyy.zzzzzzz.aaaaa.api"); - codegen.additionalProperties().put(CodegenConstants.INVOKER_PACKAGE,"xyz.yyyyy.zzzzzzz.iiii.invoker"); + codegen.additionalProperties().put(CodegenConstants.INVOKER_PACKAGE, "xyz.yyyyy.zzzzzzz.iiii.invoker"); codegen.processOpts(); Assert.assertEquals(codegen.additionalProperties().get(CodegenConstants.HIDE_GENERATION_TIMESTAMP), Boolean.TRUE); @@ -190,7 +193,7 @@ public class JavaClientCodegenTest { Assert.assertEquals(codegen.getInvokerPackage(), "xyz.yyyyy.zzzzzzz.mmmmm"); Assert.assertEquals(codegen.additionalProperties().get(CodegenConstants.INVOKER_PACKAGE), "xyz.yyyyy.zzzzzzz.mmmmm"); } - + @Test public void testGetSchemaTypeWithComposedSchemaWithAllOf() { final OpenAPI openAPI = TestUtils.parseSpec("src/test/resources/2_0/composed-allof.yaml"); @@ -213,7 +216,7 @@ public class JavaClientCodegenTest { Assert.assertNotNull(enumVars); Map testedEnumVar = enumVars.get(0); Assert.assertNotNull(testedEnumVar); - Assert.assertEquals(testedEnumVar.getOrDefault("name", ""),"NUMBER_1"); + Assert.assertEquals(testedEnumVar.getOrDefault("name", ""), "NUMBER_1"); Assert.assertEquals(testedEnumVar.getOrDefault("value", ""), "1"); } @@ -229,7 +232,7 @@ public class JavaClientCodegenTest { Assert.assertNotNull(enumVars); Map testedEnumVar = enumVars.get(0); Assert.assertNotNull(testedEnumVar); - Assert.assertEquals(testedEnumVar.getOrDefault("name", ""),"ONE"); + Assert.assertEquals(testedEnumVar.getOrDefault("name", ""), "ONE"); Assert.assertEquals(testedEnumVar.getOrDefault("value", ""), "1"); } @@ -293,7 +296,7 @@ public class JavaClientCodegenTest { String defaultApiFilename = new File(output, "src/main/java/xyz/abcdef/api/DefaultApi.java").getAbsolutePath().replace("\\", "/"); String defaultApiConent = generatedFiles.get(defaultApiFilename); - Assert.assertTrue(defaultApiConent.contains("public class DefaultApi")); + assertTrue(defaultApiConent.contains("public class DefaultApi")); WrittenTemplateBasedFile templateBasedFile = TestUtils.getTemplateBasedFile(generator, output, "src/main/java/xyz/abcdef/api/DefaultApi.java"); Assert.assertEquals(templateBasedFile.getTemplateData().get("classname"), "DefaultApi"); @@ -316,6 +319,38 @@ public class JavaClientCodegenTest { Assert.assertEquals("Request", header.baseName); } + @Test + public void testAuthorizationScopeValues_Issue392() { + final OpenAPI openAPI = TestUtils.parseSpec("src/test/resources/3_0/issue392.yaml"); + + final DefaultGenerator defaultGenerator = new DefaultGenerator(); + + final ClientOptInput clientOptInput = new ClientOptInput(); + clientOptInput.setOpenAPI(openAPI); + clientOptInput.setConfig(new JavaClientCodegen()); + + final ClientOpts clientOpts = new ClientOpts(); + clientOpts.setProperties(Collections.emptyMap()); + clientOptInput.setOpts(clientOpts); + + defaultGenerator.opts(clientOptInput); + final List codegenOperations = defaultGenerator.processPaths(openAPI.getPaths()).get("Pet"); + + // Verify GET only has 'read' scope + final CodegenOperation getCodegenOperation = codegenOperations.stream().filter(it -> it.httpMethod.equals("GET")).collect(Collectors.toList()).get(0); + assertTrue(getCodegenOperation.hasAuthMethods); + assertEquals(getCodegenOperation.authMethods.size(), 1); + final List> getScopes = getCodegenOperation.authMethods.get(0).scopes; + assertEquals(getScopes.size(), 1, "GET scopes don't match. actual::" + getScopes); + + // POST operation should have both 'read' and 'write' scope on it + final CodegenOperation postCodegenOperation = codegenOperations.stream().filter(it -> it.httpMethod.equals("POST")).collect(Collectors.toList()).get(0); + assertTrue(postCodegenOperation.hasAuthMethods); + assertEquals(postCodegenOperation.authMethods.size(), 1); + final List> postScopes = postCodegenOperation.authMethods.get(0).scopes; + assertEquals(postScopes.size(), 2, "POST scopes don't match. actual:" + postScopes); + } + @Test public void testFreeFormObjects() { final OpenAPI openAPI = TestUtils.parseSpec("src/test/resources/3_0/issue796.yaml"); @@ -352,11 +387,11 @@ public class JavaClientCodegenTest { private void ensureContainsFile(Map generatedFiles, File root, String filename) { File file = new File(root, filename); String absoluteFilename = file.getAbsolutePath().replace("\\", "/"); - if(!generatedFiles.containsKey(absoluteFilename)) { - Assert.fail("Could not find '" + absoluteFilename + "' file in list:\n" + + if (!generatedFiles.containsKey(absoluteFilename)) { + Assert.fail("Could not find '" + absoluteFilename + "' file in list:\n" + generatedFiles.keySet().stream().sorted().collect(Collectors.joining(",\n"))); } - Assert.assertTrue(generatedFiles.containsKey(absoluteFilename), "File '" + absoluteFilename + "' was not fould in the list of generated files"); + assertTrue(generatedFiles.containsKey(absoluteFilename), "File '" + absoluteFilename + "' was not fould in the list of generated files"); } private CodegenProperty codegenPropertyWithArrayOfIntegerValues() { @@ -385,7 +420,7 @@ public class JavaClientCodegenTest { return codegenParameter; } - private CodegenParameter createStringParam(String name){ + private CodegenParameter createStringParam(String name) { CodegenParameter codegenParameter = new CodegenParameter(); codegenParameter.paramName = name; codegenParameter.baseName = name; diff --git a/modules/openapi-generator/src/test/resources/3_0/issue392.yaml b/modules/openapi-generator/src/test/resources/3_0/issue392.yaml new file mode 100644 index 00000000000..14bbc723fec --- /dev/null +++ b/modules/openapi-generator/src/test/resources/3_0/issue392.yaml @@ -0,0 +1,99 @@ +openapi: 3.0.0 +servers: + - url: 'http://petstore.swagger.io/v2' +info: + description: Used for verification of AuthorizationScope resolution issue + version: 1.0.0 + title: OpenAPI Petstore + license: + name: Apache-2.0 + url: 'http://www.apache.org/licenses/LICENSE-2.0.html' +paths: + /pet: + post: + tags: + - pet + summary: Add a new pet to the store + description: '' + operationId: addPet + responses: + '405': + description: Invalid input + security: + - petstore_auth: + - 'write:pets' + - 'read:pets' + requestBody: + content: + application/json: + schema: + $ref: '#/components/schemas/Pet' + '/pet/{petId}': + get: + tags: + - pet + summary: Find pet by ID + description: Returns a single pet + operationId: getPetById + parameters: + - name: petId + in: path + description: ID of pet to return + required: true + schema: + type: integer + format: int64 + responses: + '200': + description: successful operation + content: + application/json: + schema: + $ref: '#/components/schemas/Pet' + '400': + description: Invalid ID supplied + '404': + description: Pet not found + security: + - petstore_auth: + - 'read:pets' +components: + securitySchemes: + petstore_auth: + type: oauth2 + flows: + implicit: + authorizationUrl: 'http://petstore.swagger.io/api/oauth/dialog' + scopes: + 'write:pets': modify pets in your account + 'read:pets': read your pets + api_key: + type: apiKey + name: api_key + in: header + schemas: + Pet: + title: a Pet + description: A pet for sale in the pet store + type: object + required: + - name + - photoUrls + properties: + id: + type: integer + format: int64 + name: + type: string + example: doggie + photoUrls: + type: array + items: + type: string + status: + type: string + description: pet status in the store + enum: + - available + - pending + - sold diff --git a/modules/openapi-generator/src/test/resources/3_0/petstore.yaml b/modules/openapi-generator/src/test/resources/3_0/petstore.yaml index a89eb653265..5cddb68050e 100644 --- a/modules/openapi-generator/src/test/resources/3_0/petstore.yaml +++ b/modules/openapi-generator/src/test/resources/3_0/petstore.yaml @@ -94,7 +94,6 @@ paths: description: Invalid status value security: - petstore_auth: - - 'write:pets' - 'read:pets' /pet/findByTags: get: @@ -141,7 +140,6 @@ paths: description: Invalid tag value security: - petstore_auth: - - 'write:pets' - 'read:pets' deprecated: true '/pet/{petId}': diff --git a/samples/server/openapi3/petstore/kotlin-springboot/src/main/kotlin/org/openapitools/api/PetApi.kt b/samples/server/openapi3/petstore/kotlin-springboot/src/main/kotlin/org/openapitools/api/PetApi.kt index 0a3515cb1ca..8f49e172090 100644 --- a/samples/server/openapi3/petstore/kotlin-springboot/src/main/kotlin/org/openapitools/api/PetApi.kt +++ b/samples/server/openapi3/petstore/kotlin-springboot/src/main/kotlin/org/openapitools/api/PetApi.kt @@ -66,7 +66,7 @@ class PetApiController(@Autowired(required = true) val service: PetApiService) { notes = "Multiple status values can be provided with comma separated strings", response = Pet::class, responseContainer = "List", - authorizations = [Authorization(value = "petstore_auth", scopes = [AuthorizationScope(scope = "write:pets", description = "modify pets in your account"), AuthorizationScope(scope = "read:pets", description = "read your pets")])]) + authorizations = [Authorization(value = "petstore_auth", scopes = [AuthorizationScope(scope = "read:pets", description = "read your pets")])]) @ApiResponses( value = [ApiResponse(code = 200, message = "successful operation", response = Pet::class, responseContainer = "List"),ApiResponse(code = 400, message = "Invalid status value")]) @RequestMapping( @@ -83,7 +83,7 @@ class PetApiController(@Autowired(required = true) val service: PetApiService) { notes = "Multiple tags can be provided with comma separated strings. Use tag1, tag2, tag3 for testing.", response = Pet::class, responseContainer = "List", - authorizations = [Authorization(value = "petstore_auth", scopes = [AuthorizationScope(scope = "write:pets", description = "modify pets in your account"), AuthorizationScope(scope = "read:pets", description = "read your pets")])]) + authorizations = [Authorization(value = "petstore_auth", scopes = [AuthorizationScope(scope = "read:pets", description = "read your pets")])]) @ApiResponses( value = [ApiResponse(code = 200, message = "successful operation", response = Pet::class, responseContainer = "List"),ApiResponse(code = 400, message = "Invalid tag value")]) @RequestMapping( diff --git a/samples/server/petstore/kotlin-springboot/src/main/kotlin/org/openapitools/model/Category.kt b/samples/server/petstore/kotlin-springboot/src/main/kotlin/org/openapitools/model/Category.kt deleted file mode 100644 index c34cec48df1..00000000000 --- a/samples/server/petstore/kotlin-springboot/src/main/kotlin/org/openapitools/model/Category.kt +++ /dev/null @@ -1,24 +0,0 @@ -package org.openapitools.model - -import java.util.Objects -import com.fasterxml.jackson.annotation.JsonProperty -import javax.validation.Valid -import javax.validation.constraints.* -import io.swagger.annotations.ApiModelProperty - -/** - * A category for a pet - * @param id - * @param name - */ -data class Category ( - - @ApiModelProperty(example = "null", value = "") - @JsonProperty("id") val id: Long? = null, - - @ApiModelProperty(example = "null", value = "") - @JsonProperty("name") val name: String? = null -) { - -} -