Updating selected OIDC/OpenID guides #42846
Conversation
quarkus-bot
bot
added
area/docstyle
|
🙈 The PR is closed and the preview is expired. |
This comment has been minimized.
This comment has been minimized.
jedla97
force-pushed
the
oicd-docs-small-fixes
branch
from
a5e0eec
to
a727fec
Compare
This comment has been minimized.
This comment has been minimized.
| @@ -191,7 +192,7 @@ public class CustomTenantResolver implements TenantConfigResolver { | |||
| config.setApplicationType(ApplicationType.HYBRID); | |||
| return Uni.createFrom().item(config); | |||
| } else { | |||
| // resolve to default tenant config | |||
| context.put(OidcUtils.TENANT_ID_ATTRIBUTE, OidcUtils.DEFAULT_TENANT_ID); | |||
There was a problem hiding this comment.
@jedla97 Custom resolvers are not required to set the default tenant id manually, returning null is enough
There was a problem hiding this comment.
@sberyozkin as this is guide with quickstart the code should be same as close as possible. So I looked in quickstart and see it there https://github.com/quarkusio/quarkus-quickstarts/blob/main/security-openid-connect-multi-tenancy-quickstart/src/main/java/org/acme/quickstart/oidc/CustomTenantResolver.java#L34
Also the quickstart test was failing for me without this. I now find why and also see that in test section the coverage is outdated from quickstart. I'll look into that.
There was a problem hiding this comment.
@jedla97 I'd not worry about it, I think it is there to support a test where the user switches between multiple realms while being already logged into one of them. It is not something that we should recommend, manually deal with setting the default property by default.
This specific case is covered at the very end of https://quarkus.io/guides/security-openid-connect-multitenancy#tenant-resolution-for-web-app
There was a problem hiding this comment.
@sberyozkin Ok I'll leave it as it is now.
| @@ -209,8 +210,10 @@ Otherwise, it initiates an authorization code flow when authentication is requir | |||
| [source,properties] | |||
| ---- | |||
| # Default tenant configuration | |||
| %prod.quarkus.oidc.auth-server-url=http://localhost:8180/realms/quarkus | |||
| %prod.keycloak.url=http://localhost:8180 | |||
There was a problem hiding this comment.
Why can't a single property be used ? It looks more complex now
There was a problem hiding this comment.
I change it to be cloaser to quickstart but when I think about it let's leave it as %prod.quarkus.oidc.auth-server-url=http://localhost:8180/realms/quarkus
There was a problem hiding this comment.
Hi @jedla97 Thanks for the careful review, I have a couple of questions, but otherwise LGTM
jedla97
force-pushed
the
oicd-docs-small-fixes
branch
from
a727fec
to
36c2857
Compare
| @@ -233,13 +235,16 @@ Alternatively, you can configure the tenant `tenant-a` directly in `application. | |||
| [source,properties] | |||
| ---- | |||
| # Default tenant configuration | |||
| %prod.quarkus.oidc.auth-server-url=http://localhost:8180/realms/quarkus | |||
| %prod.keycloak.url=http://localhost:8180 | |||
There was a problem hiding this comment.
@jedla97 Please keep a single property here too
There was a problem hiding this comment.
@jedla97 In general, we'd like to avoid highlighting Keycloak may be involved as there is still some perception that quarkus-oidc is for Keycloak users only
There was a problem hiding this comment.
Done
@jedla97 In general, we'd like to avoid highlighting Keycloak may be involved as there is still some perception that quarkus-oidc is for Keycloak users only
Thanks I didn't know that
jedla97
force-pushed
the
oicd-docs-small-fixes
branch
from
36c2857
to
b655d52
Compare
This comment has been minimized.
This comment has been minimized.
Status for workflow
|
There was a problem hiding this comment.
Thanks @jedla97 for trying to make security docs perfect :-)
Updating and fixing some small things like misspell, missing imports, not working code out of the box and others.