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

[fixes #4480] - Initial Code Flow Support #4884

Merged
merged 1 commit into from
Oct 29, 2019

Conversation

pedroigor
Copy link
Contributor

@pedroigor pedroigor commented Oct 25, 2019

@pedroigor
Copy link
Contributor Author

@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 quarkus.oidc.client-type to web-app so that the code flow (forcing a redirect to a OpenID Connect Provider) is configured to authenticate users.

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 quarkus.oidc.default-scopes property that can be set with a fixed list of scopes.

@pedroigor
Copy link
Contributor Author

pedroigor commented Oct 26, 2019

Another thing that I'm missing is how to use different application.properties in different tests when running integration tests. I did not find any example, probably missing something.

I've disabled the code flow tests but they work fine if you set the quarkus.oidc.client-type=web-app.

I also need to provide tests for public and confidential web apps using code flow. So, distinct configuration files ...

@sberyozkin
Copy link
Member

@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).
One comment is about some minor cosmetic change. I think we should still go with ApplicationType instead of ClientType to stay in line with what we've agreed with Stian, as my understanding we would not like the users start wondering about the OIDC client type. ApplicationType should be all right because the users have a task of protecting their applications.

The other minor one is about the default scopes - as discussed at the issue itself, lets create a config group (code or redirect ? etc) and start with a property like String scopes with some default values; it will not conflict in anyway with what yourself and Stian proposed around the beans for populating the redirect properties.

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 TokenCredential or injected as JsonWebToken is actually an IdToken while with the bearer flow - AccessToken - but the user code need to have a guarantee that it gets the access token as expected when it is packaged as a service app or as a web-app app, using the same code.

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 JsonWebToken a yet to be introduced @IdToken qualifier needs to be checked, and 2) as far as its availability from SecurityIdentity is concerned we probably need to do something basic like add a getTokenType method to TokenCredential or may be even better have AccessTokenCredential and IdTokenCredential extending TokenCredential.
I can try to help around this last item next week, it should not be complex to do and we'll have few days to tune it for the release which follows 0.27.0

Thanks

@gsmet
Copy link
Member

gsmet commented Oct 27, 2019

@pedroigor could you rebase? There is a conflict. Thanks.

@gsmet gsmet added the triage/needs-rebase This PR needs to be rebased first because it has merge conflicts label Oct 27, 2019
@sberyozkin
Copy link
Member

@pedroigor Hi Pedro, we should probably also make sure that the keycloak-authorization does not start doing something when the mode is web-app

*/
@QuarkusTestResource(KeycloakTestResource.class)
@SubstrateTest
@Disabled("While figuring out how to have different application.properties for different tests")
Copy link
Member

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

Copy link
Member

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.

Copy link
Contributor Author

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 ?

Copy link
Contributor Author

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.

@pedroigor
Copy link
Contributor Author

@sberyozkin It should be fine to use keycloak-authorization with web-app. As long as the token can be obtained at runtime, it should be fine.

@pedroigor pedroigor force-pushed the issue-4480 branch 4 times, most recently from f41977b to 9bf214d Compare October 28, 2019 13:32
@pedroigor
Copy link
Contributor Author

@sberyozkin As requested, now we are using ApplicationType.

Regarding the scopes. Initially, I did try to use a config group. The name I used was quarkus.oidc.authentication.scopes. Later I realized that I would be introducing a new group where OIDC is all about authentication, so only default-scopes looked better for me. I'm OK to introducing this new config group, so, do you like the name I mentioned? Or something else?

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 prompt=none + id_token_hint when sending an authorization request.

Wdyt ?

@pedroigor
Copy link
Contributor Author

@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 @IDToken qualifier to inject the ID Token. Do you think we should also make the refresh token available?

Still, in regards to the RT, shall we also support automatically refresh tokens ?

@sberyozkin
Copy link
Member

sberyozkin commented Oct 28, 2019

@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

@pedroigor
Copy link
Contributor Author

@sberyozkin Now the initial set of changes provides:

  • Injection of JsonWebToken representing the access_token
  • Injection of JsonWebToken representing the id_token through the @IdToken qualifier
  • Injection of RefreshToken representing the refresh_token. Only available when application is web-app. In the future, we can add more methods to help users to refresh tokens
  • Added the quarkus.oidc.authentication.scopes for sending custom scopes when performing authorization requests

@sberyozkin
Copy link
Member

@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.

@stuartwdouglas stuartwdouglas added triage/waiting-for-ci Ready to merge when CI successfully finishes and removed triage/needs-rebase This PR needs to be rebased first because it has merge conflicts labels Oct 28, 2019
@stuartwdouglas stuartwdouglas force-pushed the issue-4480 branch 2 times, most recently from f310f83 to 6cc9c98 Compare October 29, 2019 02:50
@stuartwdouglas stuartwdouglas merged commit fe06e75 into quarkusio:master Oct 29, 2019
@stuartwdouglas stuartwdouglas added this to the 0.27.0 milestone Oct 29, 2019
@pedroigor pedroigor deleted the issue-4480 branch October 29, 2019 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage/waiting-for-ci Ready to merge when CI successfully finishes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants