-
Notifications
You must be signed in to change notification settings - Fork 490
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
support OAuth2 (ORCID, Google, and GitHub) authentication within dataverse #3338
Comments
Hey @pameyer - is this what you guys are working with @michbarsinai on? Feel free to link up and add any details here. |
Yes. I had observed yesterday that there's a |
My bad - I thought this was a pure HMS interest, I didn't even think of searching for an issue. Good thing I didn't fork into a new repo. I'm happy to change the name of the branch to start with #3338 if that helps. |
@michbarsinai - I'm pretty sure your branch predates this issue. I'll defer to the others on branch naming conventions. |
@michbarsinai please don't worry about the name of the branch but I'll assign this issue to you since you're actively working on it. I moved it to the "Development" column at https://waffle.io/IQSS/dataverse |
@michbarsinai as I mentioned last week, I'm having trouble getting GitHub authentication to "just work" on this branch. If there's any special setup I need to do, please advise. Also, once we have login via Google working, it would be neat to see if I could get my Android app to authenticate to Dataverse via a Google account in order to retrieve one's API token. That's what this issue is about: IQSS/dataverse-android#1 |
I took a quick swing at merging the latest from the develop branch (efc83c6) into ORCiD-integration branch (as of e146e21) but merge conflicts in JsonPrinter.java especially are tricky and I think @michbarsinai is probably in a better position than I am to resolve these. Once this is done I'm happy to jump on this branch and poke around. |
923 - looks out of scope to me (assuming all users are using orcid/oauth2, being able to specify it is redundant). |
There was no description in this issue so yesterday I added one, attempting to define what is in scope and out of scope for this issue as it travels through our Waffle board: https://waffle.io/IQSS/dataverse |
The persistentuserid field in authenticateduserlookup now contains an immutable value (i.e. "1938468") rather than a mutable one such as a username (i.e. "jane_doe"). This is much preferred because most systems allow you to change usernames (from "jane_doe" to "jane_smith", for example, sometimes with side effects) in the case of divorce, etc. Until IQSS#1445 is fixed, we'll persist the mutable value ("jane_doe") in the useridentifier field in the authenticateduser column because it's what is (unfortunately) displayed in the UI. This value is also used for the assigneeidentifier field in the roleassignment table (see IQSS#1151).
See the "2016-10-18 Meeting and demo about OAuth and ORCID login" doc below for notes and screenshots from a demo by @michbarsinai to @djbrooke @kcondon @sekmiller @raprasad @dlmurphy, @jggautier , @scolapasta , @mheppler and myself: https://docs.google.com/document/d/1Lja6sqG0Ljg2Q6suoMJbYr3J01DTpw6HkgvRwdbEDXQ/edit?usp=sharing Public comments are enabled and very welcome! |
We had a list of items coming from the demo. We should work on these
|
Had to re-architect the user/provider stuff to revolve around capability queries (e.g. |
There was some question about whether or not we should support Google and Github (from a product standpoint). We should do so! |
We should test #3410 as part of this. |
@kcondon since you indicated you plan to look at this soon, I thought I'd leave a comment here to help. All the code is in pull request #3539. The description of this issue explains what's in and out of scope and links to related issues. There's a checklist in that description as well, but it's pretty in the weeds and is probably only useful to jog my memory and @mheppler 's of fixes we made in development. Probably the best entry point for QA is this newly re-written section of the Installation Guide: http://guides.dataverse.org/en/3338-oauth-login/installation/config.html#auth-modes-local-vs-remote-vs-both . From that page you find links to information on how to configure OAuth. |
getUsername is used in other places and was preventing conversion!
Better than not being able to create an account! We may need to move away from `@SessionScoped`.
On 2016-10-03 @djbrooke and I (this is @pdurbin writing) discussed how this issue doesn't have a description and agreed that I would take a swing at filling it in to explain in expected to be in scope and out of scope for this issue. I'm very open to feedback and will likely be pulling in @pameyer and others to help define requirements. Some iteration on the lists below is expected.
In scope
shibd
,mod_shib
, etc.) have been configured a boolean is changed to have Shibboleth show up as a login option in the GUI: http://guides.dataverse.org/en/4.5/installation/shibboleth.html#enable-shibboleth . There are some screenshots at c9b1ea0#commitcomment-19632756http://localhost:8080/api/admin/authenticatedUsers
as documented at http://guides.dataverse.org/en/4.5/api/native-api.html#adminOut of scope
Checklist
After meeting on 2016-11-04 to review the revised Log In/Sign Up workflow mockups, we have a high level outline of the features required for this issue. Since that date we have been actively iterating on this checklist, adding items and checking them off. The checklist was moved to the the description of this issue on 2016-12-15 by @pdurbin so we can find it more easily. Further iteration is encouraged!
Sign Up
Log In
:DefaultAuthProvider
in c913d45 - A display order was hard coded in 44ed735subtitle
field already in database?) - https://iqssharvard.mybalsamiq.com/projects/login-dataverse/Log%20In%20-%/dataverse/mra
rather than/dataverse.xhtml?alias=mra
. Fixed in 0040f9cWelcome
Single pg for converting local to either Shibb and OAuth2 ("welcome" or "convert", code cleanup thing, a nice to have, probably deferring until Improve consistency of various remote auth options (Shibboleth vs. OAuth) #3486 ) - convert page added in b360633(NO CONVERTING INSTITUTIONAL TO OUATH, contact Harvard Support msg needed?)curl http://localhost:8080/api/admin/settings/:DebugOAuthAccountType -X PUT -d RANDOM_EMAIL0
throughcurl http://localhost:8080/api/admin/settings/:DebugOAuthAccountType -X PUT -d RANDOM_EMAIL3
f:ajax
params plususernameLabels
andemailLabels
logic from backend which causes blue spinner when entering user info into fieldstabView:newAccountForm:username: Validation Error: Value is required.
- Fixed in 1c69e44tabView:newAccountForm:selectedEmail: Validation Error: Value is required.
- Fixed in 1c69e44duplicate key value violates unique constraint "authenticateduser_email_key"
in the logs). Fixed in ba0a18e.displayIdentifier
and is non-null inlogo
from theauthenticationproviderrow
table. Done in 59b6e20.Convert
Callback
OAuth2Page.error.httpReturnCode
andOAuth2Page.error.messageBody
need to be displayed in UI for end user (no, there's nothing useful per support OAuth2 (ORCID, Google, and GitHub) authentication within dataverse #3338 (comment) ) or in server log for sysAdmin. More logging added in e133c11.OAuth2Exception getting code parameter. HTTP return code: -1. Message: Remote system did not return an authorization code. Message body:
if someone clicks "Deny".Invalid 'state' parameter
bug: support OAuth2 (ORCID, Google, and GitHub) authentication within dataverse #3338 (comment) . Cannot reproduce.Redirect
Account
(ORCID ID not in scope)displayIdentifier
and is non-null inlogo
from theauthenticationproviderrow
table. Done in 59b6e20.Edit Account
Logout
Notifications
email, subject "Dataverse: Your account has been edited"We don't send an email notification to Shib users on convert either (we probably should) so this is being deferred until Notifications - User Account Converted #3525in-app notification, "blah, blah, blah... log in using ORCID from now on..."We don't send an in-app notification to Shib users on convert either (we probably should) so this is being deferred until Notifications - User Account Converted #3525Documentation
3134-docs-build-errors
into3338-oauth-login
)Integration Tests
(Also, we'll have to make sure to refer to ORCID's Trademark and iD Display Guidelines page.)
The text was updated successfully, but these errors were encountered: