Assorted OIDC changes #4811
Assorted OIDC changes #4811
Conversation
And we need it to be overridable.
| @@ -7,7 +7,7 @@ | |||
| import io.quarkus.runtime.annotations.ConfigPhase; | |||
| import io.quarkus.runtime.annotations.ConfigRoot; | |||
|
|
|||
| @ConfigRoot(phase = ConfigPhase.BUILD_AND_RUN_TIME_FIXED) | |||
| @ConfigRoot(phase = ConfigPhase.RUN_TIME) | |||
There was a problem hiding this comment.
Why is this configuration build time? It's only used at the runtime init phase AFAICS and it's very useful to be able to override it at runtime.
There was a problem hiding this comment.
BTW, I looked at this code because I had a user asking how to override the realm at runtime so it's not a rhetorical question.
| public VertxOAuth2AuthenticationMechanism setConfig(OidcConfig config) { | ||
| this.config = config; | ||
| return this; | ||
| } |
There was a problem hiding this comment.
I don't think we should expose this and AFAICS it's not used. @michalszynkiewicz it's you who introduced that, can you confirm it's not used?
There was a problem hiding this comment.
Neither my memory nor git blame thinks I introduced that but I don't see getConfig used anywhere and the field is not used either.
There was a problem hiding this comment.
Oh sorry, my Eclipse history made me think you added this when working on the security interceptors :).
Yeah, I think it's safe to remove it. Will wait for @stuartwdouglas .
| @@ -1,4 +1,4 @@ | |||
| package io.quarkus.oidc; | |||
| package io.quarkus.oidc.runtime; | |||
There was a problem hiding this comment.
Is this class supposed to be exposed to the users? It looks like an internal one but I would like to check.
| @@ -1,4 +1,4 @@ | |||
| package io.quarkus.oidc; | |||
| package io.quarkus.oidc.runtime; | |||
There was a problem hiding this comment.
Is this class supposed to be exposed to the user or not? E.g. is there any interest in him injecting this particular bean?
Maybe they could need an access to getAuth()? In this case, we need to move it back to io.quarkus.oidc.
| @@ -8,6 +8,7 @@ | |||
| import org.jose4j.jwt.JwtClaims; | |||
| import org.jose4j.jwt.consumer.InvalidJwtException; | |||
|
|
|||
| import io.quarkus.oidc.VertxJwtCallerPrincipal; | |||
There was a problem hiding this comment.
I'm not convinced by the name of this class. It's exposed to the user AFAICS, could we find a better name? I don't see why we have Vertx in it typically. It looks historical to me. OidcJwtCallerPrincipal maybe?
I will do the changes. Just asking for advice.
|
I'm definitely no expert at this area but the changes look good to me |
|
@gsmet Hi Guillaume, it all looks fine. |
|
Though restricting the visibility by moving into |
|
OK, looks like I wasted my time, apart for the doc fix and the move to runtime config. @pedroigor any chance you could submit a PR with your changes. They are 12 days old AFAICS and they would be better in the Quarkus tree than in your own repo :). |
|
@sberyozkin I removed the renaming part as @pedroigor 's work should rather be included. Can you check with him if he can rebase once this is in and submit a PR? Thanks! |
|
Hi @gsmet. We've talked with @pedroigor a bit earlier, the PR needs a bit more work (as the new flow proposed has to be distinguished from the default one where the user is not redirected and few other minor tweaks - I've actually started working on it on one of branches). We'll try to complete with Pedro next once the policy-enforcer PR is closed |
Let's see what CI has to say and I added a few comments here and there to be sure I don't do anything wrong.
@pedroigor @sberyozkin @michalszynkiewicz can you check my comments inline and answer my questions? Thanks!