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

adding support for changing sessionId at login #7111

Merged
merged 2 commits into from Jul 31, 2020

Conversation

qqmyers
Copy link
Member

@qqmyers qqmyers commented Jul 22, 2020

What this PR does / why we need it: Changes the sessionId at login

Which issue(s) this PR closes:

Closes #3254

Special notes for your reviewer: The code changes here work for the standard login (e.g. as dataverseAdmin) but haven't been checked with other login types. @landreev was going to look for other places where the same sessionId change might be needed for alternate login methods. (FWIW: It does look like this login method calls multiple auth providers, so it may cover all or at least several.) I put the code to change the Id in a util class to make that easier. Nominally this PR doesn't fully close #3254 until it covers a all the login mechanisms. (edit: I was indeed wondering earlier whether this needed to be applied in several places; similar to how we call configureSessionTimeout() from all the different auth provider impls.; but that does not appear to be necessary here: when increasing the session timeout, we wanted to be sure we were doing it only once the user has successfully authenticated/logged in; in this case, we are perfectly fine with issuing a new session before we try to authenticate - L.A.)

Suggestions on how to test this: Open the dev console and watch the jsessionId - cookie (usually) or in the URL -when you succeed in logging in, it should change, and the login should succeed as normal (i.e. no changes except to the session id)
Please test with different auth providers - Shib, etc. - in addition to the builtin provider.

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

Additional documentation:

@coveralls
Copy link

coveralls commented Jul 22, 2020

Coverage Status

Coverage decreased (-0.01%) to 19.599% when pulling 3ac1ed5 on GlobalDataverseCommunityConsortium:IQSS/3254 into 941d17d on IQSS:develop.

IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from Code Review 🦁 to QA 🔎✅ Jul 30, 2020
@kcondon kcondon self-assigned this Jul 30, 2020
@kcondon
Copy link
Contributor

kcondon commented Jul 30, 2020

This works for built in -the session id changes upon log in, but doesn't appear to change when using oauth (orcid). Will also test shib after initial investigation.

@kcondon kcondon merged commit 78527a2 into IQSS:develop Jul 31, 2020
IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from QA 🔎✅ to Done 🚀 Jul 31, 2020
@djbrooke djbrooke added this to the Dataverse 5 milestone Jul 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Sign Up Url - Session ID
5 participants