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

Closed
pameyer opened this Issue Sep 8, 2016 · 43 comments

Comments

@pameyer
Contributor

pameyer commented Sep 8, 2016

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

Out 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

  • New "convert account" info msg to include OAuth2 and not specific to "institution" - https://iqssharvard.mybalsamiq.com/projects/login-dataverse/Sign%20Up%20-%20Dataverse - fixed (but simplfied as a new static message) in d1377f9
  • Consider rewording "Too many passwords? Log In with existing credentials." This might imply that the person already has some sort of Dataverse account. They might be more likely to sign up if it's easier because they can use their Google account or whatever. See FIXME added in 448b13d

Log In

  • New single, scalable log in form layout, with "Other Log In Options" - https://iqssharvard.mybalsamiq.com/projects/login-dataverse/Log%20In%20-%20ORCID - bulk of work done in c913d45
  • Backend "log in display order" configuration (no UI for this but will be documented in the guides) - Rather than a "display order", this has been simplified into a single setting called :DefaultAuthProvider in c913d45 - A display order was hard coded in 44ed735
  • OAuth2 to have "info message" only (no form inputs) views (rather than just jumping right into the external site; backend: use subtitle field already in database?) - https://iqssharvard.mybalsamiq.com/projects/login-dataverse/Log%20In%20-%
  • Reconcile previous Shib msg'n to follow format of OAuth2 msg'n (deferred until #3486)
    • Shib: Use Dataverse with your institutional log in instead of creating an account. Learn More. Leaving your institution? Please contact Dataverse Support for assistance.
    • OAuth: Log in with your GitHub credentials. Having trouble? Please contact Dataverse Support for assistance.
  • Sort order for "Other Log In Options" buttons
    • Dataverse (builtin) always first
    • Shib, ORCID
    • (the rest alphabetical...?) GitHub, Google
  • Help text, branding, etc as outlined by ORCID branding and trademark guidelines (Highlighted items are included elsewhere in this checklist, drawn from feedback from ORCID Support at https://docs.google.com/document/d/12Ee7xb8J3O2RIWM8CyyICnevuh1hq_mPEgJAv006BNY/edit?usp=sharing )
    • ORCID logo
    • "Learn more..." text
    • Modal -- not using modal/popup, instead loading ORCID in same window
    • Redirect? -- callback?
    • Skip?
  • Hide "Cancel" button (...always goes to Root Dataverse?)
    • Research + test
  • For local accounts log in redirects to account pg and not previously viewed pg, after toggling Other Log In Options. Fixed for local accounts in 45c9472.
  • Only render "Other Log In Options" if there is more than one. Fixed in a405eb3
  • Like Shibboleth users (#1551) OAuth users do not benefit from redirect logic. Fixed in 7e526df
  • Like Builtin users, OAuth users should be redirected to /dataverse/mra rather than /dataverse.xhtml?alias=mra. Fixed in 0040f9c
  • Add "learn more" User Guide links to help text
  • Consistent help text msg:
    • Your Institution: Use Dataverse with your institutional log in instead of creating an account. Learn More. Leaving your institution? Please contact Dataverse Support for assistance.
    • ORCID/OAuth: Log in with your ORCID Sandbox credentials. Learn More. Having trouble? Please contact Dataverse Support for assistance.

Welcome

  • Fix bug where first and last name are being put into "Given Name" and nothing is being put into "Family Name". - fixed in 808b9cc
  • Single pg for converting local to either Shibb and OAuth2 ("welcome" or "convert", code cleanup thing, a nice to have, probably deferring until #3486 ) - convert page added in b360633
  • Validation for duplicate account info - https://iqssharvard.mybalsamiq.com/projects/login-dataverse/Account%20-%20Create%20Account%20ORCID%20Error - fixed in b360633
  • (NO CONVERTING INSTITUTIONAL TO OUATH, contact Harvard Support msg needed?)
  • Don't suggest a username. Fixed in f9cde16
  • Multiple email input/select (PrimeFaces Input Autocomplete Dropdown: http://www.primefaces.org/showcase/ui/input/autoComplete.xhtml) - Fixed in c2c7d3e. To exercise use curl http://localhost:8080/api/admin/settings/:DebugOAuthAccountType -X PUT -d RANDOM_EMAIL0 through curl http://localhost:8080/api/admin/settings/:DebugOAuthAccountType -X PUT -d RANDOM_EMAIL3
  • (@scolapasta and @pdurbin) Remove no longer used f:ajax params plus usernameLabels and emailLabels logic from backend which causes blue spinner when entering user info into fields
  • Validation message problem: Blanking out the username shows tabView:newAccountForm:username: Validation Error: Value is required. - Fixed in 1c69e44
  • Validation message problem: Blanking out the first and last name doesn't show a message next to the field indicating a problem. - Fixed in 1c69e44
  • Validation message problem: if you blank out the email after a failed save attempt it shows tabView:newAccountForm:selectedEmail: Validation Error: Value is required. - Fixed in 1c69e44
  • Improve consistency of label, tips, buttons, tab title, page title, etc.
  • "Select email" should say enter email or “Email” - fixed in a96184a
  • Email error shows although email has not been entered - c149ca4
  • Username alert says OK but there is not a username entered - fixed in d19adb2
  • Display ORCID iD with logo - fixed in d95a782
  • As of 2e49d12 there's an intermittent bug where the the tab with all the information to creating an OAuth account is collapsed. We saw this on 2016-12-02 during the 10am usability test. - Fixed in 5e5d00d.
  • Uniqueness of username is not checked. A number will be added silently if there's a conflict (i.e. dataverseAdmin1). - fixed in 1c69e44
  • 500 Internal Server Error if you navigate directly to http://localhost:8080/oauth2/firstLogin.xhtml - Fixed in 0ca251b.
  • As of c2c7d3e or earlier email validation is not checking for duplicates. 500 error (duplicate key value violates unique constraint "authenticateduser_email_key" in the logs). Fixed in ba0a18e.
  • Give all fields (especially email) room to breath so that input areas are large enough not to be cut off (consistent with Sign Up form).
  • Fix bug where when multiple emails are sent by on OAuth provider, more than one input component is rendered. Workaround added in 7f00410 It was actually just a CSS thing. The fix could be further improved via CSS, perhaps.
  • Try to reproduce bug where you log back in with ORCID and get a 404 at https://shibtest.dataverse.org/oauth2/dataverse.xhtml rather than going to https://shibtest.dataverse.org/dataverse.xhtml . Cannot reproduce but should let QA know about this.
  • Make a final decision branch on suggesting a username or not for OAuth users (ORCID, Google, GitHub). When you log in with an institution (Shibboleth, HarvardKey) you are automatically assigned a username under the idea of "don't make me think." We could make all "remote auth" options (ORCID, Google, GitHub, institution/Shibboleth/HarvardKey) consistent when we work on #3486. When you create a local/builtin account we do not suggest a username based on what they type for first and last name. Deferring this and will revisit all places in the app where usernames can be suggested.
  • Confirm that when ORCID passes "affiliation" that is is populated on the welcome page. Fixed in 0ebed7b.
  • Improve help text:
    • Create (Currently): This information is provided by ORCID and will be used to create your Demo Dataverse account.
    • Convert (Currently): Please enter your Demo Dataverse account username and password to convert your account to use ORCID for authentication.
    • Suggested: "...you will need to log in through ORCID from now on, blah blah blah."
  • Show icon generically but only for ORCID because it's stored in the boolean displayIdentifier and is non-null in logo from the authenticationproviderrow table. Done in 59b6e20.

Convert

Callback

  • Clean up layout, consistent with other error pgs (404, 500, et al)
  • Determine if OAuth2Page.error.httpReturnCode and OAuth2Page.error.messageBody need to be displayed in UI for end user (no, there's nothing useful per #3338 (comment) ) or in server log for sysAdmin. More logging added in e133c11.
  • Show a reasonable error when user clicks "Deny" on the ORCID side. See comments from ORCID Support at https://docs.google.com/document/d/12Ee7xb8J3O2RIWM8CyyICnevuh1hq_mPEgJAv006BNY/edit?usp=sharing . The page was cleaned up and as of ba0a18e it says "OAuth2 Error - Sorry, the identification process did not succeed. Remote system did not return an authorization code. If you believe this is an error, please contact Dataverse Support for assistance." Seems fine. In server.log you see OAuth2Exception getting code parameter. HTTP return code: -1. Message: Remote system did not return an authorization code. Message body: if someone clicks "Deny".
  • Fix Invalid 'state' parameter bug: #3338 (comment) . Cannot reproduce.

Redirect

  • Retest that redirect logic still works following refactoring done in 3f03f6f. Tested by @pdurbin on 3f03f6f on shibtest. Works for dataverse, dataset, and file.

Account

  • Same as local/built-in - https://iqssharvard.mybalsamiq.com/projects/login-dataverse/Account%20-%20Dataverse%20ORCID - showing ORCID iD as of 8d24183
  • Info msg about converted account/support email link - added in 8d24183
  • Verify any and all email addresses received from OAuth2 or user submitted - verification is enforced, no code change required.
  • (ORCID ID not in scope)
  • Bean validation rules are not enforced when editing accounts. (Formerly "Lots of bugs with validation?") See comment at c64fd62#commitcomment-19928538 - fixed in 3b29569
  • Show ORCID icon next to ORCID iD as requested by ORCID Support at https://docs.google.com/document/d/12Ee7xb8J3O2RIWM8CyyICnevuh1hq_mPEgJAv006BNY/edit?usp=sharing
  • "Not Verified" label displays for all new accounts, no "Verify Email" button is displayed -- known issue tracked in #3438
  • Change account page message to dynamic version (can be "Google" or "GitHub") of this: "Your Dataverse account uses ORCID for login. If you're interested in changing login method, please contact Dataverse Support for assistance." Updated in 3a374f1 (used "you are" instead of "you're" to avoid needing two single quotes in bundle file).
  • Show icon generically but only for ORCID because it's stored in the boolean displayIdentifier and is non-null in logo from the authenticationproviderrow table. Done in 59b6e20.

Edit Account

Logout

Notifications

  • Create
    • email, subject "Dataverse: Your account has been created" (...just like builtin/shib) - Added in 373a0f8.
    • in-app notification, "Welcome to Demo Dataverse!" (...just like builtin/shib) - Added in 373a0f8.
  • Convert
    • 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 #3525
    • in-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 #3525

Documentation

  • Just like Shibboleth (Institutional Login), the User Guide should be updated to include information about ORCID login: http://guides.dataverse.org/en/4.5.1/user/account.html#institutional-log-in
  • Update "Institutional Login" screenshots with new Log In pg UI
  • Add sections for Google, GitHub or "Other OAuth"
  • Just like Shibboleth, a sysadmin must be able to convert a OAuth/ORCID account to a local account. Write doc similar to http://guides.dataverse.org/en/4.5/installation/shibboleth.html#converting-shibboleth-users-to-local . Added in 33387dd.
  • Just like Shibboeth, OAuth/ORCID login must be disabled out of the box but instructions in the Installation Guide must be provided for how a sysadmin would configure it.
  • Make it so PDFs can be built again (#3134, start by merging branch 3134-docs-build-errors into 3338-oauth-login)
  • Incorporate suggested wording for "Shibboleth: Improve help text/provide options if your institution isn't listed" issue #3420.
  • Clean up/organize Shibboleth guides, moving content into Admin Guide, Preparation > Installation Guide, etc.

Integration Tests

  • Automated tests to convert an OAuth account to local - Done in 2f46105

(Also, we'll have to make sure to refer to ORCID's Trademark and iD Display Guidelines page.)

screen shot 2016-11-04 at 4 09 23 pm

@djbrooke

This comment has been minimized.

Contributor

djbrooke commented Sep 9, 2016

Hey @pameyer - is this what you guys are working with @michbarsinai on? Feel free to link up and add any details here.

@pdurbin

This comment has been minimized.

Member

pdurbin commented Sep 9, 2016

Yes. I had observed yesterday that there's a ORCiD-integration branch at https://github.com/IQSS/dataverse/tree/ORCiD-integration that isn't prepended with a GitHub issue number so @pameyer @bmckinney agreed that we should create one. #829 is related but less well defined, in my opinion. This new issue is clearly about OAuth. #923 is also related but out of scope for now, I believe.

@michbarsinai

This comment has been minimized.

Member

michbarsinai commented Sep 9, 2016

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.

@pameyer

This comment has been minimized.

Contributor

pameyer commented Sep 9, 2016

@michbarsinai - I'm pretty sure your branch predates this issue. I'll defer to the others on branch naming conventions.

@pdurbin

This comment has been minimized.

Member

pdurbin commented Sep 14, 2016

@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

@pdurbin

This comment has been minimized.

Member

pdurbin commented Sep 20, 2016

@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

@pdurbin

This comment has been minimized.

Member

pdurbin commented Sep 28, 2016

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.

@pameyer

This comment has been minimized.

Contributor

pameyer commented Oct 3, 2016

923 - looks out of scope to me (assuming all users are using orcid/oauth2, being able to specify it is redundant).
2974 - possibly in scope (but may make sense to leave as separate issue)

@pdurbin

This comment has been minimized.

Member

pdurbin commented Oct 4, 2016

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

pdurbin added a commit to pdurbin/dataverse that referenced this issue Oct 5, 2016

Persist immutable id (a number) rather than mutable one IQSS#3338
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).
@pdurbin

This comment has been minimized.

Member

pdurbin commented Oct 20, 2016

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!

@pdurbin pdurbin changed the title from support OAuth2 (ORCID) authentication within dataverse to support OAuth2 (ORCID, Google, and GitHub) authentication within dataverse Oct 20, 2016

@djbrooke

This comment has been minimized.

Contributor

djbrooke commented Oct 20, 2016

We had a list of items coming from the demo. We should work on these

@michbarsinai

This comment has been minimized.

Member

michbarsinai commented Oct 21, 2016

Had to re-architect the user/provider stuff to revolve around capability queries (e.g. provider.isUserInfoUpdateAllowed()) rather than classes (user.isBuiltInUser()). Here's a sequence diagram to make some sense of it: How the Account Information page decides whether to show the account info update screen. PlantUml file is in doc/Architecture/update-user-account-info.puml.

update-user-account-info

@djbrooke

This comment has been minimized.

Contributor

djbrooke commented Oct 24, 2016

There was some question about whether or not we should support Google and Github (from a product standpoint). We should do so!

@djbrooke

This comment has been minimized.

Contributor

djbrooke commented Nov 3, 2016

We should test #3410 as part of this.

pdurbin added a commit that referenced this issue Dec 22, 2016

pdurbin added a commit that referenced this issue Jan 4, 2017

pdurbin added a commit that referenced this issue Jan 5, 2017

pdurbin added a commit that referenced this issue Jan 5, 2017

pdurbin added a commit that referenced this issue Jan 5, 2017

pdurbin added a commit that referenced this issue Jan 12, 2017

pdurbin added a commit that referenced this issue Jan 13, 2017

@pdurbin

This comment has been minimized.

Member

pdurbin commented Jan 13, 2017

@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.

pdurbin added a commit that referenced this issue Jan 13, 2017

pdurbin added a commit that referenced this issue Jan 17, 2017

pdurbin added a commit that referenced this issue Jan 18, 2017

pdurbin added a commit that referenced this issue Jan 18, 2017

pdurbin added a commit that referenced this issue Jan 18, 2017

pdurbin added a commit that referenced this issue Jan 19, 2017

pdurbin added a commit that referenced this issue Jan 19, 2017

pdurbin added a commit that referenced this issue Jan 19, 2017

pdurbin added a commit that referenced this issue Jan 19, 2017

pdurbin added a commit that referenced this issue Jan 19, 2017

pdurbin added a commit that referenced this issue Jan 20, 2017

pdurbin added a commit that referenced this issue Jan 20, 2017

be more careful about nulling out username #3338
getUsername is used in other places and was preventing conversion!

pdurbin added a commit that referenced this issue Jan 20, 2017

revert back to bug where username is suggested #3338
Better than not being able to create an account! We may need to move
away from `@SessionScoped`.

@kcondon kcondon closed this Jan 20, 2017

@kcondon kcondon removed the Status: QA label Jan 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment