Skip to content

9229 - enable OIDC bearer token API access #9230

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.

Sign up for GitHub

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

Jump to bottom
Closed
poikilotherm wants to merge 13 commits into from
Closed

9229 - enable OIDC bearer token API access #9230

poikilotherm wants to merge 13 commits into from

Conversation

@poikilotherm
Copy link
Contributor

@poikilotherm poikilotherm commented Dec 14, 2022 •
edited by pdurbin
Loading

What this PR does / why we need it:

We want to enable OIDC bearer token based access to the API for existing OIDC users.

TODOs:

  • Retrieve provider metadata from existing provider
  • Create feature flag mechanism (should be reusable for other stuff, too) and make it configurable via JvmSettings
  • Add docs
  • Probably some more...

Which issue(s) this PR closes:

Closes #9229

Special notes for your reviewer:
None yet.

Suggestions on how to test this:
None yet.

Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Nope.

Is there a release notes update needed for this change?:
Yes.

Additional documentation:
None yet.

@poikilotherm poikilotherm self-assigned this Dec 14, 2022
@poikilotherm poikilotherm changed the title feat(api): first draft to enable OIDC bearer token API access #9229 9229 - enable OIDC bearer token API access Dec 14, 2022
@coveralls
Copy link

coveralls commented Dec 14, 2022 •
edited
Loading

Coverage Status

Coverage decreased (-0.007%) to 19.988% when pulling 34a1117 on poikilotherm:9229-api-oidc-access into 6c87b39 on IQSS:develop.

poikilotherm and others added 11 commits December 14, 2022 19:48
@poikilotherm
feat(settings): add feature gates functionality for the first time IQ…
f5862e3 
…SS#9229 IQSS#7000
@poikilotherm
feat(api): add OIDC API access feature gate to authentication IQSS#9229
04b7eac
@poikilotherm
chore(api): remove unused imports from AbstractApiBean IQSS#9229
3f900e5
@poikilotherm
refactor(settings,api): rename feature gates to flags as requested by @…
5e85143 
…pdurbin IQSS#9229
@poikilotherm
doc(dev,auth): add initial feature flags documentation to the guides I…
b11e4f1 
…QSS#9229 IQSS#9223
@poikilotherm
refactor(auth): cleanup OIDCAuthProvider + expose UserInfo URI IQSS#9229
dd4310b 


- Expose the UserInfo endpoint URI from the OIDC provider metadata
- Not exposing the complete metadata on purpose to keep it
  non-modifiable and sealed inside the provider instance
- Minor method renaming to better explain what the method is about
- Fix missing newlines for single line method
@poikilotherm
feat(api): query all avail OIDC providers for user info IQSS#9229
3694d81 
Replace the placeholder of static endpoint with retrieving the UserInfo
endpoint URI from all know OIDC authentication providers and iterate
over them. The access token might match for any of them.

Also making the errors a bit more descriptive and adding logging.
@vera
fix(api): don't default to guest user if OIDC bearer token is present
b1a7b85
@vera
fix(api): don't strip OIDC auth scheme from Authorization header
065630f 
Otherwise, BearerAccessToken.parse throws a ParseException since it expects the full HTTP Authorization header value.
@vera
fix(api): send appropriate error response when OIDC user is unknown
6561bac
@vera
fix(api): allow Authorization header in incoming API requests
62ce097
@vera
Copy link
Contributor

vera commented Dec 15, 2022

I pushed a few commits fixing issues I ran into while testing here: https://github.com/vera/dataverse/commits/9229-api-oidc-access

I ran into one more issue which I am not sure how to fix:

dataverse/src/main/java/edu/harvard/iq/dataverse/api/AbstractApiBean.java

Line 453 in 3694d81

AuthenticatedUser authUser = authSvc.getAuthenticatedUser(userInfo.getSubject().getValue());

This line does not succeed in finding the correct user. If I understand correctly, it tries to look up an authenticateduser with a userIdentifier equal to the subject of the OIDC user info (for me, this is some hex string like xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx). But the userIdentifier of the user which should be found isn't that. It's the username I entered when I registered the user via the Dataverse UI. Thus, the lookup fails.

Is the OIDC user ID stored anywhere during user registration?

@poikilotherm
Merge pull request #599 from vera/9229-api-oidc-access
34a1117 
Some fixes for 9229 - enable OIDC bearer token API access
qqmyers
qqmyers reviewed Jan 4, 2023
View reviewed changes
src/main/java/edu/harvard/iq/dataverse/api/AbstractApiBean.java
// This will need to be modified to provide mappings somehow for existing non-OIDC-users.
// TODO: If we keep the current login infrastructure alive, we should introduce a common static
// method in OIDCAuthProvider to create the identifier in both places.
AuthenticatedUser authUser = authSvc.getAuthenticatedUser(userInfo.getSubject().getValue());
Copy link
Member

@qqmyers qqmyers Jan 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW: Using authSvc.getAuthenticatedUserByEmail(userInfo.getEmail().toString()); should work. This isn't checking to see if the account was created using OIDC though.

@poikilotherm poikilotherm mentioned this pull request Jan 9, 2023
Closed
@GPortas GPortas mentioned this pull request Jan 16, 2023
Merged
3 tasks
@mreekie
Copy link

mreekie commented Jan 18, 2023

sizing:

  • This is not ready for sizing yet.

@mreekie
Copy link

mreekie commented Jan 23, 2023

sizing:

  • Skipping for today.
  • @GPortas @poikilotherm update in the comments when you believe this is ready to be sized and I'll catch it in the next pass

@mreekie mreekie added Size: Queued PM has called this issue out specifically for sizing bklog: NeedsDiscussion labels Jan 24, 2023
@mreekie
Copy link

mreekie commented Jan 24, 2023

Sizing:

  • Added it to the queue by adding the size queue label.
  • Added bklog: needs discussion label to indicate that we will need to revisit this.
  • All that changes nothing in terms of what's next. We'll size this when we get notified that it's ready

@mreekie mreekie added the NIH OTA: 1.7.1 (reArchitecture) 7 | 1.7.1 | Research & architecture for separating backend and frontend to enable a flexible, sca... label Feb 28, 2023
@mreekie mreekie added the pm.GREI-d-1.7.1 NIH, yr1, aim7, task1: Research & architecture for separating backend and frontend label Mar 20, 2023
@johannes-darms johannes-darms mentioned this pull request Apr 18, 2023
Closed
@poikilotherm
Copy link
Contributor Author

poikilotherm commented May 15, 2023

Replaced by #9532. Thanks @johannes-darms

@cmbz cmbz added the FY26 Sprint 14 FY26 Sprint 14 (2025-12-31 - 2026-01-14) label Jan 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@qqmyers qqmyers qqmyers left review comments

Assignees

@poikilotherm poikilotherm

Labels

Feature: Account & User Info Feature: Admin Guide Feature: API Guide Feature: API Feature: Permissions FY26 Sprint 14 FY26 Sprint 14 (2025-12-31 - 2026-01-14) NIH OTA: 1.7.1 (reArchitecture) 7 | 1.7.1 | Research & architecture for separating backend and frontend to enable a flexible, sca... pm.GREI-d-1.7.1 NIH, yr1, aim7, task1: Research & architecture for separating backend and frontend Size: Queued PM has called this issue out specifically for sizing

Projects

IQSS Dataverse Project
Status: No status

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

As a developer, I'd like to use Bearer Token to send authenticated API requests Clarify in guides that APIs do not support OIDC

6 participants

@poikilotherm @coveralls @vera @mreekie @qqmyers @cmbz