-
Notifications
You must be signed in to change notification settings - Fork 477
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
Conversation
Related to #9299, as using a feature flag. |
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. Merge this first, then refactor after oliver PR. The consensus is to have this come AFTER #9299 Size: 3 |
@GPortas feature flags has been merged: |
2943ff7
to
e427b82
Compare
The failing test is just this: |
The above works but I don't want to have to edit Instead I want to set an environment variable like this...
... and then just start the image (without rebuilding it). This seemed to work for me last time with an uncommitted feature flag ( 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.) |
Changing from hyphens to underscores didn't help...
... I checked and...
... still didn't work. |
In 5eb13b0 I went ahead and pushed some doc changes (feel free to edit or revert). |
@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 😄 |
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.) |
@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
|
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
I guess this is fine. I didn't have to rebuild the image. I still need to add some docs though. |
I added some additional docs in 9764188. This is ready for QA. |
There was a problem hiding this 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!! 🎉
@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: |
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.
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:
dataverse.feature.api-session-auth=true
curl --cookie "JSESSIONID=<cookie_value>" -X GET http://localhost:8080/api/datasets/2
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