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

Support for combining OIDC with other auth mechanisms #11937

Merged

Conversation

sberyozkin
Copy link
Member

@sberyozkin sberyozkin commented Sep 7, 2020

Related to #11886.
This PR fixes a minor issue in the OIDC code (returning 'null' token credentials when no token is available) to have the OIDC service authentication combined with the other auth mechanims (basic, etc). Note I've updated the test and changed Principal to SecurityIdentity - this is because smallrye-jwt ships a (JsonWebToken) Principal producer and, with the basic auth, it returns a null name.

@gsmet I think it is a bug fix, but it is not an urgent fix as some more work needs to be done around #11886, hence not adding a backport label

@sberyozkin sberyozkin added this to the 1.9.0 - master milestone Sep 7, 2020
@sberyozkin sberyozkin force-pushed the combine_oidc_and_basic_auth branch 3 times, most recently from c5e1538 to 1711697 Compare September 8, 2020 11:54
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.

@sberyozkin Is returning an empty credential instance from the producer OK? Why not just return null as it stands today?

@sberyozkin
Copy link
Member Author

sberyozkin commented Sep 8, 2020

Hi @pedroigor
Right, yes, keeping it as it is now prevents combining OIDC with the other authentication mechanisms which is a nice feature added by Stuart @stuartwdouglas. Example, see the linked issue (its scope is a bit different, there is a requirement to enable Basic for a given path only but combining the mechanisms is a prerequisite). It will also block the cases where one of the JAX-RS methods is @permitAll and the service code has an injected JsonWebToken/TokenCredential.
Hence returning a Null JsonWebToken/TokenCredential instance when no token is available is needed for a RequestScoped bean injection to work (null is not allowed - this is for injecting TokenCredentials, while with JsonWebToken we throw an exception which is also a problem).
I don't think either BearerAuthenticationMechanism or CodeFlowAuthenticationMechanism are affected as they block the requests without tokens :-), we have Bearer and Code flow tests asserting it I believe.
What do you think, does it clarify it ?

@sberyozkin sberyozkin force-pushed the combine_oidc_and_basic_auth branch from 1711697 to 7dfed06 Compare September 8, 2020 14:47
@sberyozkin
Copy link
Member Author

@stuartwdouglas thanks; @pedroigor I'll merge then, as I said, it is simply a RequestScoped requirement, ping me if you still have some concerns :-)

@sberyozkin sberyozkin merged commit 70f40d9 into quarkusio:master Sep 9, 2020
@sberyozkin sberyozkin deleted the combine_oidc_and_basic_auth branch September 9, 2020 10:32
@Kirbylix
Copy link

Hello,

is there a guide how i use oidc (keycloak) and basic auth together ?
regarding this task

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants