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

OIDC document shoud update #32143

Closed
SetoKaiba opened this issue Mar 26, 2023 · 11 comments · Fixed by #42938
Closed

OIDC document shoud update #32143

SetoKaiba opened this issue Mar 26, 2023 · 11 comments · Fixed by #42938
Labels
area/oidc kind/bug Something isn't working triage/needs-feedback We are waiting for feedback.
Milestone

Comments

@SetoKaiba
Copy link
Contributor

Describe the bug

https://quarkus.io/guides/security-openid-connect#testing-the-application
username=alice&password=alice&grant_type=password should change to username=alice&password=alice&grant_type=password&scope=openid

Or the token response scope will be only with "profile email". No "openid". Then the authorization will fail.

Expected behavior

No response

Actual behavior

No response

How to Reproduce?

No response

Output of uname -a or ver

No response

Output of java -version

No response

GraalVM version (if different from Java)

No response

Quarkus version or git rev

No response

Build tool (ie. output of mvnw --version or gradlew --version)

No response

Additional information

No response

@SetoKaiba SetoKaiba added the kind/bug Something isn't working label Mar 26, 2023
@quarkus-bot
Copy link

quarkus-bot bot commented Mar 26, 2023

/cc @pedroigor (oidc), @sberyozkin (oidc)

@sberyozkin
Copy link
Member

@SetoKaiba I'm not sure I understand, openid scope is needed for the authorization code flow. Do those documented steps how to test fail for you ?

@SetoKaiba
Copy link
Contributor Author

SetoKaiba commented Mar 26, 2023

I didn't try @RolesAllowed approach.

But for the below documentation.
https://quarkus.io/guides/security-keycloak-authorization#testing-the-application
At least, for this, I use the keycloak to centralize the authorization. My service will return 401 without scope=openid.

Because I'm writing a standalone application without keycloak-js. So I have to generate the access token myself. I use the way the documentation mentioned. I use that access token unable to access to my service. Then I compared the two token response. I found that the keycloak-js one is with scope openid. But mine doesn't. My access is working again after I added &scope=openid.

@sberyozkin
Copy link
Member

sberyozkin commented Mar 26, 2023

@SetoKaiba I've re-tested quarkus-quickstarts/security-keycloak-authorization-quickstart (which does not use @RolesAllowed but the authorization policy configured in the realm) and it works as expected.

I'm not sure why openid scope is needed when the password grant is used to get a token as this scope is nothing to do with the password grant - it is the OIDC authorization code flow or implicit flow level scope. keycloak.js must be using either the authorization code flow or may be implicit and therefore it uses openid.

I appreciate this is what you have to do, add an openid scope for your setup to work, but the docs you linked to describe how to test quarkus-qucikstarts/security-keycloak-authorization-quickstart, where the token is acquired using a password grant, which is a core and (deprecated) lower level, OAuth2 grant where openid is an unknown concept. So require users to add this scope even if they don't need it to successfully test quarkus-qucikstarts/security-keycloak-authorization-quickstart would be confusing IMHO

@SetoKaiba
Copy link
Contributor Author

Thank you. I found out the problem. This require the openid scope. This is not set in the example but it's mentioned in the doc. So I use it to get more user info. Then I found the problem today with the password grant workflow. Should this be mentioned or is it a common sense for the developers with OIDC?

quarkus.oidc.authentication.user-info-required=true

@sberyozkin
Copy link
Member

@SetoKaiba I see, if you use the code flow then Quarkus itself will add this scope, or if it is SPA then keycloak.js will do it. However, for testing, where AT is retrived manually (DevServices for Keycloak will add it itself) and if this token is expected to support the UserInfo retrieval, then indeed, I see why this scope has to be added.

This warrants a Note for sure, to the keycloak authorization and oidc bearer token docs. Would you like to add such a note to both docs, just beneath the example showing how to get the token with curl ?

@SetoKaiba
Copy link
Contributor Author

I'd like to.

For the guides of the website, is the repo path below that I should add a note and create a PR to?

https://github.com/quarkusio/quarkusio.github.io/tree/develop/_guides

I found that there's security related adoc in the main repo as well. But some guides in the mainsite is missing here.

https://github.com/quarkusio/quarkus/tree/main/docs/src/main/asciidoc

Which one should I create a PR to add the note? Thank you.

@sberyozkin
Copy link
Member

@SetoKaiba Thanks,
https://github.com/quarkusio/quarkus/blob/main/docs/src/main/asciidoc/security-keycloak-authorization.adoc#testing-the-application is the section you linked, so at the very end of this section, please add a note showing how to request a token if this token will have to support the UserInfo retrieval in the tested application.

And please do the same here:
https://github.com/quarkusio/quarkus/blob/main/docs/src/main/asciidoc/security-oidc-bearer-token-authentication-tutorial.adoc#test-the-application

@gsmet
Copy link
Member

gsmet commented Aug 21, 2024

@sberyozkin @michalvavrik could you take care of the doc addition or close this issue as not planned? Thanks.

@gsmet gsmet added the triage/needs-feedback We are waiting for feedback. label Aug 21, 2024
@SetoKaiba
Copy link
Contributor Author

Sorry that I was busy. I'll create a PR this weekend.

@michalvavrik
Copy link
Member

Sorry that I was busy. I'll create a PR this weekend.

No hurry, I think adding note makes sense. Thank you

SetoKaiba added a commit to SetoKaiba/quarkus that referenced this issue Sep 2, 2024
SetoKaiba added a commit to SetoKaiba/quarkus that referenced this issue Sep 2, 2024
SetoKaiba added a commit to SetoKaiba/quarkus that referenced this issue Sep 2, 2024
@quarkus-bot quarkus-bot bot added this to the 3.16 - main milestone Sep 2, 2024
danielsoro pushed a commit to danielsoro/quarkus that referenced this issue Sep 20, 2024
bschuhmann pushed a commit to bschuhmann/quarkus that referenced this issue Nov 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/oidc kind/bug Something isn't working triage/needs-feedback We are waiting for feedback.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants