Add permission checker shortcuts to QuarkusSecurityIdentity.Builder

[sberyozkin]

Fixes #43717.

This PR adds a few QuarkusSecurityIdentity.Builder permission checker shortcuts for users be able to avoid having to write permission checker functions.

You can see the impact on the changed OIDC code which converts scopes to permissions. This code is tested in the oidc-tenancy tests (see #36361).

IMHO we should not be talking about permission checkers in the Permission docs to avoid the confusion, and to try to give a message it is all very simple. We can add some notes later if some users find that simply adding permissions to the identity builder is not enough for them

[github-actions]

🙈 The PR is closed and the preview is expired.

[michalvavrik]

LGTM, thank you, I think this will improve user experience

[gsmet]

I added a very small suggestion.

[sberyozkin]

Thanks @gsmet @michalvavrik, I've tweaked docs, shortcut method signatures.

I've also added unit tests and while working on them I remembered that a conversion like 'read:all' -> new StringPermission("read:all") is not correct, as we detected earlier during the OIDC scope to permission conversion, because it won't work against @PermissionAllowed("read:all") where all is an action and hence new StringPermission("read", "all") is produced...

So I've fixed that and it let me remove all the String to Permission conversion code from the OIDC too...

Should be better now...

[michalvavrik]

No, I didn't expect that addPermission(String) works with actions. I think I reviewed when that javadoc about actions wasn't there? Or I made mistake. You would have to do something like addPermission(String, String...) having varargs for actions. I didn't catch that issue with OIDC, I suppose tests would fail if you didn't remove them? Anyway, I think having method that accepts StringPermission name and actions seemed like a nice shortcut to me.

[sberyozkin]

@michalvavrik We already have a convention enforced at the @PermissionAllowed("read:all") level where all becomes an action, so I'm not sure adding another shortcut like addPermissionAsString(String permission, String... actions) really works.
We'd have to drop addPermissionsAsString(Set<String> permissions) as there is no way to separately specify actions here.
IMHO JavaDocs make it clear, and users have an option to avoid it with addPermission(new StringPermission("read:all")) - though it won't really work with @PermissionAllowed("read:all").

What do you think ?

[michalvavrik]

We already have a convention enforced at the @PermissionAllowed("read:all") level where all becomes an action, so I'm not sure adding another shortcut like addPermissionAsString(String permission, String... actions) really works.

Sure, let's don't do it, I don't mind at all. I just say - I liked it :-). Let's not continue on discussing that, it's ok.

IMHO JavaDocs make it clear, and users have an option to avoid it with addPermission(new StringPermission("read:all"))

new StringPermission("read:all") -> new StringPermission("read", "all") see https://github.com/quarkusio/quarkus-security/blob/main/src/main/java/io/quarkus/security/StringPermission.java#L20

What do you think ?

TBH I am lost, I think that last commit won't do. Let me come back to it tonight, I am just preparing on something. I'll come back to it and try to understand changes properly. Maybe I missed something. Thanks

[sberyozkin]

@michalvavrik Sure there is no rush...

We add these shortcuts to make it easy for users to have constraints like @PermissionAllowed("read:all") enforced.
At the addPermissionAsString(String) level users don't deal with Permission, it is a String, the same String they see in @PermissionAllowed("read:all"). But If we don't split it internally into new StringPermission("read", "all") then @PermissionAllowed("read:all") will fail and users will think why addPermissionAsString("read:all") does not work...

Have a look at the OIDC test resource. Users see read:data. The tokens scopes are read:data too.

Now try to comment out OidcUtils code which converts the scope like read:data into new StringPermission("read", "data") and you'll see a failure...

I wonder if I should just drop the comment that a String like read:all represents a permission name with an action, the fact it is converted internally to new StringPermission("read", "all") is an implementation detail, what matters to users is that addPermissionAsString("read:all") helps to get the @PermissionAllowed("read:all") check passed...

The JavaDoc suggestion that they can avoid the read:all to new StringPermission("read", "all") conversion by going to addPermission(Permission) is misguided because if they do new StringPermission("read:all") then it will fail to imply @PermissionAllowed("read:all").

I'll drop the related comment and let you verify things later...

[sberyozkin]

Dropped the comments related to how the String to Permission conversion works and that should avoid the confusion

[sberyozkin]

However, @michalvavrik, I think StringPermission JavaDocs should mention that users must use new StringPermission("read", "all") for it to work against @PermissionAllowed("read:all"), and @PermissionAllowed JavaDocs should mention that read:all would work against a Permission with the read name and all action, but we can take care of it at the quarkus-security project level...

Take you time please to check things, no rush at all, thanks

[michalvavrik]

I'll comment & review later (maybe tomorrow), just quick note:

However, @michalvavrik, I think StringPermission JavaDocs should mention that users must use new StringPermission("read", "all") for it to work against @PermissionAllowed("read:all"), and @PermissionAllowed JavaDocs should mention that read:all would work against a Permission with the read name and all action, but we can take care of it at the quarkus-security project level...

See https://github.com/quarkusio/quarkus-security/blob/main/src/main/java/io/quarkus/security/PermissionsAllowed.java#L87 and https://github.com/quarkusio/quarkus-security/blob/main/src/main/java/io/quarkus/security/PermissionsAllowed.java#L94. Don't get any better than that :-) I can put it to the StringPermission constructor Javadoc, np.

[quarkus-bot]

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 94f7050.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 94f7050.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

---

Flaky tests - Develocity

⚙️ JVM Tests - JDK 17 Windows

📦 extensions/resteasy-reactive/rest-client/deployment

✖ io.quarkus.rest.client.reactive.stork.StorkResponseTimeLoadBalancerTest.shouldUseFasterService - History

• expected: "hello, Alice" but was: "hello, I'm a slow server" - org.opentest4j.AssertionFailedError

org.opentest4j.AssertionFailedError: 

expected: "hello, Alice"
 but was: "hello, I'm a slow server"
	at io.quarkus.rest.client.reactive.stork.StorkResponseTimeLoadBalancerTest.shouldUseFasterService(StorkResponseTimeLoadBalancerTest.java:58)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)
	at io.quarkus.test.QuarkusUnitTest.runExtensionMethod(QuarkusUnitTest.java:513)
	at io.quarkus.test.QuarkusUnitTest.interceptTestMethod(QuarkusUnitTest.java:427)

[michalvavrik]

Not sure if you changed it again, but it looks good now :-D

[sberyozkin]

@michalvavrik I dropped comments related to informing users that we transform it internally :-).

If that looks good to you now then if we can get one more approval, from Steph or Guillaume, it will be good to go

[sberyozkin]

@gsmet, @FroMage, can you have a look please ? It should be ready to go, minor updates at the Security Augmentor build level but which should make it unnecessary for most case for users to write permission checker functions themselves

[FroMage]

Looks much better this way :)

[sberyozkin]

Thanks @FroMage @michalvavrik @gsmet