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 added this to Ready for Review ⏩ in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) via automation May 15, 2023

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 moved this from Ready for Review ⏩ to In Review 🔎 in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) May 15, 2023

pdurbin approved these changes May 16, 2023

View reviewed changes

IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from In Review 🔎 to Ready for QA ⏩ May 16, 2023

pdurbin removed their assignment May 16, 2023

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

kcondon moved this from Ready for QA ⏩ to QA ✅ in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) May 16, 2023

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
9 of 10 checks passed

IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from QA ✅ to Done 🚀 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

coveralls commented May 16, 2023 •

edited

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

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

coveralls commented May 16, 2023 • edited

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

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

coveralls commented May 16, 2023 •

edited

poikilotherm May 16, 2023 •

edited