OpenID Connect (OIDC) support #6433
Conversation
…TODO about using real JSON
…static for reuse in OIDC factory
…provider configuration. Before this refactoring, a static list of providers has been used, no matter if they where enabled or not. This is not acceptable for the new OIDC provider, as we might have multiple of 'em and it retrieves metadata on creation. So the idea was to not create some static list of IDs (which had been used to look up the real provider from the "registry"), but instead get the registred providers, sort them and retrieve the display info. Sorting is done by a numerical value first, then by id attribute (which is set by factories), so we have a deterministic, unchanging order for the user. This lead to the creation of a new method AuthenticationProvider.getOrder(), defaulting to 1 and overridden by implementations. This opens up the possibility to change ordering by configuration (as suggested in an old TODO comment of the refactored code)
|
|
||
| for (AuthenticationProvider idp : idps) { | ||
| if (idp != null) { | ||
| infos.add(idp.getInfo()); |
There was a problem hiding this comment.
Is the change above necessary for OIDC to work? I don't think so. It does look like a nice cleanup though. I like how it's deterministic. Perhaps this change should be documented.
There was a problem hiding this comment.
Actually yes it is. From this function, authSvc.getAuthenticationProviderIdsSorted() was called, which is deleted below. This was done in 3fd326c and here's what I wrote in the commit log:
Refactor LoginPage.listAuthenticationProviders() to represent actual provider configuration.
Before this refactoring, a static list of providers has been used, no matter if they where
enabled or not. This is not acceptable for the new OIDC provider, as we might have multiple
of 'em and it retrieves metadata on creation.So the idea was to not create some static list of IDs (which had been used to look up the real
provider from the "registry"), but instead get the registred providers, sort them and
retrieve the display info.Sorting is done by a numerical value first, then by id attribute (which is set by factories),
so we have a deterministic, unchanging order for the user.This lead to the creation of a new method AuthenticationProvider.getOrder(), defaulting to 1
and overridden by implementations. This opens up the possibility to change ordering by
configuration (as suggested in an old TODO comment of the refactored code)
| * @return an integer value (sort ascending) | ||
| */ | ||
| @Override | ||
| public int getOrder() { return 100; } |
There was a problem hiding this comment.
What if there are more than 100 authentication providers? Some have the same number? Is it ok to have two 42s?
There was a problem hiding this comment.
Actually it doesn't matter if some have the same number, as they are sorted by number first, then by name.
This is mostly about sorting Builtin first (order = 1), Shib second (order=20) and OAuth last (all order = 100), as it has been sorted before my refactoring.
Once we have a configurable value for this, you could also realize sth. like provider A first, then all the others.
Please see also my comment above, containing the commit log message explaining details.
| package edu.harvard.iq.dataverse.authorization.providers.oauth2.oidc; | ||
|
|
||
| import com.github.scribejava.core.builder.api.DefaultApi20; | ||
| import com.nimbusds.oauth2.sdk.*; |
There was a problem hiding this comment.
In general, we try not to use star imports. I regret to say that we don't mention this at http://guides.dataverse.org/en/4.18.1/developers/coding-style.html 😄
There was a problem hiding this comment.
I'm sorry, IntelliJ IDEA does this by default.
Should I mention checkstyle again to catch this sort of stuff?
We could create a Jenkins job for it...
There was a problem hiding this comment.
Oh and should I change this in a new PR as this one is already merged?
@poikilotherm can you also provide an OIDC environment for testing the actual feature this pull request introduces? |
|
I can report great success for using Google OpenID Connect support. Worked flawless, does what it should do. I love standards... 😄 Should we talk to @alejandratenorio and @Gerafp for testing it with Microsoft Azure AD? |
What this PR does / why we need it:
This pull request adds very basic support for authentication with an OpenID Connect capable identity provider.
We need this is as a basis to solve #5974, which is about using a more sophisticated user authentication system for Dataverse.
Which issue(s) this PR closes:
Special notes for your reviewer:
This has been tested so far with an internal IdP based on Unity IDM.
I think about introducing integration testing this feature by using Testcontainers, but this seems like beyond scope for this PR and keeping that narrow.
Suggestions on how to test this:
We should test this with Google, maybe with ORCID.
Does this PR introduce a user-facing change?:
Kind of. It reuses all the existing OAuth2 UI stuff, but of course enables new buttons for providers. It might become an issue in case someone really wants to add a big bunch of stuff, as the list grows in that case.
It might affects the order of auth providers, too, as the sorting feature for the UI needed to be refactored.
Is there a release notes update needed for this change?
Sure. But maybe this should be a separate PR, as there are some more things still to come.
Additional documentation:
This PR is a demo for #6226 on @pdurbins demand on IRC