OIDC auth: MPCONFIG provisioning, PKCE support and integration tests #9273
Conversation
These values should not be changed once the provider has been initialized.
Also updating Postgres Server version in "tc" Maven profile.
Instead of only allowing to supply static String values for a setting, also allow referencing a static method in the test class to retrieve dynamic data. This is inspired by the JUnit5 MethodSource example.
Only one provider can be configured via MPCONFIG for now. The provider is configured with an appropriate ID to distinguish it from other providers configured via the API. It can be configured in addition to other OIDC providers when desired.
…QSS#9268 Using Testcontainers to start a Keycloak instance with our default development realm, the provider is created using MPCONFIG settings.
To use data in /conf for tests, adding the folder in Maven to copy them to the test classpath as resources helps to use them in tests very easily. All dirs under /conf will be copied to the /target/test-classes directory recursively. This also works when running tests in IDEs like IntelliJ.
poikilotherm
added
Component: Code Infrastructure
|
This is probably a "10" in size, as one needs to understand Testcontainers concept, read the details about the changes test helper and then also learn what this configuration thing is doing in addition to the PKCE support. That will very likely require a full day if you're not familiar with the auth code and OAuth/OIDC concepts. |
mreekie
added
the
NIH OTA: 1.7.1 (reArchitecture)
mreekie
added
the
pm.GREI-d-1.7.1
When the client is returning from the provider to us, carrying along the authorization code we need to retrieve user details, we also receive again the state. The state was generated and sent by us, and will not be altered by the provider, which makes it perfect to identify the original request we built before sending the client to the provider. Passing this state to the provider enables the provider to reuse this information. This is crucial to enable PKCE support, as we need to remember which secret code we sent to the provider - otherwise we will not be able to verify the authz code. Tests have been adapted.
- Adding PKCE parameters to constructor - Adding a hashmap to cache the code verifiers mapped by the unique state we generate - Enabling the actual workflow of PKCE
To enable backward compatibility, default to disabled and method S256.
No it shouldn't - the experimentation there is way more than what we do in this PR: Here we "just" add some external deps to a unit test to make it an integration test, while with Arquillian we would actually put code inside a deployment, thus having an actual Jakarta container around it. |
Co-authored-by: Philip Durbin <philipdurbin@gmail.com>
There was a problem hiding this comment.
I'm making a few more tiny doc suggestions that I can live without.
I'm pretty much ready to send this to QA but we have testing failing on Jenkins/Ansible, something to do with Jacoco. We make need to enlist @donsizemore to take a look: #9273 (comment)
… more about MPCONFIG
Co-authored-by: Philip Durbin <philipdurbin@gmail.com>
As the OIDC provider does not yet provide proper support mapping from OIDC groups to Dataverse groups, remove this from the docs for now. Re-add when IQSS#9969 is (being) resolved. Co-authored-by: Philip Durbin <philipdurbin@gmail.com>
There was a problem hiding this comment.
I believe this is ready for QA. Test are passing for me locally with mvn clean verify. Docs look good.
Jenkins is failing but I have a PR in to Ansible that I'm testing at https://jenkins.dataverse.org/job/IQSS-dataverse-9273-oidc/1/
The path to the jacoco unit test file changed. Here's the PR:
poikilotherm
added
Size: 10
|
@pdurbin said this has been more than a "size 3", so I'm bumping it up to be a "10" now. |
|
@poikilotherm in the description would you be able to replace "or do manual tests" with a little more detail? |
|
Nice merge @kcondon ! How did you like the Testcontainers experience? |
|
@poikilotherm Apologies but I saw that Phil had tested it and thought this might be needed so approved it. I can circle back and take a look. |
|
One of the ideas beyond this PR was to make testing easier and cover more, so less manual testing is necessary in your precious time. But of course all of the stuff added has actually to be usable... I'd appreciate if you would take a look, but if there's no time, feel free to skip 😉 |
What this PR does / why we need it:
Provisioning an OIDC provider without wrangling API commands is much quicker for simple deployments in testing or production. MPCONFIG enables that. This is important for all SPA work that requires authentication, as it simplifies the setup during development and testing.
Also, this PR adds PKCE support for OIDC providers. This is more and more becoming a requirement and will certainly be necessary for the SPA (which is a public client).
Third, this PR includes a real integration test for all functionality around OIDC authentication. Testing if auth works is getting much easier this way.
Which issue(s) this PR closes:
Special notes for your reviewer:
Note that this PR also extends the capabilities of
@JvmSettingJUnit test helper to allow method references for data. This is necessary to lookup the URL from the Testcontainers' ruled Keycloak instance. Also updating Testcontainer to latest version here. Also enabling the annotation to be used as a class annotations for real now!Suggestions on how to test this:
mvn clean verify. Or do manual tests. 😉 (Manual tests mean run a Keycloak or other OIDC provider on your machine, target it by using the JVM options to enable the provider and try to login to Dataverse with it.)Does this PR introduce a user interface change? If mockups are available, please link/include them here:
No.
Is there a release notes update needed for this change?:
Yes, included.
Additional documentation:
Included: https://dataverse-guide--9273.org.readthedocs.build/en/9273/installation/oidc.html