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

johannes-darms · 2023-05-15T08:59:39Z

closes As a developer, I'd like to use Bearer Token to send authenticated API requests #9229
replaces 9229 - Add bearer token auth mechanism with feature flag #9532

coveralls · 2023-05-16T18:39:32Z

Coverage: 20.261% (+0.05%) from 20.214% when pulling 9c7fe07 on johannes-darms:9229-2-bearer-token into fc23042 on IQSS:develop.

pdurbin

I tested the previous iteration of this pull request (#9532) and it worked so I'm approving this.

It looks like @johannes-darms incorporated all of my suggestions (thanks!).

@kcondon please note that the previous PR has all the details on how to test. I did my testing with Docker. I'm happy to walk your through what I did.

poikilotherm

I'm sorry to be pedantic, but I do see some places that IMHO could result in a bad API user experience. Thanks for keeping up the good work.

poikilotherm · 2023-05-16T20:48:55Z

doc/sphinx-guides/source/installation/config.rst

@@ -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!**


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>`!**

poikilotherm · 2023-05-16T20:55:43Z

doc/sphinx-guides/source/installation/config.rst

+.. _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.
+


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?

poikilotherm · 2023-05-16T20:57:14Z

docker-compose-dev.yml

@@ -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


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

poikilotherm · 2023-05-16T21:02:07Z

docker-compose-dev.yml

+  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'
+


Please note:

Keycloak 19 is outdated.

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?

poikilotherm · 2023-05-16T21:05:04Z

src/main/java/edu/harvard/iq/dataverse/api/auth/BearerTokenAuthMechanism.java

+    @Inject
+    protected UserServiceBean userSvc;
+    private static final Logger logger = Logger.getLogger(BearerTokenAuthMechanism.class.getCanonicalName());
+    @Override


Suggested change

@Override

@Override

poikilotherm · 2023-05-16T21:55:19Z

src/main/java/edu/harvard/iq/dataverse/settings/FeatureFlags.java

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


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

poikilotherm · 2023-05-16T22:00:10Z

src/test/java/edu/harvard/iq/dataverse/api/auth/BearerTokenAuthMechanismTest.java

+        sut.authSvc = Mockito.mock(AuthenticationServiceBean.class);
+        sut.userSvc = Mockito.mock(UserServiceBean.class);


Please move these into setUp() to make the tests more readable. (Goes as well for all the other occurences...)

poikilotherm · 2023-05-16T22:01:00Z

src/test/java/edu/harvard/iq/dataverse/api/auth/BearerTokenAuthMechanismTest.java

+    }
+
+    @Test
+    @JvmSetting(key = JvmSettings.FEATURE_FLAG, value = "true", varArgs = "api-bearer-auth")


I'm sorry that this is a bit messy - I did not yet implement setting something like this on a class level, this needs tuning. (Would avoid the repetition)

poikilotherm · 2023-05-16T22:02:46Z

src/main/java/edu/harvard/iq/dataverse/api/auth/BearerTokenAuthMechanism.java

+                return null;
+            }
+        }
+        return null;


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

poikilotherm · 2023-05-16T22:04:14Z

...main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/oidc/OIDCAuthProvider.java

+     * @throws IOException
+     * @throws OAuth2Exception


When are these exceptions thrown?

src/main/java/edu/harvard/iq/dataverse/api/auth/BearerTokenAuthMechanism.java

pdurbin · 2023-05-17T18:57:21Z

@poikilotherm I'm not sure if this was your intention by selecting "request changes" but merging is blocked until you switch to "comment" or "approve".

Co-authored-by: Oliver Bertuch <poikilotherm@users.noreply.github.com>

pdurbin

@poikilotherm and I chatted about this PR and left some comment.

src/main/java/edu/harvard/iq/dataverse/api/auth/BearerTokenAuthMechanism.java

pdurbin · 2023-05-17T21:12:12Z

src/main/java/edu/harvard/iq/dataverse/api/auth/BearerTokenAuthMechanism.java

+                        return userInfo.get();
+                    }
+                } catch ( IOException| OAuth2Exception e) {
+                    logger.log(Level.FINE, "Bearer token detected, provider " + provider.getId() + " indicates an invalid Token, skipping", e);


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

pdurbin · 2023-05-17T21:15:44Z

...main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/oidc/OIDCAuthProvider.java

+     * @throws IOException
+     * @throws OAuth2Exception
+     */
+    public Optional<UserRecordIdentifier> getUserIdentifierForValidToken(BearerAccessToken accessToken) throws IOException, OAuth2Exception{


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

kcondon · 2023-05-18T19:00:35Z

Tested with assistance from @pdurbin. Looks good from a basic functionality perspective. I know there are additional suggestions/to dos but seems ok to merge this step?

johannes-darms and others added 5 commits May 15, 2023 08:34

fix(API.Authentication.Bearer): Fix Typo and markup

e2197f7

feat(API.Authentication): Added BearerTokenAuthMechanism

dc92c47

fix(API.Authentication.Bearer): Fix Typo and markup

c4a596f

add bearer token config and docs IQSS#9229

bb1f64e

fix(API.Authentication.Bearer): Resolve comments from @pdurbin

af93712

pdurbin self-assigned this May 15, 2023

pdurbin added Feature: Account & User Info Size: 3 A percentage of a sprint. 2.1 hours. labels May 15, 2023

pdurbin mentioned this pull request May 15, 2023

9229 - Add bearer token auth mechanism with feature flag #9532

Closed

pdurbin approved these changes May 16, 2023

View reviewed changes

pdurbin removed their assignment May 16, 2023

pdurbin requested review from poikilotherm and GPortas May 16, 2023 18:41

kcondon self-assigned this May 16, 2023

poikilotherm requested changes May 16, 2023

View reviewed changes

poikilotherm reviewed May 17, 2023

View reviewed changes

src/main/java/edu/harvard/iq/dataverse/api/auth/BearerTokenAuthMechanism.java Outdated Show resolved Hide resolved

add TODO about caching token validation IQSS#9229

2885f68

Co-authored-by: Oliver Bertuch <poikilotherm@users.noreply.github.com>

pdurbin reviewed May 17, 2023

View reviewed changes

add a TODO about better errors for the client IQSS#9229

9c7fe07

kcondon merged commit 3d8ca99 into IQSS:develop May 18, 2023

pdurbin added this to the 5.14 milestone May 18, 2023

pdurbin mentioned this pull request Sep 27, 2023

Clarify in guides that APIs do not support OIDC #9223

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

johannes-darms commented May 15, 2023 •

edited by pdurbin

Loading

coveralls commented May 16, 2023 •

edited

Loading

pdurbin left a comment

poikilotherm left a comment

poikilotherm May 16, 2023

poikilotherm May 16, 2023

poikilotherm May 16, 2023

poikilotherm May 16, 2023

poikilotherm May 16, 2023

poikilotherm May 16, 2023

poikilotherm May 16, 2023 •

edited

Loading

poikilotherm May 16, 2023

poikilotherm May 16, 2023

poikilotherm May 16, 2023

pdurbin commented May 17, 2023

pdurbin left a comment

pdurbin May 17, 2023

pdurbin May 17, 2023

kcondon commented May 18, 2023

	- 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>`!

		sut.authSvc = Mockito.mock(AuthenticationServiceBean.class);
		sut.userSvc = Mockito.mock(UserServiceBean.class);

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

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

Conversation

johannes-darms commented May 15, 2023 • edited by pdurbin Loading

coveralls commented May 16, 2023 • edited Loading

pdurbin left a comment

Choose a reason for hiding this comment

poikilotherm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poikilotherm May 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdurbin commented May 17, 2023

pdurbin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kcondon commented May 18, 2023

johannes-darms commented May 15, 2023 •

edited by pdurbin

Loading

coveralls commented May 16, 2023 •

edited

Loading

poikilotherm May 16, 2023 •

edited

Loading