-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
[fixes #4480] - Initial Code Flow Support #4884
Conversation
@sberyozkin @stuartwdouglas, This is the initial code flow to get code flow supported. By using this code, is now possible to protected two main types of applications: web apps and service apps. The type of the application being protected influences the flow we'll use to authenticate/authorize requests. By default, we enable bearer token authorization. For web applications, you can just set the property Regarding logout, the current changeset supports logout based on the token lifetime. I've also been working on another method that should enhance logout so that users are also logged out when their sessions are invalidated at the OpenID Connect Provider. I did not include here because I had to change Keycloak to better adhere to the specs and support silent re-authentication. Once we have that, I can grab changes from here. Another thing that is out of this initial changeset is the necessary code to resolve authorization request scopes during runtime. What I have now is a |
Another thing that I'm missing is how to use different I've disabled the code flow tests but they work fine if you set the I also need to provide tests for public and confidential web apps using code flow. So, distinct configuration files ... |
@pedroigor Hi Pedro, it looks very good IMHO, structurally wise, the separation between the authentication mechanisms, initial logout support (I'll add a link to a more advanced logout support at your branch to the sep logout issue). The other minor one is about the default scopes - as discussed at the issue itself, lets create a config group ( Finally, we discussed a bit earlier, right now we have the inconsistency in that if it is the code flow then what the user code will get from Right now the code flow does not even provide an access token - not a blocker for a PR (we will follow up to support a case of the web-app calling out with the ATs) but as far as the availability of IdToken is concerned, I believe we need to make it correct in this PR: 1) to support its injection as Thanks |
@pedroigor could you rebase? There is a conflict. Thanks. |
@pedroigor Hi Pedro, we should probably also make sure that the |
*/ | ||
@QuarkusTestResource(KeycloakTestResource.class) | ||
@SubstrateTest | ||
@Disabled("While figuring out how to have different application.properties for different tests") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only possible for native tests at the moment by creating new maven modules
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it looks like this only really needs to set runtime config? If so it may be possible to use multiple maven profiles, and tell surefire/failsafe to use a different profile for each run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try what you suggest in regard to using profiles. But, wouldn't be more simple if we could annotate a test or reuse some test annotation to specify the properties we want to load ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stuartwdouglas I've added a new module for code flow tests.
@sberyozkin It should be fine to use keycloak-authorization with |
f41977b
to
9bf214d
Compare
@sberyozkin As requested, now we are using Regarding the scopes. Initially, I did try to use a config group. The name I used was In regards to access and ID tokens available to applications. As you know, currently we are storing the IDToken in the cookie. With this token, we are able to silently re-authenticate the user as well as obtain information about it. Standard-wise, the only way to check whether or not the user still has a valid session is using Wdyt ? |
@sberyozkin in regards to the access token, I thought that we would be addressing the authentication bits for now and do not deal with access or refresh tokens. I can change that though, so we would be adding both AT and RT to the cookie. So, we would inject by default the AT and use the Still, in regards to the RT, shall we also support automatically refresh tokens ? |
@pedroigor thanks for the initial changes and for the discussion, yes, all I wanted was to ensure the users have a specific way to get to either of the token types to avoid any user confusions in the future. The PR providing only IdToken for now is perfect, we'll deal with AT (+RT) in the next phase. Thanks |
@sberyozkin Now the initial set of changes provides:
|
@pedroigor Thanks a million - this looks super good IMHO. Now the adapter will scale to all the cases, very nice, thanks for the super fast update. |
f310f83
to
6cc9c98
Compare
6cc9c98
to
4b2c71b
Compare
Quickstart: quarkusio/quarkus-quickstarts#337