Support OidcProviderClient injection and token revocation

[sberyozkin]

Fixes #44157.

@sschellh asked about an option for the user code be able to revoke tokens (I guess, either access or refresh tokens or both) when one of the Logout events is observed.
It reminded me that in Renarde, for the Apple authentication, this is also a typical requirement.

So this PR:

• makes OidcProviderClient which is used by quarkus-oidc, both injectable and also accessible as a SecurityIdentity attribute
• SecurityEvent listeners can react to logout, and other events like authentication failure, by using OidcProviderClient to revoke tokens if they need to - test is provided
• Or, the user code can use an injected OidcProviderClient to revoke tokens or do additional actions like token introspection. For example, Renarde code can have @Inject OidcSession session; , logout, and then follow up with revoking tokens.

The only remaining thing that I believe should be done is that OidcProviderClient should become an API interface, with the runtime subpackage providing an implementation

[michalvavrik]

This looks good, but my understanding was that you want to move towards unification of the io.quarkus.oidc.client.runtime.OidcClientImpl and io.quarkus.oidc.runtime.OidcProviderClient as part of the #39298. Wouldn't it be smarter to define some common API shared by both clients that you will expose? Otherwise when that happens, you will need to take into considerations users may already sue the OidcProviderClient when making changes.

[github-actions]

🎊 PR Preview e4cab19 has been successfully built and deployed to https://quarkus-pr-main-44993-preview.surge.sh/version/main/guides/

• Images of blog posts older than 3 months are not available.
• Newsletters older than 3 months are not available.

[FroMage]

This looks good, but my understanding was that you want to move towards unification of the io.quarkus.oidc.client.runtime.OidcClientImpl and io.quarkus.oidc.runtime.OidcProviderClient as part of the #39298. Wouldn't it be smarter to define some common API shared by both clients that you will expose? Otherwise when that happens, you will need to take into considerations users may already use the OidcProviderClient when making changes.

I didn't realise we have two client classes. Good point about this being confusing.

Also, this PR doesn't have any docs, which is what I'm most interested in :-/

[sberyozkin]

Hi @FroMage, quarkus-oidc-client is designed to be used independently of quarkus-oidc, primarily for acquiring tokens when no human user is involved, for service applications.

What @michalvavrik pointed out is indeed a target, expand the scope of what quarkus-oidc-client can do, for example, @stianst has ideas about using it in tools, for it be able to do more than just acquiring tokens, but also introspecting them to get their active status, or get some info about the user...

Michal, I actually started thinking about it during the oidc-client-registration work, by pushing abstract json beans which support quarkus-oidc UserInfo and TokenIntrospection down to quarkus-oidc-common, but I'm not sure having a shared OIDC client interface works:

• UserInfo and TokenIntrospection are already part of the quarkus-oidc API, moving them down to some other package will impact quarkus-oidc users with deprecations
• quarkus-oidc client is really focused on authorization code flow, on acquiring verification keys - which is out of scope for quarkus-oidc-client - which is meant to work with any token grant type, which does not need an option to get verification keys

So, as far as quarkus-oidc--client is concerned, thinking even more about it, I'd just add its own TokenIntrospection extending the shared abstract JSON bean and be done with #39298. And may be I'd also do the same for UserInfo but for the type of token grants quarkus-oidc-client is primarily designed to work with, like client_credentials or jwt bearer, UserInfo won't be returned. quarkus-oidc-client can be used to support custom authorization code flow implementations but it is not something the command line tools Stian has in mind can do.

So, to summarize, right now, I'm not certain at all a shared interface idea is worth it given that probably the only missing piece in quarkus-oidc-client is the ability to introspect acquired tokens.

[michalvavrik]

Got it @sberyozkin , I had a look again and I think this is useful. I suppose you will add docs when you make this ready for a review, so just one question:

• until now users didn't really use OidcProviderClient, correct? If so, would it make sense to make it an implementation and expose as CDI bean only an interface that won't be in a runtime module (aka add interface for this class, maybe rename the impl. class)? I think that together with javadoc explaining what the interface methods do would make it nicer

[sberyozkin]

Hi @michalvavrik

until now users didn't really use OidcProviderClient, correct? If so, would it make sense to make it an implementation and expose as CDI bean only an interface that won't be in a runtime module (aka add interface for this class, maybe rename the impl. class)? I think that together with javadoc explaining what the interface methods do would make it nicer

Sure, this is what I was thinking about as well, in the description with the only remaining thing that I believe should be done is that OidcProviderClient should become an API interface, with the runtime subpackage providing an implementation...

I've been thinking quite a lot during the last week about how to share interfaces between quarkus-oidc-client, quarkus-oidc, thanks for suggesting it. For example, I thought, what if we introduce some common interface which offers introspection, userinfo, revocation... But then I was invariably got myself asking, what is the goal, we don't really expect quarkus-oidc users do any of these calls, revocation is really the only action they may want to do, for example, on the logout events or on some other events, like authentication or authorization failures.

I may be repeating it, but I guess trying to come up with a shared interface to support these 2 extensions whose user audiences and goals are different may not be particularly useful and be rather constraining.

I think what we can definitely try to do is to minimize duplication between OidcClientImpl and what will become OidcProviderClientImpl, as you have seen, there is some duplication to setup authentication...

As far as what OidcProviderClient interface should provider I start leaning to starting with a minimum set of methods that users might actually try to use for some practical purposes, namely, revocation, token introspection and userinfo... I don't think quarkus-oidc users should see JWK key and token acquisition methods... We might add them later if necessary to the interface if we will see some evidence users are really interested...

After this PR is reviewed and approved, I can try to do #39298 by going after introducing shared TokenIntrospection and UserInfo beans, I can probably get quarkus-oidc TokenIntrospection and UserInfo just extend the common beans.

[sberyozkin]

@michalvavrik Hi Michal, so I've introduced OidcProviderClient interface. I've probably stared at it and OidcClient interfaces for an hour again :-).
My current thinking is as follows:

• quarkus-oidc-client OidcClient is about supporting a host of token grants to acquire and refresh tokens, used in filters and sometimes directly by the users, it is generic in nature. The current endpoint is not the target of these tokens.
• quarkus-oidc OidcProviderClient is about helping users to enhance the endpoint code which deals with verifying incoming bearer or authorization code flow tokens.
• UserInfo, TokenIntrospection are of primary interest to quarkus-oidc users.
• quarkus-oidc users are not really expected to deal with OidcProviderClient interface directly in most cases, UserInfo, TokenIntrospection will be typically done for them by quarkus-oidc - however this interface will now allow them, for example, to get a delayed UserInfo acquisition, or check the cached token status by introspecting it. Token revocation will most likely be the main case when they may want to use this interface.
• Right now, quarkus-oidc OidcProviderClient does not offer and will most likely not offer, options to get JWK keys to verify, manage the authorization code flow tokens acquisition or refresh - it should really remain under control of quarklus-oidc
• Right now, both interfaces have a bit of intersection, token revocation, and might get more intersection, related to the token introspection, userinfo - I'm not too concerned because the use case where quarkus-oidc-client OidcClient may have to be used to introspect tokens or get UserInfo would most likely be confined to the future CLI tool - these operations won't be used by OIDC client filters - these are are server-side operations. The focus is on helping out users to do fine grained control of logout, token verification failure events to do the revocation for ex.
• I think we may be able to eventually introduce OidcCommonClient interface - which will have the current quarkus-oidc OidcProviderClient methods pushed down to it, and both OidcProviderClient and OidcClient just extending it. That will end up being transparent to users with respect to the token revocation at least - we may be able to achieve the transparency even for the the userinfo and token introspection beans

What are your thoughts about it ? I plan to add some docs next.

[sberyozkin]

@michalvavrik, @FroMage

I've added initial docs. I'm not really focusing there on OidcProviderClient which has JavaDocs, but on the main use case it is supposed to support. Cases where users may want to use it to get UserInfo or do the token introspection are advanced.

[michalvavrik]

I'll read all tomorrow evening @sberyozkin , quite busy on finishing other PR ATM.

[michalvavrik]

Looks very good!

[michalvavrik]

It reads bit strange to me to start with code quarkus.oidc.OidcProviderClient. Not sure if The belongs there, but let see if anyone else things it reads strange too. If not, it's OK.

[sberyozkin]

@michalvavrik Yeah, I'm not sure either :-), let me change this sentence slightly and drop the package name as it will be shown in the example below

[sberyozkin]

@michalvavrik I've kept the package name but restructured a bit, from what I've seen, omitting the seems acceptable when referring to (Java) class names, but it can be easily tuned if needed

[michalvavrik]

Thanks, read well now.

[sberyozkin]

@michalvavrik Thanks for the review.

I've found it a bit tricky to expose some of these methods cleanly. Currently quarkus-oidc has OidcProvider and OidcProviderClient. The latter is only about sending requests, the former if about keeping keys, having some verification logic.

On main, OidcProviderClient returns UserInfoResponse as opposed to UserInfo, because UserInfo can be a signed JWT in which case OidcProvider verifies it and converts JWT to UserInfo. But we can't really expect users do it themselves.

So, in this PR I pushed some of OidcProvider code down to OidcProviderClient so that no matter what format UserInfo is returned in, the user gets UserInfo. This is a bit hacky though, I think eventually, OidcProvider and OidcProviderClient should become a single OidcProviderClient, with the actual verification logic pushed up from OidcProvider up to OidcIdentityProvider - but this is a sensitive work that requires some dedicated PRs at a later stage :-)

[quarkus-bot]

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 8874a06.

✅ 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 8874a06.

✅ 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.

[sberyozkin]

@FroMage Have a look please next week if you can at the docs and comments clarifying some design decisions, I believe with this PR you can have an Apple logout and token revocation issue resolved quite neatly.
Michal is happy enough with the PR.

In general, I'm thinking it will be hard to create a uniform client interface between quarkus-oidc and quarkus-oidc-client extensions because OIDC one is dealing with verifying incoming tokens and the OIDC client one is a generic interface designed to support many types of grants with some OIDC operations like JWKs retrieval not relevant. quarkus-oidc users in most cases do not need to deal with this interface therefore the goals of trying to design some shared interface between two extensions are unclear.

Going forward I may be able to reuse some beans like UserInfo, TokenIntrospection between both types of clients.

[FroMage]

Added comments and questions about the API and docs, but I haven't checked the impl code.

[FroMage]

Are you sure there's a use-case for revoking tokens after a back-channel logout? Aren't the tokens automatically revoked by the OIDC server?

[FroMage]

What's the rationale for obtaining the provider client via an untyped attribute rather than via injection, or a new method on the security identity?

[FroMage]

This looks more useful, as a use-case, because for client-side logouts, the tokens won't be automatically invalidated by the server.

[FroMage]

Are we sure it's better to show a sync endpoint method doing 3 await().indefinitely() rather than show a reactive endpoint with composition?

Opinions, @geoand @cescoffier ?