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.

Sign up for GitHub

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

Jump to bottom

Better support for security requirement objects #519

Merged
merged 1 commit into from
Apr 19, 2022
Merged

Better support for security requirement objects #519

merged 1 commit into from
Apr 19, 2022

Conversation

Azquelt
Copy link
Member

@Azquelt Azquelt commented Mar 17, 2022 •
edited
Loading

  • Make SecurityRequirementsSet repeatable
  • Add securitySet parameters to OpenAPIDefinition and CallbackOperation
  • Clarify that using @SecurityRequirement alone is equivalent to using
    @SecurityRequirementsSet with one element
  • Add TCK for @SecurityRequirementsSet
  • Add TCK for securitySet in @OpenAPIDefinition

Fixes #468
Fixes #483

Azquelt added a commit to Azquelt/microprofile-open-api that referenced this pull request Mar 17, 2022
@Azquelt
Support for SecurityRequirementsSet
cb1f776 
Support SecurityRequriementsSet, passing the TCKs added to
eclipse#519

Objects which support security requirements now support having both
individual security requirements and security requirements sets.

An individual security requirement is equivalent to a security
requirement set with a single element. Therefore, when reading
annotations, we always process both lists together.
@Azquelt Azquelt mentioned this pull request Mar 17, 2022
Closed
@MikeEdgar MikeEdgar added this to the MP OpenAPI 3.1 milestone Mar 18, 2022
@MikeEdgar
Copy link
Member

@Azquelt thanks for the PR. I'm planning to look at this together with your smallrye-open-api PR in the next few days. I think this also fixes #483, correct?

@Azquelt
Copy link
Member Author

Azquelt commented Mar 21, 2022

Yes, I think it does. I'll note that.

MikeEdgar
MikeEdgar reviewed Mar 25, 2022
View reviewed changes
Copy link
Member

@MikeEdgar MikeEdgar left a comment

Choose a reason for hiding this comment

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

A few comments/questions.

api/src/main/java/org/eclipse/microprofile/openapi/annotations/OpenAPIDefinition.java Outdated Show resolved Hide resolved
api/src/main/java/org/eclipse/microprofile/openapi/annotations/callbacks/CallbackOperation.java Outdated Show resolved Hide resolved
...src/main/java/org/eclipse/microprofile/openapi/annotations/security/SecurityRequirement.java Outdated Show resolved Hide resolved
...ain/java/org/eclipse/microprofile/openapi/annotations/security/SecurityRequirementsSets.java Outdated
Comment on lines 27 to 28
* Represents an array of security requirement sets that can be specified for the operation or at definition level. Only
* one of requirement sets needs be satisfied.
Copy link
Member

Choose a reason for hiding this comment

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

or at definition level

I know this phrase is in the equivalent SecurityRequirements annotation docs as well, but reading it makes it seem that this annotation is supported alongside @OpenAPIDefinition on an Application or package-info. What do you think?

Copy link
Member Author

@Azquelt Azquelt Mar 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think it's very imprecise about where it's supported.

Would it be better to list the places where it is supported? (which would be on a resource method, on a resource class, within @OpenAPIDefinition#securitySet and within @CallbackOperation#securitySet)

Copy link
Member

Choose a reason for hiding this comment

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

I think that makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've gone through and added more detail to the Javadoc for SecurityRequirementsSet and SecurityRequirementsSets. I've also updated SecurityRequirements to remove the suggestion that it can be used at "operation or definition level".

MikeEdgar
MikeEdgar reviewed Apr 14, 2022
View reviewed changes
Copy link
Member

@MikeEdgar MikeEdgar left a comment

Choose a reason for hiding this comment

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

Only one thing came to mind this pass through.

api/src/main/java/org/eclipse/microprofile/openapi/annotations/OpenAPIDefinition.java Outdated Show resolved Hide resolved
MikeEdgar
MikeEdgar approved these changes Apr 14, 2022
View reviewed changes
Copy link
Member

@MikeEdgar MikeEdgar left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

@Azquelt
Better support for security requirement objects
cf946e8 
* Make SecurityRequirementsSet repeatable
* Add securitySets parameters to OpenAPIDefinition and CallbackOperation
* Clarify that using @securityrequirement alone is equivalent to using
@SecurityRequirementsSet with one element
* Add TCK for @SecurityRequirementsSet
* Add TCK for securitySets in @OpenAPIDefinition
@Azquelt
Copy link
Member Author

Azquelt commented Apr 19, 2022

Review commits squashed

@Azquelt Azquelt merged commit 7a80156 into eclipse:master Apr 19, 2022
@Azquelt Azquelt deleted the security-requirement-sets branch April 19, 2022 09:29
Azquelt added a commit to Azquelt/smallrye-open-api that referenced this pull request Apr 21, 2022
@Azquelt
Support for SecurityRequirementsSet
5d57133 
Support SecurityRequriementsSet, passing the TCKs added to
eclipse/microprofile-open-api#519

Objects which support security requirements now support having both
individual security requirements and security requirements sets.

An individual security requirement is equivalent to a security
requirement set with a single element. Therefore, when reading
annotations, we always process both lists together.
MikeEdgar pushed a commit to Azquelt/smallrye-open-api that referenced this pull request Apr 26, 2022
@Azquelt @MikeEdgar
Support for SecurityRequirementsSet
ae9034c 
Support SecurityRequriementsSet, passing the TCKs added to
eclipse/microprofile-open-api#519

Objects which support security requirements now support having both
individual security requirements and security requirements sets.

An individual security requirement is equivalent to a security
requirement set with a single element. Therefore, when reading
annotations, we always process both lists together.
MikeEdgar pushed a commit to Azquelt/smallrye-open-api that referenced this pull request May 4, 2022
@Azquelt @MikeEdgar
Support for SecurityRequirementsSet
ee56501 
Support SecurityRequriementsSet, passing the TCKs added to
eclipse/microprofile-open-api#519

Objects which support security requirements now support having both
individual security requirements and security requirements sets.

An individual security requirement is equivalent to a security
requirement set with a single element. Therefore, when reading
annotations, we always process both lists together.
MikeEdgar pushed a commit to MikeEdgar/smallrye-open-api that referenced this pull request May 7, 2022
@Azquelt @MikeEdgar
Support for SecurityRequirementsSet
dae1cfd 
Support SecurityRequriementsSet, passing the TCKs added to
eclipse/microprofile-open-api#519

Objects which support security requirements now support having both
individual security requirements and security requirements sets.

An individual security requirement is equivalent to a security
requirement set with a single element. Therefore, when reading
annotations, we always process both lists together.
@MikeEdgar MikeEdgar mentioned this pull request May 7, 2022
Merged
@Azquelt Azquelt removed this from the MP OpenAPI 3.1 milestone Jun 13, 2022
@Azquelt Azquelt mentioned this pull request Jun 14, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MikeEdgar MikeEdgar MikeEdgar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support generation of empty SecurityRequirement via annotation @SecurityRequirementsSet not working
2 participants
@Azquelt @MikeEdgar