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

9063 - add session API auth mechanism with feature flag #9290

Merged
merged 12 commits into from Mar 28, 2023

Conversation

GPortas
Copy link
Contributor

@GPortas GPortas commented Jan 16, 2023

What this PR does / why we need it:

As stated in #9063, for the development of the new Dataverse Single Page Application (SPA), the SPA needs to be able to authenticate against Dataverse Native API.

The final authentication mechanism for the SPA is yet to be developed. Meanwhile, only for development purposes, this mechanism allows to authenticate given a session (Defined by JSESSIONID cookie). Due to the CSRF risks of using the JSESSIONID cookie for API authentication, this functionality should not be enabled for production purposes.

In order to keep this functionality disabled by default and following the docs, a new JVMSetting has been created, with a boolean value of 'false' by default.

Being aware of @poikilotherm's work in progress of #9230, where he developed a generic mechanism for feature flags, the naming of the JVMSetting created in this PR follows the same structure he uses in his PR, considering a later refactor that will include this JVMSetting into the generic mechanism.

Edit (01/17): After discussion with @pdurbin and @poikilotherm, we have decided to include the generic mechanism for feature flags that Oliver has worked on and its associated documentation in this PR.

Edit (01/18): New issue created for the new generic mechanism for feature flags, #9297.

TODOs.

  • Add feature flag for session API auth
  • Add generic feature flag mechanism
  • Add documentation for feature flag for session API auth

Which issue(s) this PR closes:

Special notes for your reviewer:

I have considered to add test coverage to the AbstractApiBean changes, but I find it difficult to test the changes in isolation. The future refactor towards a filter-based module should make this easier.

Suggestions on how to test this:

Follow the next steps:

  1. Enable the JVMSetting in microprofile-config.properties file: dataverse.feature.api-session-auth=true
  2. Build and deploy Dataverse locally.
  3. Navigate to Dataverse Login Page.
  4. Without logging in, obtain the current JSESSIONID cookie value from the browser (it corresponds to a Guest User).
  5. Using the obtained cookie value, perform a curl command against a secured Dataverse Native API endpoint. For example: curl --cookie "JSESSIONID=<cookie_value>" -X GET http://localhost:8080/api/datasets/2
  6. A message indicating that the guest user is not authorized should appear.
  7. Again on the Dataverse Login Page, log in with valid user credentials. The JSESSIONID cookie value should be updated.
  8. Repeat the same curl test but with the new cookie value.
  9. The requested data should be successfully retrieved.

An additional test is to repeat the above steps but keeping dataverse.feature.api-session-auth=false. Whether logged in or not, requests must fail by recognizing a guest user (Default Dataverse behavior).

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?:

Not sure

Additional documentation:

No

@GPortas GPortas added Feature: API User Role: API User Makes use of APIs NIH OTA: 1.7.1 (reArchitecture) 7 | 1.7.1 | Research & architecture for separating backend and frontend to enable a flexible, sca... labels Jan 17, 2023
@poikilotherm
Copy link
Contributor

Related to #9299, as using a feature flag.

@mreekie
Copy link

mreekie commented Jan 18, 2023

This feature makes is possible to for a user to access our entire API with the UI cookies.

This is the addition of a mechanism to help speed up SPA mechanism.
For devel purposes.
This coincides with work Oliver is working on in 9299 to setup a place to put these experimental dev only settings.

Merge this first, then refactor after oliver PR.
Set this aside, and get 9299 through first, followed by this one.

The consensus is to have this come AFTER #9299

Size: 3

@mreekie mreekie added this to New in deleteMeAfterTesting via automation Feb 23, 2023
@mreekie mreekie removed this from New in deleteMeAfterTesting Feb 23, 2023
@mreekie mreekie added this to New in deleteMeAfterTesting via automation Feb 23, 2023
@mreekie mreekie removed this from New in deleteMeAfterTesting Feb 23, 2023
@mreekie mreekie added this to New in deleteMeAfterTesting via automation Feb 23, 2023
@mreekie mreekie removed this from New in deleteMeAfterTesting Feb 23, 2023
@pdurbin pdurbin moved this from ▶ SPRINT READY to This Sprint 🏃‍♀️ 🏃 in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) Mar 15, 2023
@GPortas GPortas added Size: 10 A percentage of a sprint. 7 hours. and removed Size: 3 A percentage of a sprint. 2.1 hours. labels Mar 15, 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
@pdurbin
Copy link
Member

pdurbin commented Mar 20, 2023

@GPortas feature flags has been merged:

@GPortas GPortas moved this from This Sprint 🏃‍♀️ 🏃 to IQSS Team - In Progress 💻 in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) Mar 23, 2023
@GPortas GPortas self-assigned this Mar 23, 2023
@GPortas GPortas marked this pull request as ready for review March 24, 2023 17:27
@GPortas GPortas moved this from IQSS Team - In Progress 💻 to Ready for Review ⏩ in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) Mar 24, 2023
@GPortas GPortas removed their assignment Mar 24, 2023
@coveralls
Copy link

coveralls commented Mar 24, 2023

Coverage Status

Coverage: 20.153% (+0.004%) from 20.149% when pulling 9764188 on GPortas:9063-session-api-auth into 47f0b0e on IQSS:develop.

@pdurbin
Copy link
Member

pdurbin commented Mar 24, 2023

The failing test is just this:

@pdurbin pdurbin self-assigned this Mar 24, 2023
@pdurbin pdurbin moved this from Ready for Review ⏩ to Review 🔎 in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) Mar 24, 2023
@pdurbin
Copy link
Member

pdurbin commented Mar 27, 2023

Enable the JVMSetting in microprofile-config.properties file: dataverse.feature.api-session-auth=true

The above works but I don't want to have to edit src/main/resources/META-INF/microprofile-config.properties and then rebuild the image with mvn -Pct clean package.

Instead I want to set an environment variable like this...

export DATAVERSE_FEATURE_API_SESSION_AUTH=1

... and then just start the image (without rebuilding it).

This seemed to work for me last time with an uncommitted feature flag (export DATAVERSE_FEATURE_UPLOAD_METHODS=1) I played around with: #9299 (comment)

I'm not sure why it isn't working now. Underscores vs. hyphens? I'm really not sure. I'll poke at the code some more but @poikilotherm (who added feature flags) might know.

(Unrelated, I also have a few doc changes queued up.)

@pdurbin
Copy link
Member

pdurbin commented Mar 27, 2023

Changing from hyphens to underscores didn't help...

-    API_SESSION_AUTH("api-session-auth"),
+    API_SESSION_AUTH("api_session_auth"),

... I checked and...

export DATAVERSE_FEATURE_API_SESSION_AUTH=1

... still didn't work.

@pdurbin
Copy link
Member

pdurbin commented Mar 27, 2023

In 5eb13b0 I went ahead and pushed some doc changes (feel free to edit or revert).

@poikilotherm
Copy link
Contributor

@pdurbin the docs you want to look at are these: https://docs.docker.com/compose/environment-variables/set-environment-variables

Docker Compose will not just pickup any (random) env var from your shell without you telling it where it shall go. Plenty of options to go for, depending on your favorite. Enjoy 😄

@poikilotherm
Copy link
Contributor

poikilotherm commented Mar 27, 2023

Changing from hyphens to underscores didn't help...

-    API_SESSION_AUTH("api-session-auth"),
+    API_SESSION_AUTH("api_session_auth"),

... I checked and...

export DATAVERSE_FEATURE_API_SESSION_AUTH=1

... still didn't work.

No no no, you are mixing things up here! The definition is with dashes, MPCONFIG spec says "go look for env vars and start replacing special chars with dashes". You need to be aware that you must transfer the env var into the container, it's not enough to have it in your shell (exported or not). (See also my comment above with a docs link how to do the transfer.)

@pdurbin
Copy link
Member

pdurbin commented Mar 27, 2023

@poikilotherm thank you, yes, I had found that doc before I left the office.

Any change I make locally to a file I don't want to show up in git diff so I'm thinking I'll try this next...

  • cp .env .env.pem (.pem is in our .gitignore)
  • vi .env.pem adding DATAVERSE_FEATURE_API_SESSION_AUTH=1
  • docker-compose --env-file .env.pem -f docker-compose-dev.yml up
  • if this works, add it to the docs somewhere

@pdurbin
Copy link
Member

pdurbin commented Mar 28, 2023

Ok, the idea above worked but not with the .env file. Instead I did this (again .pem because it's in .gitignore so I don't have to worry about accidentally committing docker-compose-dev.yml.pem):

cp docker-compose-dev.yml docker-compose-dev.yml.pem
echo "DATAVERSE_FEATURE_API_SESSION_AUTH=1" >> docker-compose-dev.yml.pem
docker-compose -f docker-compose-dev.yml.pem up

I guess this is fine. I didn't have to rebuild the image. I still need to add some docs though.

@pdurbin
Copy link
Member

pdurbin commented Mar 28, 2023

I added some additional docs in 9764188. This is ready for QA.

Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

Works great! Thanks, @GPortas!! 🎉

IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from Review 🔎 to Ready for QA ⏩ Mar 28, 2023
@pdurbin pdurbin removed their assignment Mar 28, 2023
@pdurbin
Copy link
Member

pdurbin commented Mar 28, 2023

@kcondon as always, I'm happy to chat about what I did. As a quick tip, here's where I found the cookie in Firefox:

Screen Shot 2023-03-27 at 2 39 26 PM

@kcondon kcondon self-assigned this Mar 28, 2023
@kcondon kcondon merged commit 49ef7f8 into IQSS:develop Mar 28, 2023
6 checks passed
IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from QA ✅ to Done 🚀 Mar 28, 2023
@pdurbin pdurbin added this to the 5.14 milestone May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature: API 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: 10 A percentage of a sprint. 7 hours. User Role: API User Makes use of APIs
Projects
Status: No status
6 participants