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

9229 - Add bearer token auth mechanism for OIDC user with feature flag #9591

Merged
merged 7 commits into from
May 18, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion conf/keycloak/oidc-keycloak-auth-provider.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,6 @@
"factoryAlias": "oidc",
"title": "OIDC-Keycloak",
"subtitle": "OIDC-Keycloak",
"factoryData": "type: oidc | issuer: http://localhost:8090/auth/realms/oidc-realm | clientId: oidc-client | clientSecret: ss6gE8mODCDfqesQaSG3gwUwZqZt547E",
"factoryData": "type: oidc | issuer: http://keycloak.mydomain.com:8090/realms/oidc-realm | clientId: oidc-client | clientSecret: ss6gE8mODCDfqesQaSG3gwUwZqZt547E",
"enabled": true
}
1 change: 1 addition & 0 deletions doc/release-notes/9229-bearer-api-auth.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
A feature flag called "api-bearer-auth" has been added. This allows OIDC useraccounts to send authenticated API requests using Bearer Tokens. Note: This feature is limited to OIDC! For more information, see http://preview.guides.gdcc.io/en/develop/installation/config.html#feature-flags
17 changes: 17 additions & 0 deletions doc/sphinx-guides/source/api/auth.rst
Original file line number Diff line number Diff line change
Expand Up @@ -63,3 +63,20 @@ Resetting Your API Token
------------------------

You can reset your API Token from your account page in your Dataverse installation as described in the :doc:`/user/account` section of the User Guide.

.. _bearer-tokens:

Bearer Tokens
-------------

Bearer tokens are defined in `RFC 6750`_ and can be used as an alternative to API tokens if your installation has been set up to use them (see :ref:`bearer-token-auth` in the Installation Guide).

.. _RFC 6750: https://tools.ietf.org/html/rfc6750

To test if bearer tokens are working, you can try something like the following (using the :ref:`User Information` API endpoint), substituting in parameters for your installation and user.

.. code-block:: bash

export TOKEN=`curl -s -X POST --location "http://keycloak.mydomain.com:8090/realms/oidc-realm/protocol/openid-connect/token" -H "Content-Type: application/x-www-form-urlencoded" -d "username=kcuser&password=kcpassword&grant_type=password&client_id=oidc-client&client_secret=ss6gE8mODCDfqesQaSG3gwUwZqZt547E" | jq '.access_token' -r | tr -d "\n"`

curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/users/:me
4 changes: 4 additions & 0 deletions doc/sphinx-guides/source/developers/remote-users.rst
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,13 @@ Now when you go to http://localhost:8080/oauth2/firstLogin.xhtml you should be p

----

.. _oidc-dev:

OpenID Connect (OIDC)
---------------------

STOP! ``oidc-keycloak-auth-provider.json`` was changed from http://localhost:8090 to http://keycloak.mydomain.com:8090 to test :ref:`bearer-tokens`. In addition, ``docker-compose-dev.yml`` in the root of the repo was updated to start up Keycloak. To use these, you should add ``127.0.0.1 keycloak.mydomain.com`` to your ``/etc/hosts file``. If you'd like to use the docker compose as described below (``conf/keycloak/docker-compose.yml``), you should revert the change to ``oidc-keycloak-auth-provider.json``.

If you are working on the OpenID Connect (OIDC) user authentication flow, you do not need to connect to a remote provider (as explained in :doc:`/installation/oidc`) to test this feature. Instead, you can use the available configuration that allows you to run a test Keycloak OIDC identity management service locally through a Docker container.

(Please note! The client secret (``ss6gE8mODCDfqesQaSG3gwUwZqZt547E``) is hard-coded in ``oidc-realm.json`` and ``oidc-keycloak-auth-provider.json``. Do not use this config in production! This is only for developers.)
Expand Down
20 changes: 18 additions & 2 deletions doc/sphinx-guides/source/installation/config.rst
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,19 @@ As for the "Remote only" authentication mode, it means that:
- ``:DefaultAuthProvider`` has been set to use the desired authentication provider
- The "builtin" authentication provider has been disabled (:ref:`api-toggle-auth-provider`). Note that disabling the "builtin" authentication provider means that the API endpoint for converting an account from a remote auth provider will not work. Converting directly from one remote authentication provider to another (i.e. from GitHub to Google) is not supported. Conversion from remote is always to "builtin". Then the user initiates a conversion from "builtin" to remote. Note that longer term, the plan is to permit multiple login options to the same Dataverse installation account per https://github.com/IQSS/dataverse/issues/3487 (so all this talk of conversion will be moot) but for now users can only use a single login option, as explained in the :doc:`/user/account` section of the User Guide. In short, "remote only" might work for you if you only plan to use a single remote authentication provider such that no conversion between remote authentication providers will be necessary.

.. _bearer-token-auth:

Bearer Token Authentication
---------------------------

Bearer tokens are defined in `RFC 6750`_ and can be used as an alternative to API tokens. This is an experimental feature hidden behind a feature flag.

.. _RFC 6750: https://tools.ietf.org/html/rfc6750

To enable bearer tokens, you must install and configure Keycloak (for now, see :ref:`oidc-dev` in the Developer Guide) and enable ``api-bearer-auth`` under :ref:`feature-flags`.

You can test that bearer tokens are working by following the example under :ref:`bearer-tokens` in the API Guide.

Comment on lines +333 to +345
Copy link
Contributor

Choose a reason for hiding this comment

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

I do understand this is explained in the API docs (perfect place), this repetition and elaboration here seems somewhat misplaced IMHO. Shouldn't we keep this in the API guide?

.. _database-persistence:

Database Persistence
Expand Down Expand Up @@ -2391,6 +2404,9 @@ please find all known feature flags below. Any of these flags can be activated u
* - api-session-auth
- Enables API authentication via session cookie (JSESSIONID). **Caution: Enabling this feature flag exposes the installation to CSRF risks!** We expect this feature flag to be temporary (only used by frontend developers, see `#9063 <https://github.com/IQSS/dataverse/issues/9063>`_) and removed once support for bearer tokens has been implemented (see `#9229 <https://github.com/IQSS/dataverse/issues/9229>`_).
- ``Off``
* - api-bearer-auth
- Enables API authentication via Bearer Token for OIDC User Accounts. **Information: This feature works only for OIDC UserAccounts!**
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Enables API authentication via Bearer Token for OIDC User Accounts. **Information: This feature works only for OIDC UserAccounts!**
- Enables API authentication via Bearer Token for OIDC User Accounts. **Note: This feature works only when using an :doc:`OpenID Connect authentication provider <oidc>`!**

- ``Off``

**Note:** Feature flags can be set via any `supported MicroProfile Config API source`_, e.g. the environment variable
``DATAVERSE_FEATURE_XXX`` (e.g. ``DATAVERSE_FEATURE_API_SESSION_AUTH=1``). These environment variables can be set in your shell before starting Payara. If you are using :doc:`Docker for development </container/dev-usage>`, you can set them in the `docker compose <https://docs.docker.com/compose/environment-variables/set-environment-variables/>`_ file.
Expand Down Expand Up @@ -3837,7 +3853,7 @@ To use the current GDCC version directly:
:CategoryOrder
++++++++++++++

A comma separated list of Category/Tag names defining the order in which files with those tags should be displayed.
A comma separated list of Category/Tag names defining the order in which files with those tags should be displayed.
The setting can include custom tag names along with the pre-defined tags(Documentation, Data, and Code are the defaults but the :ref:`:FileCategories` setting can be used to use a different set of tags).
The default is category ordering disabled.

Expand All @@ -3849,7 +3865,7 @@ A true(default)/false option determining whether datafiles listed on the dataset
:AllowUserManagementOfOrder
+++++++++++++++++++++++++++

A true/false (default) option determining whether the dataset datafile table display includes checkboxes enabling users to turn folder ordering and/or category ordering (if an order is defined by :CategoryOrder) on and off dynamically.
A true/false (default) option determining whether the dataset datafile table display includes checkboxes enabling users to turn folder ordering and/or category ordering (if an order is defined by :CategoryOrder) on and off dynamically.

.. _supported MicroProfile Config API source: https://docs.payara.fish/community/docs/Technical%20Documentation/MicroProfile/Config/Overview.html

20 changes: 20 additions & 0 deletions docker-compose-dev.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ services:
- DATAVERSE_DB_HOST=postgres
- DATAVERSE_DB_PASSWORD=secret
- DATAVERSE_DB_USER=${DATAVERSE_DB_USER}
- DATAVERSE_FEATURE_API_BEARER_AUTH=1
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather not have this feature on by default. I'd rather keep this file less feature enabled, but more bare-bones.

ports:
- "8080:8080" # HTTP (Dataverse Application)
- "4848:4848" # HTTP (Payara Admin Console)
Expand Down Expand Up @@ -98,6 +99,25 @@ services:
tmpfs:
- /mail:mode=770,size=128M,uid=1000,gid=1000

dev_keycloak:
container_name: "dev_keycloack"
image: 'quay.io/keycloak/keycloak:19.0'
hostname: keycloak
environment:
- KEYCLOAK_ADMIN=kcadmin
- KEYCLOAK_ADMIN_PASSWORD=kcpassword
- KEYCLOAK_LOGLEVEL=DEBUG
- KC_HOSTNAME_STRICT=false
networks:
dataverse:
aliases:
- keycloak.mydomain.com #create a DNS alias within the network (add the same alias to your /etc/hosts to get a working OIDC flow)
command: start-dev --import-realm --http-port=8090 # change port to 8090, so within the network and external the same port is used
ports:
- "8090:8090"
volumes:
- './conf/keycloak/oidc-realm.json:/opt/keycloak/data/import/oidc-realm.json'

Comment on lines +102 to +120
Copy link
Contributor

Choose a reason for hiding this comment

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

Please note:

  1. Keycloak 19 is outdated.
  2. I dislike adding more containers than necessary here. This is an experimental feature and right now, for most of development this is waste of CPU cycles. Can we make this scale: $ENABLE_KEYCLOAK, have ENABLE_KEYCLOAK=0 in .env so you need to enable this by adding ENABLE_KEYCLOAK=1 when starting this?

networks:
dataverse:
driver: bridge
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
package edu.harvard.iq.dataverse.api.auth;

import com.nimbusds.oauth2.sdk.ParseException;
import com.nimbusds.oauth2.sdk.token.BearerAccessToken;
import edu.harvard.iq.dataverse.UserServiceBean;
import edu.harvard.iq.dataverse.authorization.AuthenticationServiceBean;
import edu.harvard.iq.dataverse.authorization.UserRecordIdentifier;
import edu.harvard.iq.dataverse.authorization.providers.oauth2.OAuth2Exception;
import edu.harvard.iq.dataverse.authorization.providers.oauth2.oidc.OIDCAuthProvider;
import edu.harvard.iq.dataverse.authorization.users.AuthenticatedUser;
import edu.harvard.iq.dataverse.authorization.users.User;
import edu.harvard.iq.dataverse.settings.FeatureFlags;

import javax.inject.Inject;
import javax.ws.rs.container.ContainerRequestContext;
import javax.ws.rs.core.HttpHeaders;
import java.io.IOException;
import java.util.List;
import java.util.Optional;
import java.util.logging.Level;
import java.util.logging.Logger;
import java.util.stream.Collectors;

public class BearerTokenAuthMechanism implements AuthMechanism {
private static final String BEARER_AUTH_SCHEME = "Bearer";
public static final String UNAUTHORIZED_BEARER_TOKEN = "Unauthorized bearer token";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public static final String UNAUTHORIZED_BEARER_TOKEN = "Unauthorized bearer token";
public static final String UNAUTHORIZED_BEARER_TOKEN = "Unauthorized bearer token";

public static final String INVALID_BEARER_TOKEN = "Could not parse bearer token";
public static final String BEARER_TOKEN_DETECTED_NO_OIDC_PROVIDER_CONFIGURED = "Bearer token detected, no OIDC provider configured";

@Inject
protected AuthenticationServiceBean authSvc;
@Inject
protected UserServiceBean userSvc;
private static final Logger logger = Logger.getLogger(BearerTokenAuthMechanism.class.getCanonicalName());
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be moved to other private static final member above.

@Override
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@Override
@Override

public User findUserFromRequest(ContainerRequestContext containerRequestContext) throws WrappedAuthErrorResponse {
if (FeatureFlags.API_BEARER_AUTH.enabled()) {
Optional<String> bearerToken = getRequestApiKey(containerRequestContext);
// No Bearer Token present, hence no user can be authenticated
if (!bearerToken.isPresent()) {
return null;
}
//validate and verify provided Bearer Token, and retrieve UserRecordIdentifier
UserRecordIdentifier userInfo = verifyOidcBearerTokenAndGetUserIndentifier(bearerToken.get());

// retrieve Authenticated User from AuthService
AuthenticatedUser authUser = authSvc.lookupUser(userInfo);
if (authUser != null) {
// track the API usage
authUser = userSvc.updateLastApiUseTime(authUser);
return authUser;
} else {
// a valid Token was presented, but we have no associated user account.
logger.log(Level.WARNING, "Bearer token detected, OIDC provider {0} validated Token but no linked UserAccount", userInfo.getUserRepoId());
Copy link
Contributor

Choose a reason for hiding this comment

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

May I also request to log the sub so one may get to know which user is acually trying to do this? Also, it might be good the emphasize that the token is valid - our database is the problem / the sub might have changed on the provider end.

return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Also - one might argue that this should cause a server side error. The token the client sent is valid (so they didn't do anything wrong), but our database is out of sync. Returning null here means the provider will fail to lookup a user, maybe falling back to guest, resulting in potentially strange errors about not be authorized when things clearly are not a clients fault.

(Yes, there might be a case that someone did not signup for an account first, I'll give you that. But then the error message should say that as a hint.)

}
}
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not covered by a test, but is part of the API contract. Please make sure there is a unit test for this.

}

/**
* Verifies the given Bearer token and obtain information about the corresponding user within respective AuthProvider.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Verifies the given Bearer token and obtain information about the corresponding user within respective AuthProvider.
* Verifies the given Bearer token and obtains information about the corresponding user within respective {@link AuthProvider}.

*
* @param token The string containing the encoded JWT
* @return
Copy link
Contributor

Choose a reason for hiding this comment

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

Return is missing

*/
private UserRecordIdentifier verifyOidcBearerTokenAndGetUserIndentifier(String token) throws WrappedAuthErrorResponse {
try {
BearerAccessToken accessToken = BearerAccessToken.parse(token);
// Get list of all authentication providers using Open ID Connect
// @TASK: Limited to OIDCAuthProviders, could be widened to OAuth2Providers.
List<OIDCAuthProvider> providers = authSvc.getAuthenticationProviderIdsOfType(OIDCAuthProvider.class).stream()
.map(providerId -> (OIDCAuthProvider) authSvc.getAuthenticationProvider(providerId))
.collect(Collectors.toUnmodifiableList());
// If not OIDC Provider are configured we cannot validate a Token
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// If not OIDC Provider are configured we cannot validate a Token
// If no OIDC providers are configured, we cannot validate a Token

if(providers.isEmpty()){
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if(providers.isEmpty()){
if (providers.isEmpty()) {

logger.log(Level.WARNING, "Bearer token detected, no OIDC provider configured");
throw new WrappedAuthErrorResponse(BEARER_TOKEN_DETECTED_NO_OIDC_PROVIDER_CONFIGURED);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bad response to send to an API user. It should contain a more helpful message what went wrong and what to do next. In my opinion this should even be a error 500. Someone switched on the feature flag and did not configure any OIDC provider? That does not seem to be a client issue (error 401, see WrappedAuthErrorResponse).

}

// Iterate over all OIDC providers if multiple. Sadly needed as do not know which provided the Token.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Iterate over all OIDC providers if multiple. Sadly needed as do not know which provided the Token.
// Iterate over all OIDC providers, as we cannot not know apriori which provided the token.

for (OIDCAuthProvider provider : providers) {
try {
// The OIDCAuthProvider need to verify a Bearer Token and equip the client means to identify the corresponding AuthenticatedUser.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// The OIDCAuthProvider need to verify a Bearer Token and equip the client means to identify the corresponding AuthenticatedUser.
// The OIDCAuthProvider needs to verify a Bearer Token and equip the client means to identify the corresponding AuthenticatedUser.

I'm sorry, but I don't understand "equip the client means to identify". What are you trying to say?

Optional<UserRecordIdentifier> userInfo = provider.getUserIdentifierForValidToken(accessToken);
if(userInfo.isPresent()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if(userInfo.isPresent()) {
if (userInfo.isPresent()) {

logger.log(Level.FINE, "Bearer token detected, provider {0} confirmed validity and provided identifier", provider.getId());
return userInfo.get();
}
} catch ( IOException| OAuth2Exception e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
} catch ( IOException| OAuth2Exception e) {
} catch (IOException | OAuth2Exception e) {

logger.log(Level.FINE, "Bearer token detected, provider " + provider.getId() + " indicates an invalid Token, skipping", e);
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer not to just let an IOException slide here. That kind of exception means, something went wrong on our end: the remote end died or our internet cable in the DC was cut. Whatever happened, this is bad. This maybe is fine to just note and try next one first, but in case the whole thing is not validating and it's because we couldn't reach one or more providers, that shouldn't be an error 401, but maybe a 503.

Copy link
Member

Choose a reason for hiding this comment

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

It sounds like the 401 is sent later because we aren't doing anything in the catch here.

}
}
} catch (ParseException e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a personal dislike for such long and large try/catch blocks. Can't we finish this business early on and then just go ahead?

logger.log(Level.FINE, "Bearer token detected, unable to parse bearer token (invalid Token)", e);
throw new WrappedAuthErrorResponse(INVALID_BEARER_TOKEN);
}

// No UserInfo returned means we have an invalid access token.
logger.log(Level.FINE, "Bearer token detected, yet no configured OIDC provider validated it.");
throw new WrappedAuthErrorResponse(UNAUTHORIZED_BEARER_TOKEN);
}

/**
* Retrieve the raw, encoded token value from the Authorization Bearer HTTP header as defined in RFC 6750
* @return An {@link Optional} either empty if not present or the raw token from the header
*/
private Optional<String> getRequestApiKey(ContainerRequestContext containerRequestContext) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private Optional<String> getRequestApiKey(ContainerRequestContext containerRequestContext) {
private Optional<String> getRequestApiKey(ContainerRequestContext containerRequestContext) {

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe make static? Also, being private means there's no distinct unit test here to proof business logic works. Baaaad for test coverage.

String headerParamApiKey = containerRequestContext.getHeaderString(HttpHeaders.AUTHORIZATION);
if (headerParamApiKey != null && headerParamApiKey.toLowerCase().startsWith(BEARER_AUTH_SCHEME.toLowerCase() + " ")) {
return Optional.of(headerParamApiKey);
} else {
return Optional.empty();
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
}
}

Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@ public class CompoundAuthMechanism implements AuthMechanism {
private final List<AuthMechanism> authMechanisms = new ArrayList<>();

@Inject
public CompoundAuthMechanism(ApiKeyAuthMechanism apiKeyAuthMechanism, WorkflowKeyAuthMechanism workflowKeyAuthMechanism, SignedUrlAuthMechanism signedUrlAuthMechanism, SessionCookieAuthMechanism sessionCookieAuthMechanism) {
public CompoundAuthMechanism(ApiKeyAuthMechanism apiKeyAuthMechanism, WorkflowKeyAuthMechanism workflowKeyAuthMechanism, SignedUrlAuthMechanism signedUrlAuthMechanism, SessionCookieAuthMechanism sessionCookieAuthMechanism, BearerTokenAuthMechanism bearerTokenAuthMechanism) {
// Auth mechanisms should be ordered by priority here
add(apiKeyAuthMechanism, workflowKeyAuthMechanism, signedUrlAuthMechanism, sessionCookieAuthMechanism);
add(apiKeyAuthMechanism, workflowKeyAuthMechanism, signedUrlAuthMechanism, sessionCookieAuthMechanism,bearerTokenAuthMechanism);
}

public CompoundAuthMechanism(AuthMechanism... authMechanisms) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import com.nimbusds.openid.connect.sdk.op.OIDCProviderConfigurationRequest;
import com.nimbusds.openid.connect.sdk.op.OIDCProviderMetadata;
import edu.harvard.iq.dataverse.authorization.AuthenticatedUserDisplayInfo;
import edu.harvard.iq.dataverse.authorization.UserRecordIdentifier;
import edu.harvard.iq.dataverse.authorization.exceptions.AuthorizationSetupException;
import edu.harvard.iq.dataverse.authorization.providers.oauth2.AbstractOAuth2AuthenticationProvider;
import edu.harvard.iq.dataverse.authorization.providers.oauth2.OAuth2Exception;
Expand Down Expand Up @@ -272,4 +273,18 @@ Optional<UserInfo> getUserInfo(BearerAccessToken accessToken) throws IOException
throw new OAuth2Exception(-1, ex.getMessage(), BundleUtil.getStringFromBundle("auth.providers.exception.userinfo", Arrays.asList(this.getTitle())));
}
}

/**
* Returns the UserRecordIdentifier corresponding to the given accessToken if valid.
* UserRecordIdentifier (same used as in OAuth2UserRecord), i.e. can be used to find a local UserAccount.
* @param accessToken
* @return Returns the UserRecordIdentifier corresponding to the given accessToken if valid.
* @throws IOException
* @throws OAuth2Exception
Comment on lines +282 to +283
Copy link
Contributor

Choose a reason for hiding this comment

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

When are these exceptions thrown?

*/
public Optional<UserRecordIdentifier> getUserIdentifierForValidToken(BearerAccessToken accessToken) throws IOException, OAuth2Exception{
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sorry, but I don't like this. Why would we still throw a checked OAuth2Exception, if we already do an Optional and also the only usage is catching them, putting them into the log and that's it. Can we please move the try/catch block in here then for this type? It's of no use in the auth mechanism, as we just skip anyway.

Copy link
Member

Choose a reason for hiding this comment

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

The idea is to just return an empty optional rather than throwing an OAuth2Exception.

// Request the UserInfoEndpoint to obtain UserInfo, since this endpoint also validate the Token we can reuse the existing code path.
// As an alternative we could use the Introspect Endpoint or assume the Token as some encoded information (i.e. JWT).
return Optional.of(new UserRecordIdentifier( this.getId(), getUserInfo(accessToken).get().getSubject().getValue()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Please please please split this up in multiple lines (with intermediate vars). (Unavoidably when using try/catch as suggested anyway) It does help a lot with debugging, etc. and this line is somewhat spaghetti. At least, please do linebreaks between logical blocks. Thx.

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,12 @@ public enum FeatureFlags {
* @since Dataverse 5.14
*/
API_SESSION_AUTH("api-session-auth"),

/**
* Enables API authentication via Bearer Token.
* @apiNote Raise flag by setting "dataverse.feature.api-bearer-auth"
* @since Dataverse @TODO:
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO. Maybe fine to put 5.14 here, I have a feeling this might make it.

*/
API_BEARER_AUTH("api-bearer-auth"),
;

final String flag;
Expand Down
Loading