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

[PLAT-1145][Hotfix] Confirming ORCID account doesn't work if you are logged in #8769

Merged

Conversation

Johnetordoff
Copy link
Contributor

Purpose

If you try to confirm your ORCID account while logged in the OSF will redirect to the dashboard without confirming the account. This fix allows you to comfirm it without logging out.

Changes

  • moves redirect clause to after confirmation clause so it's not missed.
  • adds test

QA Notes

I tested this in the shell by creating a user with (paraphasing) :

user.email_verifications = {'token': {'email': user.email, 'external_identity': {'ORCID' {'fake_external_identity_id': 'fake status'}}}}
me.external_identity = {'ORCID': {'fake_external_identity_id': 'fake status'}}

and hitting endpoint http://localhost:5000/confirm/external/<user guid>/token/?destination=dashboard then checking account setting to see if the user had a confirmed fake ORCID.

Documentation

🐞 fix, no docs

Side Effects

None that I know of.

Ticket

https://openscience.atlassian.net/browse/PLAT-1145

Copy link
Member

@erinspace erinspace left a comment

Choose a reason for hiding this comment

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

Just 2 questions!

self.provider_id: 'PENDING'
}
}
user = AuthUserFactory(external_identity=external_identity)
Copy link
Member

Choose a reason for hiding this comment

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

Could you modify self.user for your test instead of taking the time to create your own?

@@ -543,6 +526,23 @@ def external_login_confirm_email_get(auth, uid, token):
user.verification_key = generate_verification_key()
user.save()

# if user is already logged in
Copy link
Member

Choose a reason for hiding this comment

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

Curious of how much of the above should indeed be happening before the current logged in status of the user is checked - for example user.date_last_logged_in = timezone.now() should probably still happen after the logged in check.

…direct and make sure user_last_logged_in date reflects times the user isn't already logged in
status.push_status_message(language.WELCOME_MESSAGE, kind='default', jumbotron=True, trust=True, id='welcome_message')
return redirect(web_url_for('dashboard'))

user.date_last_logged_in = timezone.now()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this moved? It looks like this change will never get saved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what you mean, the idea with this is that if the user was logged in already their date_last_logged_in shouldn't change, but if they log in with this function it should miss the redirect and hit that line. Edit: look at @erinspace comment above.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, but the user isn't being saved after this point.

@sloria sloria merged commit 552498f into CenterForOpenScience:master Nov 6, 2018
@sloria sloria mentioned this pull request Nov 8, 2018
@sloria sloria deleted the verifiy-while-orcid-loggedin branch November 8, 2018 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants