Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Support injection of the @BeanParam parameters into a custom permission referenced in the @PermissionAllowed security annotation #43353
Support injection of the @BeanParam parameters into a custom permission referenced in the @PermissionAllowed security annotation #43353
Changes from all commits
393565a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 834 in docs/src/main/asciidoc/security-authorize-web-endpoints-reference.adoc
GitHub Actions / Linting with Vale
Check warning on line 927 in docs/src/main/asciidoc/security-authorize-web-endpoints-reference.adoc
GitHub Actions / Linting with Vale
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.
Once again a permission where
this.
members are never used. I don't understand this model.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 model comes from the https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/security/Permission.html#implies(java.security.Permission). In Quarkus terms, the identity has permission checker that can check required permissions. Here it is:
So technically, you only need one required permission and one checker. You can do
requiredPermission.implies(null)
or whatever, but it feels very hacky, so I always expect users will declare bothrequiredPermission
andpossessedPermission
and do:possessedPermission.implies(requiredPermission)
.I expect this can be simplified with your
@RequiredPermission
or@PermissionChecker
or whatever later.BTW this particular
MyPermission
is here to test that bean parameters were passed correctly, I don't think it needs to usethis
for that purpose. I wouldn't write this to the docs.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 can understand
implies
as it's documented, which is about figuring out if, given an already granted permission, another one is implied by it. For example, given that the current user has theadmin
permission, is theread
permission implied? Probably, yes.But this is not what we're describing here with
MyPermission
. I see no relation.SecurityIdentity.checkPermission(Permission)
I also understand: does the current user have the right to do X? Fine. That makes sense.But again, this does not look like what
MyPermission.implies
is doing. Is this something related to the current PR, or the general security model in Quarkus?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.
ad
describing
: it's a test, not docs; but sure, I'll answerWhen you specify
@PermissionsAllowed(permission=CustomPermission)
thenCustomPermission
instance is created and passed to theSecurityIdentity.checkPermission(Permission)
because it is factcheckPermission(requiredPermission)
.It depends on how user writes
SecurityIdentityAugmentor
whetherthis
is used. I am using one and the same permission in the all tests for making it easy, but you can also do this (meta code ...):and this augmentor part can be simplified with #43238.
I cannot simplify this model in this PR.
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.
Oh, so wait, users have to implement a
SecurityIdentityAugmentor
to make this work?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.
It is one option. There are other ways to add
permission checker
on your security identity. For example we map token claims as StringPermission. Or you can remap SecurityIdentity roles like this:Which gives you
var possessed = new StringPermission("read")
Problem with this is that we add permission check like
possessedPermisison.implies(requiredPermission)
. It felt right, but I suppose for purpose of this particular situation,requiredPermisison.implies(possessedPermission)
is equivalent. Though it is not from POV of formal logic.So ATM you do need augmentor, but this part could be changed in this PR if it is requested.
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.
It would be yet another breaking change though.
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.
And if you wonder, if user doesn't specify any permission checker but
@PermissionsAllowed
is applied, theQuarkusSecurityIdentity
responds with false by defaultquarkus/extensions/security/runtime/src/main/java/io/quarkus/security/runtime/QuarkusSecurityIdentity.java
Line 84 in 1603d2c
so you don't have the permission unless you posses at least one that complies it. I think we shouldn't change it in this PR. I'll continue updating the docs with the augmentor.