Skip to content
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

fixing security requeriments #572

Merged
merged 3 commits into from
Nov 15, 2017
Merged

fixing security requeriments #572

merged 3 commits into from
Nov 15, 2017

Conversation

gracekarina
Copy link
Contributor

This PR fixes issue #562

Copy link

@vinscom vinscom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • As minimum JDK 1.8 is required. We should be utilising 1.8 features (Like stream) more while refactoring code.
  • Test data should reflect as much as possible real world example.

securityRequirement.addList(key,node.textValue());
if (securityRequirement != null && securityRequirement.size() > 0){
securityRequirements.add(securityRequirement);
List<String> scopes = new ArrayList<>();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think original code is fine except it is not loading scope. Whole function refactoring is not required.
For example:

JsonNode value = node.get(key);
if (key != null && JsonNodeType.ARRAY.equals(value.getNodeType())) {
    ArrayNode arrayNode = (ArrayNode)value;
    List<String> scopes = Stream
            .generate(arrayNode.elements()::next)
            .map((n) -> n.asText())
            .limit(arrayNode.size())
            .collect(Collectors.toList());
    securityRequirement.addList(key,scopes);
    if (securityRequirement.size() > 0){
        securityRequirements.add(securityRequirement);
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original code was not loading the content of the security array.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I see your point. thanks!

" operationId: testGet\n" +
" security:\n" +
" - openId:\n" +
" - https://scopes.example.com/myScope\n" +
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As scope is used for access control of API. Above scope is not good example of "scope".
You can take sample value from original Open API 3 spec.
For example:

openId_auth:
- write:pets
- read:pets

or

openId_auth:
- write
- read

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also added a test following this advice. Thanks!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, just one last question. Why not updated "https://scopes.example.com/myScope" to reflect real world scope example. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's actually a real world example jeje. from this ticket #562 :)

@vinscom
Copy link

vinscom commented Nov 8, 2017

Everything looks good to me.

@gracekarina gracekarina merged commit 98f27ba into 2.0 Nov 15, 2017
@gracekarina gracekarina deleted the fix/security-requeriments branch February 7, 2018 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants