-
Notifications
You must be signed in to change notification settings - Fork 486
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
API OIDC authentication mechanism #10905
base: develop
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Thanks! Co-authored-by: Philip Durbin <[email protected]>
I forgot to mention this: the log-in is a one time thing. The normal flow is to go to http://localhost:8080/api/v1/callback/session and only if you are not authenticated (I implemented returning better error message now), you go to http://localhost:8080/oidc/login. @pdurbin thanks for the docker run command, I will try testing it with that i.s.o. my own dev environment. |
This comment has been minimized.
This comment has been minimized.
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.
Some initial feedback. I haven't done any real testing yet.
docker-compose-dev.yml
Outdated
@@ -16,13 +16,12 @@ services: | |||
ENABLE_RELOAD: "1" |
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'm just putting this here at the top but can we please get a release note snippet?
src/main/java/edu/harvard/iq/dataverse/settings/JvmSettings.java
Outdated
Show resolved
Hide resolved
src/main/java/edu/harvard/iq/dataverse/api/OpenIDAuthentication.java
Outdated
Show resolved
Hide resolved
@Inject | ||
protected AuthenticationServiceBean authSvc; | ||
|
||
@Path("token") |
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.
Docs please! (API Guide.)
return Response.seeOther(crc.getUriInfo().getBaseUri().resolve("callback/session")).build(); | ||
} | ||
|
||
@Path("session") |
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.
Docs here too, please.
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
Another pass.
conf/keycloak/test-realm.json
Outdated
"email" : "dataverse-admin@mailinator.com", | ||
"email" : "[email protected]", |
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.
Can you please remind me why this change is necessary?
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.
We talked about matching users from OIDC to Dataverse users multiple times at LIBIS. We feel like matching users by verified emails would be the best approach. The user with "[email protected]" does not exist in the dev env, so it did not work. Matching by email would mean that you can log in with google, facebook, whatever, as long as you use the same email.
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 reverted it and implemented the regular UserRecordIdentifier lookup, however, user is not known:
I have also implemented the redirect to the first login page if user is not known:
Is it as it supposed to be? I have also noticed that oauth tokens are stored in the DB. The side effect of creating a new user after OIDC authentication is that it inserts one token there...
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 think I can just pass null as token data to prevent storing it in DB. I am not sure why we store them, I assume it is like the cache for bearer tokens? Something to investigate a little bit more.
src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OIDCLoginBackingBean.java
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
I had now reimplemented the entire OIDC code with the new mechanism. In the process I had to drop the PKCE support, which makes sense i.m.o. since the entire OIDC is now handled by the Payara built-in feature and the frontends do not need to implement it anymore. Another unexpected outcome is that I had to reimplement the bearer token support. I feel like dropping it would make this PR even harder to accept. The final implementation is not extremely bad, it reuses the cache that was first meant for the PKCE support. However, it makes the bearer tokens being stored twice in the system: once by the sessions, and once by the cache for the bearer support. I still believe that not storing the cookies, but generating one with each request is not much different from using the bearer tokens (I would leave the bearer token support feature off). For example, when using curl and not storing the cookie, the requests look like this: curl -v --cookie "JSESSIONID=short-session-id" http://localhost:8080/api/v1/users/:me | jq . vs curl -H "Authorization: Bearer very-long-bearer-token-encoded-in-base64" http://localhost:8080/api/v1/users/:me | jq . I still need to reimplement the "first login" feature for the unknown users (when email address is not found in the DB) and debug the code. Nevertheless, the main ideas are already in the code. |
This comment has been minimized.
This comment has been minimized.
📦 Pushed preview images as
🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name. |
What this PR does / why we need it:
It implements OIDC authentication mechanism for API usage/SPA
Which issue(s) this PR closes:
Closes #
Special notes for your reviewer:
This is very similar conceptually as the oauth2-proxy, as in IQSS/dataverse-frontend#504, however, it turns out that the needed functionality is already supported in Payara: https://docs.payara.fish/enterprise/docs/Technical%20Documentation/Public%20API/OpenID%20Connect%20Support.html
This PR eliminates any need for a proxy, etc. It turned out that the implementation is very simple and elegant i.m.h.o.
Suggestions on how to test this:
I have tried to configure the default dev docker-compose to work with it, however, it uses the unstable image, not the one built locally. I already have my own dev environment, so I did test it on mine. The configuration should be almost identical to the committed one (except maybe for the test user; I tried using the "nick" built-in user here).
How I did test it:
after clicking sign in it redirects you again and it shows you this JSON response:
For example:
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
Additional documentation:
https://docs.payara.fish/enterprise/docs/Technical%20Documentation/Public%20API/OpenID%20Connect%20Support.html