-
Notifications
You must be signed in to change notification settings - Fork 492
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
9531 - Add a Log Out endpoint available when the session API auth feature flag is enabled #9533
Conversation
…ssion auth feature flag
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.
I have questions. Here's some initial feedback.
} | ||
else { | ||
((HttpServletResponse) sr1).addHeader(accessControlAllowOriginHeader, "*"); | ||
} |
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.
Can we keep...
((HttpServletResponse) sr1).addHeader("Access-Control-Allow-Origin", "*");
... everywhere instead of changing it from *
to frontendOrigin
in some cases? I fear something will break.
:AllowCors is "true" by default, which means *
- https://guides.dataverse.org/en/5.13/installation/config.html#allowcors
I'm wondering if we can do something like this (untested) instead:
if (settingsSvc.isTrueForKey(SettingsServiceBean.Key.AllowCors, true )) {
((HttpServletResponse) sr1).addHeader("Access-Control-Allow-Credentials", "*");
((HttpServletResponse) sr1).addHeader("Access-Control-Allow-Methods", "PUT, GET, POST, DELETE, OPTIONS");
((HttpServletResponse) sr1).addHeader("Access-Control-Allow-Headers", "Accept, Content-Type, X-Dataverse-Key, Range");
((HttpServletResponse) sr1).addHeader("Access-Control-Expose-Headers", "Accept-Ranges, Content-Range, Content-Encoding");
}
if (FeatureFlags.API_SESSION_AUTH.enabled()) {
((HttpServletResponse) sr1).addHeader("Access-Control-Allow-Credentials", "true");
}
Now that I look at the PR description, I see this:
"On the other hand, a specific URL value for "Access-Control-Allow-Origin" is established (Loaded from a MicroProfile property), replacing the wildcard "*", which is not supported when Access-Control-Allow-Credentials is enabled."
So I guess it was done this way for a reason. I still worry something will break though, since *
is the out of the box default. 🤔
The new header really doesn't work with *
?
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.
FWIW: Here's the discussion about CORS on the bucket for previewers to use direct download: https://github.com/gdcc/dataverse-previewers/wiki/Using-Previewers-with-download-redirects-from-S3 . For the buckets, we found that dropping AllowedOrigin from '*' causes trouble - https://github.com/gdcc/dataverse-previewers/wiki/Using-Previewers-with-download-redirects-from-S3#if-you-are-interested-in-restricting-the-allowedorigins-and-allowedheaders is as far as I got towards a solution.
The previewer info may be out of date or irrelevant but I'd ask/say:
- do you need to worry about bucket rules as well?
- if the change is on all the time, please check to see that hosted previewers work with your fix, and
- if you find a better solution, perhaps we can update the previewer docs as well.
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.
Yep, remote previewers need CORS. I started a Slack thread to discuss this further: https://iqss.slack.com/archives/C04EE66HW2Y/p1682524744005719
SCOPE_FRONTEND(PREFIX, "frontend"), | ||
FRONTEND_URL(SCOPE_FRONTEND, "url"), |
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.
Question: is it necessary to create another scope and do you expect more settings to be added here? From the docs and descriptions I understand this URL is only used for CORS so far, so maybe we could move this to the API scope?
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.
Should this setting be front-end-specific at all? If someone creates another client, would this need to be more than one url? I.e. should this be named something like ALLOWED_CORS_ORIGINS with a value that is a list of urls, defaulting perhaps to just the front end url?
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.
This sounds reasonable (and is easy to apprehend with the lookup function, just give it the String[].class
as parameter...)
Should this include migrating :AllowCors
to a or this JvmSetting? The list could default to "*" as was discussed above. (And probably there is no need for backward compatibility for a DB setting that defaulted to true.)
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.
@qqmyers I prefer to make it front-end specific, just for the Dataverse Frontend SPA. Making it configurable using that list of allowed origins may end up with someone turning on the feature flag to add their own clients and authenticate them via session cookie, while the only intended use case for the feature flag is SPA development. Remember the security risks and the temporary lifetime of the feature flag (It will be removed when we have a final auth mechanism).
@poikilotherm Personally I have no preference in one scope or another. I can move it to the API scope if it is clearer in that way. Ideally, I would have liked to add it to the same scope as the feature flag, because they depend on each other. But the current feature flag mechanism does not support that kind of "subproperties" (If I am not wrong.)
sprint kickoff
|
Conflicts: src/main/java/edu/harvard/iq/dataverse/settings/JvmSettings.java Just a whitespace change vs mail settings being added.
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.
Approved. I've been testing this with the frontend PR.
What this PR does / why we need it
New Log Out endpoint
Only available when the Session Auth feature flag is enabled
Initially, to emulate the JSF Log Out feature in the SPA, we considered the option of removing the JSESSIONID session cookie from the React application code, by accessing the browser cookies using a cookie management library. This solution would have the trade-off of not terminating the session in the backend, as JSF does when clicking log out, but considering the temporary lifetime of the session based API authentication, and that it will be executed on a closed and small beta testing environment, we did not find it a bad solution.
The problem with the previous solution and what makes it unfeasible is that the JSESSIONID cookie is HttpOnly, which means that it cannot be read or managed from javascript code. This has forced us to have to enable an endpoint to perform the Log Out.
Although the endpoint is publicly exposed, it only works when the feature flag is enabled, returning a server error otherwise. When the API evolves towards the final authentication mechanism, the logic of this endpoint will be modified to make it standard for all authentication mechanisms subject to Log Out (API Key is not subject to Log Out).
Which issue(s) this PR closes
Special notes for your reviewer
This PR is part of an end-to-end flow that covers three PRs from three different repositories:
Suggestions on how to test this
Feature flag disabled curl test
curl --cookie "JSESSIONID=<YOUR_COOKIE>" -X POST http://localhost:8080/api/v1/logout
Feature flag enabled curl test
dataverse.feature.api-session-auth=true
curl --cookie "JSESSIONID=<YOUR_COOKIE>" -X POST http://localhost:8080/api/v1/logout
Real end-to-end test
Test the end-to-end flow following the frontend repository PR instructions:
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