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.

Sign up for GitHub

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

Jump to bottom

Persona shutting down 11/2016: Move from Persona to Google sign-in #530

Closed
kilodalton opened this issue Jan 12, 2016 · 26 comments
Closed

Persona shutting down 11/2016: Move from Persona to Google sign-in #530

kilodalton opened this issue Jan 12, 2016 · 26 comments
Assignees
@mrmin123
Labels
backend feature priority: high

Comments

@kilodalton
Copy link
Contributor

kilodalton commented Jan 12, 2016 •
edited by kgliu0101
Loading

Persona shutting down 11/2016

From ENCODE Redmine Ticket 3575:

"We currently authenticate users to an email address using Persona, but that service is scheduled to be shut down in November 2016. https://mail.mozilla.org/pipermail/persona-notices/2016/000005.html

Instead of authenticating the email address with Persona we could authenticate it using Google Sign In. https://developers.google.com/identity/sign-in/web/

Unfortunately Google Sign In requires per-domain configuration, so it won't work out of the box with our demo subdomains. This might be worked around by configuring the nginx proxy to accept an alternative url on a fixed domain, e.g. instead of https://feature123.demo.encodedcc.org/login perhaps use https://signin.encodedcc.org/feature123/login. See: http://stackoverflow.com/questions/24342338/google-oauth-subdomains

[Laurence] spoke to Pedro briefly about using Google Sign In rather than Persona a month or two ago, so he may have some experience with it already."

Will require some backend and some UI work.
@kilodalton kilodalton added the R5 label Feb 11, 2016
@wrightmw wrightmw added R6 and removed R5 labels Feb 25, 2016
@mrmin123 mrmin123 self-assigned this Mar 16, 2016
@mrmin123 mrmin123 self-assigned this Mar 24, 2016
@mrmin123
Copy link
Contributor

Preliminary update:

I've more or less managed to transition the old Persona authentication system on the frontend and backend to the Google authentication system. The login functionality works as expected, and other interface functionality works as expected once logged in, as well.

Changes are primarily in mixins.js (Google login action handling by frontend) and apps.js (Google login button rendering on frontend), along with some new CSS additions for the button rendering. Backend changes mainly involved switching everything from the persona namespace to google. Most of the cookie/payload decryption from the Persona implementation was also removed from persona.py.

Current todo/challenges:

  • Currently the bundle JS file and google login script are loaded asynchronously, but are dependent on each other. On the browser, this sometimes results in the global gapi variable not being defined, which is typically remedied by refreshing the page, but tests seem to consistently fail because of this. I tried modifying inline.js to have it fire off a function once both files were loaded via scriptjs, but 1) the bundle renamer script couldn't rename the @@bundleJsFile reference when it was located in there and 2) I have no guarantee that scriptjs can fire off a reactjs function from within inline.js. I am currently looking into other methods of loading the scripts in the correct order.
  • Needs a new, non-testing/official ClinGen appID. We shouldn't use the current one I'm using for development and testing
    • Need to set up fixed subdomains for testing, at least until we implement a separate login URL/server to handle all login requests. This is due to Google's restriction of no wildcard subdomains for login requests
  • Might need a more secure implementation. Most of the code I ripped out of persona.py had to do with verifying the legitimacy of the login request. I think with Google auth the considerations are different, but I do need to look into increasing security on the current implementation.

@mrmin123
Copy link
Contributor

@jimmyzhen @forresttanaka
The travis ci run: https://travis-ci.org/ClinGen/clincoded/jobs/122863050
My branch is 530_mc_google_auth_frontend

The test fails on window.gapi.load in mixins.js because I suspect that google's platform.js isn't loading (specified in the <head> of app.js. I previously had it set to gapi.load, and that worked fine too on the local server, but the linter would complain that gapi was undefined (it gets defined by platform.js), so I changed it to window.gapi, and thinking of keeping it that way.

@mrmin123
Copy link
Contributor

@selinad @wrightmw
This is currently how the new google login button is implemented:
image

Upon clicking it, you're either logged in as usual, or if you have multiple Google accounts associated on your browser, you get a popup like so:
image

These branding guidelines exist for the login button, in case you want to customize the look of it further.

@mrmin123
Copy link
Contributor

Update:

  • Proper loading order of Google's platform.js and our bundle file has been established
  • I managed to get the jest tests to pass by surrounding the mixin's calls to gapi in a check to see if gapi is defined, and by responding to the fail state with the content it expects. Unfortunately the context it expects is the default splash page for the site. Typically I feel like this wouldn't be an issue, since gapi should always be available now, UNLESS the user is using an adblocker/scriptblocker and is blocking google APIs as well. Then gapi becomes undefined in the browser, but they'd just see the portal screen, with no error messages. Maybe we should just let all users know that they need to whitelist the site on their adblockers?

@mrmin123
Copy link
Contributor

I've spun up an instance here: http://530-mc-google-auth-frontend-862500c-minchoi.instance.clinicalgenome.org/

You will need to log in to your Stanford-associated Google account. This branch passes all tests and has no issues with gapi loading.

I believe the code itself is ready for implementation, but there are a number of factors that need to be addressed which makes me feel like we should hold off for an R7 implementation. Please let me know if you disagree or have other thoughts/concerns:

  1. @kilodalton As discussed, we need to look into creating/getting access to accounts that should act as the owner for the ClinGen curation interface login key via the Google Developer's console (link)
    • This account, for the moment, needs to whitelist a number of fixed subdomains for our test instances. This will need to be the case until we can figure out handing off the authentication to a separate login server (to be addressed in a separate ticket)
    • Proper DNS handling and other potential issues related with recycling the same subdomains need to be tested
  2. @selinad @wrightmw We need to communicate with curators/current account holders that we are switching from Persona to Google for our authentication needs. Previously, Persona allowed for any e-mails to be associated with their system, but Google is slightly more restrictive. The programmers also discussed the possibility of restrictions imposed by employers/organizations that will not allow the use of their professional emails via the Google system. In any case, if the email they used to log in to Persona cannot be used with Google, they will need to provide us with a Google-accessible email address they would like associated with the curation interface.
  3. Related to the above point, our alternate logins might no longer work. This means that we will need to update our records to use our personal emails or other Google accounts you create for the purpose of testing.

@jimmyzhen
Copy link
Contributor

Hi @mrmin123,

Your workaround for the failed jest test is good. I recall implementing a similar workaround when I was doing the react 14 upgrade.

Although I did other jest workarounds in the babel conversion, they are somewhat different.

In any case, I found some useful links talking about jest workarounds and manual mocks. They may (or may not) be helpful:
http://ryanlanciaux.github.io/blog/2014/10/28/miscellaneous-jest-issues-slash-workarounds/
http://facebook.github.io/jest/docs/manual-mocks.html

@selinad
Copy link

selinad commented Apr 16, 2016 •
edited
Loading

Hi @mrmin123 - just getting to this. Thank you for all your hard work.

I hope it is not a problem that some people will need a Google associated email address....I'm thinking about Geisinger and their restrictions, but we can ask.

On a similar vein, this message always alarms me, as I am always hesitant to share info
screen shot 2016-04-15 at 4 56 40 pm

I just tried with my selinad@stanford.edu address for the first time and it just said Bad Request, but I tried again and it worked - anyone else had that problem? I repeated it on Safari.

@mrmin123
Copy link
Contributor

@selinad This should no longer have the Bad Request issue: http://530-mc-google-auth-frontend-8dee0fe-minchoi.instance.clinicalgenome.org/

@mrmin123
Copy link
Contributor

mrmin123 commented Apr 19, 2016 •
edited
Loading

Some additional reading on the lack of wildcard subdomains for google auth, and potential workarounds:

The most commonly suggested course of action seems to be set up a separate static endpoint to handle the login requests. There are also some talks of using url rewrite rules but I have no idea how that would work in the context of our cloud deployment configuration.

@mrmin123
Copy link
Contributor

@jimmyzhen I just saw your message regarding the jest tests! Thanks for the links. I don't think I'll be actually mocking the google platform API to keep the tests independent of it. Unfortunately, that means I do have to fake the 'proper' response the test expects when gapi is not found. The downside to this is that if someone is using an adblocker/noscript and disabled Google APIs, they'll just see a normal webpage sans the Google login button, instead of a specific error message as I would like to. Letting the users know ahead of time that we are using Google authentication should hopefully alleviate issues with them, since we can let them know to whitelist the curation interface ahead of time. (I'm kind of rambling at this point but I'm writing it down to keep a record of why I incorporated things the way that I did)

@mrmin123
Copy link
Contributor

For the devs: the cleaned up branch for this is now 530_mc_google_auth

@kilodalton kilodalton added R7 and removed R6 labels Apr 26, 2016
@kilodalton kilodalton added R8 and removed R7 labels Oct 5, 2016
@mrmin123
Copy link
Contributor

mrmin123 commented Oct 6, 2016

Our user use case is a bit different so we may be leaving in Auth0's database as a valid sign-in route. This and the identity providers we'll be supporting will be discussed by the others in the group.

Current look of the login:

image

To-do:

  • Wire up rules to automatically send our admin account an email when someone registers
  • Implement a post-login page for users with accounts, but not verified and/or added to our database

@kilodalton
Copy link
Contributor Author

@mrmin123 Nice work on this! Thanks for modifying their login window with ClinGen branding. Looks awesome.

@wrightmw
Copy link
Member

wrightmw commented Oct 6, 2016

@mrmin123 Ooh, looks great!

@selinad
Copy link

selinad commented Oct 6, 2016

Agreed! Looks fantastic! 👍 thx @mrmin123 !

@mrmin123
Copy link
Contributor

New error screens while logging in.

If the user has not verified their email address with Auth0 via the automated email message they get (users must be both verified via Auth0 and added to our database by us manually to be able to log in):
image

If the user has verified their email address with Auth0, but they have not been added to the database manually by us:
image

Wording could use help......

@selinad
Copy link

selinad commented Oct 17, 2016

Thanks @mrmin123 !

How about this for the 2 pages:

Account not verified:

Once you have created an account with Auth0, you must verify it via email - please check your inbox for an email from Auth0 and verify it according to their instructions.

Additionally, the same email you use for Auth0 must be registered for use with the interfaces. If you have not yet registered your email for the ClinGen interfaces, please send an email to clingen-helpdesk@lists.stanford.edu, supplying the email you used for your Auth0 account and your preferred display name within the interfaces.

Please note that access to the ClinGen interfaces is currently restricted to ClinGen curators

Login Failure

In addition to creating an account with Auth0, your email must be registered with the ClinGen interfaces in order to log in to the interfaces. If you are encountering this error message, you have either not yet registered your email with us or we have not yet been able to verify and add your account to the system.

Currently, access is restricted to ClinGen curators. Please send us an email at clingen-helpdesk@lists.stanford.edu if you feel your email should be registered for use with the ClinGen interfaces.

@mrmin123
Copy link
Contributor

@selinad @wrightmw
There are 4 'test' instances for testing this branch now. The number of instances is mainly due to the new Auto-login feature...

https://530-mc-auth0-0478103-minchoi.demo.clinicalgenome.org/ - Demo Login button should be visible, and should work
https://530-mc-auth0-proddb.demo.clinicalgenome.org/ - Demo Login button should be visible, but should NOT work
http://530-mc-auth0-prodtest-testdb.production.clinicalgenome.org/ - Demo Login should not be visible
http://530-mc-auth0-prodtest-proddb.production.clinicalgenome.org/ - Demo Login should not be visible

In all instances, the standard login should work, except that instances #1 and #3 should have the test credentials, while instances #2 and #4 only have the production database credentials (no test users). Please try out all the steps including signing up for an account and reading the emails that get sent to you (we can change wording on them if needed).

I've also invited both of you to the #usersignup channel in Slack. You will see a message in there whenever a new user signs up.

@selinad
Copy link

selinad commented Oct 20, 2016

Hi @mrmin123 - wow, you've done a ton of great work on this!

The Demo seems to work great.

I might be a little confused on what the expected behavior is for the regular login in each of the instances. Could you write a brief summary so I can make sure it's as expected? Thank you!

p.s. one quick thing - when I go to Sign up on authentication part, it looks just like Login - could we add some very brief text?

@wrightmw
Copy link
Member

wrightmw commented Oct 20, 2016 •
edited
Loading

@mrmin123 All four links worked as anticipated... with demo login functionality as you described

For standard logins.
Logging in with wrightmw@stanford.edu gave access to all instances #1 through to #4

Logging in with alwayswright@stanford.edu gave me access to instances #1 and #3 only

I assume this is as expected? Does wrightmw email have production editing rights but alwayswright only test rights?

@mrmin123
Copy link
Contributor

@selinad Sorry, your primary admin account you should have for the production server should work on all instances, while the secondary non-admin account you have for testing should ONLY work on instances #1 and #3. @wrightmw you are right with your assumptions regarding the logins.

The sign up (as opposed to sign-in) is done via these steps:

  1. Click the Login button
  2. If you see the Sign Up tab, click it and sign up, otherwise click the 'This is not my account' button, then press the Sign up tab then sign up

If you could also confirm that the auto-login function works as intended that'd be great as well.

kilodalton added a commit that referenced this issue Oct 24, 2016
@kilodalton
Merge pull request #1081 from ClinGen/530_mc_auth0
b8bc45a 
Connects to #530. Auth0 implementation. Auto-login support
@kilodalton kilodalton mentioned this issue Oct 24, 2016
Merged
@kilodalton kilodalton added QA ready and removed merged labels Nov 4, 2016
@kgliu0101
Copy link
Contributor

Auth0 functions properly to log in https://r8-rc1.demo.clinicalgenome.org/dashboard/ in Chrome (version 54.0), FF (version 49.1), Safari (version 10.0) in mac.

@kilodalton
Copy link
Contributor Author

Included in last release (R8). Nice job and thanks for your hard work.

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

@mrmin123 mrmin123

Labels
backend feature priority: high
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@Bek @kilodalton @mrmin123 @jimmyzhen @kgliu0101 @selinad @wrightmw