Skip to content
This repository has been archived by the owner on Mar 5, 2020. It is now read-only.

Support OAuth2 (take 2) #626

Merged
merged 7 commits into from
Apr 16, 2015
Merged

Support OAuth2 (take 2) #626

merged 7 commits into from
Apr 16, 2015

Conversation

toolness
Copy link
Contributor

@toolness toolness commented Apr 9, 2015

This is a different attempt at supporting OAuth2 (#576). A few distinctions from the previous attempt in #608:

  • This one removes support for Persona-based login. There's just such a big difference in the way that the two auth systems work that it's easier to substitute one out for the other wholesale, and migrate to it once the new auth is stable.
  • This one adds LoginLink and LogoutLink components that make it easier to add login/logout links from anywhere on the site. The links also work via standard href attributes rather than onClick handlers, since the new OAuth2 system just works by redirecting the browser to new pages.

Also, the login widget has changed so that it now looks like this when the page first loads:

2015-04-09_17-49-51

Typically this message will only be shown for a second or two. If the login server is successfully contacted, the usual content appears depending on whether the user is logged in or not.

However, if an error occurs contacting the login server, the following appears:

2015-04-09_17-51-08

Still need to do:

  • Remove Persona-related functionality.
  • Add unit tests for LoginLink.
  • Add unit tests for LogoutLink.
  • Add unit tests for TeachAPI::checkLoginStatus().
  • Make the login:error event trigger something other than a window.alert(), since it can now be triggered on page load and we don't want a modal to appear at page load.
  • Figure out how we want to deal with google analytics here. (punted to Google Analytics for new oAuth2 Flow #656)

@toolness
Copy link
Contributor Author

toolness commented Apr 9, 2015

Hmm, I also just noticed that if you use the "back" button after you've been redirected to login, in some browsers the page is just shown again instead of fully reloaded. In this case the login widget is stuck at "Logging in...".

Er actually, that's what happens with #608, I'm not sure if it's what happens with this PR yet.

(Checked, nope, this doesn't happen on this PR.)

@toolness
Copy link
Contributor Author

toolness commented Apr 9, 2015

Ok, I think this is ready for review, but we should not merge it until we've decided to officially switch from Persona to the new OAuth2 server. @ScottDowne, can you take a look but not merge it?

@toolness
Copy link
Contributor Author

toolness commented Apr 9, 2015

I've temporarily deployed this branch to http://mozteach.toolness.org/ if anyone wants to try it out.

@toolness
Copy link
Contributor Author

toolness commented Apr 9, 2015

Hey @adamlofting, any ideas on how to approach the switch to the new login mechanism through google analytics? As you can see in this PR, I've removed a number of ga.event() calls. A lot of them aren't relevant anymore, but I want to make sure we're still able to track metrics on people who click "log in" or "create account" but don't make it back to the site, and so on.

One thing I'm now realizing with the flow outlined in #576 is that we're redirecting users from teach.mozilla.org to teach-api (domain TBD), which then redirects them to id.webmaker.org. The process goes in reverse when the user has finished logging in or signing up. And I'm not sure if there's an easy way to track that path through GA, which concerns me.

@ScottDowne
Copy link
Contributor

There is a new test failing, r- because of that.

I'm also surprised it still uses the persona UI? Is that intended?

@toolness
Copy link
Contributor Author

Er, which test is failing? Travis CI doesn't seem to think any tests fail, and when I checked before submitting the PR, nothing seemed to be failing...

Where are you seeing persona? There should definitely not be any traces of persona left in the codebase in this PR, so I should remove any that I missed.

@ScottDowne
Copy link
Contributor

Yeah, this isn't the first time I had weirdness with teach login locally.

I think something must be up with my setup for teach api?

@ScottDowne
Copy link
Contributor

Let's get this up on staging and test there.

No point in debugging a local setup for a login system that's in flux.

@toolness
Copy link
Contributor Author

Oh, weird... Ok, as soon as id.webmaker.org is ready we can merge this then!

alicoding added a commit that referenced this pull request Apr 16, 2015
@alicoding alicoding merged commit 19fb89c into mozilla:develop Apr 16, 2015
@secretrobotron
Copy link

👍 time to start testing this. id is in a good enough place now on staging.

@hannahkane
Copy link

So many excites! Just did the migration flow and the login flow, and both worked beautifully.

@secretrobotron
Copy link

🌳 !!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants