-
Notifications
You must be signed in to change notification settings - Fork 533
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
Conversation
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.
- 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<>(); |
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.
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);
}
}
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 original code was not loading the content of the security array.
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.
Ok. I see your point. thanks!
" operationId: testGet\n" + | ||
" security:\n" + | ||
" - openId:\n" + | ||
" - https://scopes.example.com/myScope\n" + |
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.
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
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.
I also added a test following this advice. Thanks!
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.
Great, just one last question. Why not updated "https://scopes.example.com/myScope" to reflect real world scope example. :)
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.
that's actually a real world example jeje. from this ticket #562 :)
Everything looks good to me. |
This PR fixes issue #562