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.

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

Add OidcSession interface #22163

Merged
merged 1 commit into from
Dec 20, 2021
Merged

Conversation

sberyozkin
Copy link
Member

@sberyozkin sberyozkin commented Dec 13, 2021

Fixes #22197
Fixes #21928

This is a draft PR for now as I need to add the tests and update the docs.

Here is what it does:

  • registers RoutingContext tenantId attribute for custom TenantResolvers to avoid having to check the cookie names; it is already reg-ed as a SecurityIdentity attribute as well, so there should be no need to inline in the faked IdToken
  • introduced OidcSession which is a wrapper around the id token and it allows to 1) get tenant id 2) check how long the session will be valid for 3) do the local logout - without the custom code having to deal with the cookies - this last item proved more involved than I thought - but really I just had to move the related code from CodeAuthenticationMechanism to OidcUtils. Removing the cookie has to be handled correctly when the custom path/domain is set, and Uni has to be used to complete the removal of the matching state from the custom TokenStateManager, ex, if it is stored in the DB

CC @FroMage

@sberyozkin sberyozkin force-pushed the oidc_session branch 2 times, most recently from c995277 to dae61f1 Compare December 14, 2021 19:58
@sberyozkin
Copy link
Member Author

This interface is simply a wrapper around IdToken - which can be injected directly as well, but it also allows for a local logout, and in time will grow to provide more interesting methods - since the local OidcSession requires working with both IdToken and RoutingContext.
@pedroigor FYI, @FroMage would like to do a local logout in the code without a global logout which may happen when working Google/etc

@sberyozkin
Copy link
Member Author

I'll add the docs tomorrow

@sberyozkin sberyozkin marked this pull request as ready for review December 15, 2021 15:34
Copy link
Contributor

@pedroigor pedroigor left a comment

Choose a reason for hiding this comment

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

Indeed a very handy utility for interacting with the session.

One question, wouldn't be possible to leverage the SecurityIdentity for that? So that we could have a custom version of this type that adds the OIDC capabilities?

@sberyozkin
Copy link
Member Author

@FroMage, @pedroigor

I've opened it for review and here is the updated summary:

  1. OidcSession is added - it is a helper around a RoutingContext plus IdToken combo. The main task is to support the local logout but it will also offer a few other utility methods and I think we will keep adding more over time. The users can manage some of the tasks by directly injecting either RoutingContext and/or IdToken but IMHO OidcSession is a nice to have addition, with all the credits to @FroMage for his feedback.
  2. OidcAuthenticationMechanism is updated to set a tenant_id RoutingContext attribute before the tenant config resolution starts - as the tenant id is encoded in the state and session cookie names - it is done to support not only OidcSession but the custom tenant resolvers for them to avoid dealing with the cookies directly. (@FroMage FYI, SecurityIdentity also has it set, so you can get tenant-id in the augmentor as well if needed).
  3. CodeAuthenticationMechanism code where a cookie suffix is checked has been updated such that when the default tensant is used, a default tenant name is added to the cookie too, otherwise, if we have say a cookie suffix like test then for a default tenant it will be q_session_test but for tenant1 - q_session_tenant1_test so the code added at 2. may get confused and assume test is a tenant name (also CC @gastaldi, George reviewed the original cookie suffix PR)
  4. CodeAuthenticationMechanism has just had a few simplifications - some code dealing with cookies has moved to OidcUtils for OidcSession be able to use it too, and the OidcAuthenticationMechanism to Code/Bearer calls have been optimized a bit - right now duplicate config resolutions are done on this path, exactly one after another, 2 in a row Uni calls :-)
  5. Tests checking all the updates have been added
  6. Docs added

@FroMage I do hope it will help to simplify some of your code.

@sberyozkin
Copy link
Member Author

Hi @pedroigor

One question, wouldn't be possible to leverage the SecurityIdentity for that? So that we could have a custom version of this type that adds the OIDC capabilities?

Interesting, @FroMage also prototyped a proposal where one would do SecurityIdentity.logout().

IMHO it would be simpler if we have a dedicated type, I suppose OidcSession is a wider concept, we can do something useful there, while SecurityIdentity is used by all Quarkus extensions, logout won't make sense for Basic, smallrye-jwt, and other method even less so... I doubt Stuart will like it. So my proposal is to stay with OidcSession and grow it with the session specific helpers

@sberyozkin
Copy link
Member Author

I need to add another test

@FroMage
Copy link
Member

FroMage commented Dec 15, 2021

logout won't make sense for Basic, smallrye-jwt

It totally does for smallrye-jwt when using cookies ;)

@sberyozkin
Copy link
Member Author

sberyozkin commented Dec 15, 2021

@FroMage, sure, I missed a smallrye-jwt + cookie case, and I can even imagine it works for the form one, that said, even if we end up having SecurityIdentity.logout, this is as much as we can push to SecurityIdentity, I still think it would be good to have an OIDC specific interface to support more than just logout, something that will unlikely make it to SecurityIdentity.

@sberyozkin
Copy link
Member Author

sberyozkin commented Dec 15, 2021

Hi @FroMage @pedroigor So yeah, if you'd like please open a quarkus security issue to add logout to SecurityIdentity but like I said, I don't think it will match the experience which will be offered by OidcSession.
Note in case of OidcSession logout has to return Uni<Void> as the custom token state manager may block.

Also I believe OIDC web-appusers do like injecting IdToken which is a Principal with a get name method plus the access to the claims. So if one needs to do the local logout then they'd have to inject both IdToken and SecurityIdentity - not sure it is great.
But with OidcSession one can actually avoid injecting IdToken parallel to it and do oidcSession.getIdToken().
OidcSession is effectively an IdToken.

So, I'd rather keep going with OidcSession

@sberyozkin
Copy link
Member Author

Updated one of the tests to return a default tenant id instead of null, ready for review

@pedroigor
Copy link
Contributor

I was thinking more about a sub-type rather than changing SecurityIdentity itself.

@sberyozkin
Copy link
Member Author

sberyozkin commented Dec 16, 2021

Hey @pedroigor

I was thinking more about a sub-type rather than changing SecurityIdentity itself.

Do you mean something like OidcSecurityIdentity extends SecurityIdentity or taking the existing PR, OidcSession extends SecurityIdentity ?

That might work, we'd probably need to also add getAccessToken to support the service apps, if its getIdToken is to stay.
I really don't mind now to do another iteration, if you meant this kind of extension then we can just do it easily in the next round when we get to it, we'll just extend SecurityIdentity, inject SecurityIdentity to OidcSessionImpl and delegate to it to support SecurityIdentity. But I'd prefer to discuss it in a follow up issue :-), please open, thanks

@sberyozkin sberyozkin dismissed FroMage’s stale review December 16, 2021 16:36

Comments have been addressed

@sberyozkin
Copy link
Member Author

Hi @FroMage Can you confirm you are OK for this PR to go in ? I think we can review the idea of OidcSession extending SecurityIdentity - if we do it then it won't break anyone's code, but I'd prefer to handle it in a dedicate PR later

@sberyozkin
Copy link
Member Author

sberyozkin commented Dec 20, 2021

Let me go ahead with the merge, we can align OidcSession easily with Securiyidentity if agreed later

@sberyozkin sberyozkin merged commit 9a92ad5 into quarkusio:main Dec 20, 2021
@quarkus-bot quarkus-bot bot added this to the 2.7 - main milestone Dec 20, 2021
@sberyozkin sberyozkin deleted the oidc_session branch December 20, 2021 10:07
@sberyozkin sberyozkin mentioned this pull request Dec 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OIDC: TenantResolver enhancement Introduce OidcSession interface
3 participants