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

GitHub Desktop authentication primarily through browser flow #9231

Closed
billygriffin opened this issue Mar 6, 2020 · 11 comments · Fixed by #10020
Closed

GitHub Desktop authentication primarily through browser flow #9231

billygriffin opened this issue Mar 6, 2020 · 11 comments · Fixed by #10020

Comments

@billygriffin
Copy link
Contributor

GitHub is deprecating and removing the authorizations API, which handles how most users of GitHub Desktop sign into GitHub from the app. We've always had a secondary method of authenticating via the browser, but we'll need to make this the main authentication method going forward.

Considerations

  1. We know protocol handlers can be brittle depending on browser and OS, so we need to ensure people know that the sign in was successful and that they can return to the app. We're imagining this as an interstitial after a successful sign in on github.com.
  2. To help discover potential pitfalls with moving to 100% browser authentication, we might start by swapping the prominent default initially so people have something to fall back to before we're forced to remove it altogether.
  3. We need to make sure the next version of GitHub Enterprise uses both our protocol handlers (including the auth-specific one) so we get backward compatibility on Enterprise.
@steven351

This comment has been minimized.

@ampinsk
Copy link
Contributor

ampinsk commented Mar 24, 2020

Designs

Transition state

For the period of time when we're transitioning, we'll need to emphasize the browser log in flow, while still keeping the normal login flow

Welcome screen

This moves all the login options to the first screen, and emphasizes the GitHub.com option.

Transition-Welcome-Screen

Preferences

The initial login screen should stay the same:

Transition-Preferences-1

Then on this screen, I flipped the options and emphasized the GitHub.com option.

Transition-Preferences-2

Final state

For when the transition is complete, and the only option is to login using your browser

Welcome screen

After the transition is complete, we can remove the login fields

Final-Welcome-Screen

Preferences

After the transition is complete, we can just surface the GitHub.com option directly on this screen.

Final-Preferences

Interstitial page

The page on dotcom we send you to for the browser login. I left these Help links just as an idea based on other login flows I looked at, so we can change these or remove them entirely if they're not needed.

Interstitial Page

@Rexogamer

This comment has been minimized.

@billygriffin billygriffin added this to Backlog (unprioritized) in Desktop 2.5 release via automation Mar 25, 2020
@billygriffin billygriffin moved this from Backlog (unprioritized) to Prioritized backlog (ordered) in Desktop 2.5 release Apr 6, 2020
@rafeca rafeca self-assigned this Apr 7, 2020
@rafeca
Copy link
Contributor

rafeca commented Apr 7, 2020

I'm going to work on this one, by first implementing the transition state designed by @ampinsk

@rafeca rafeca moved this from Prioritized backlog (ordered) to In Progress Issues in Desktop 2.5 release Apr 7, 2020
@billygriffin billygriffin added this to Backlog (unprioritized) in Desktop 2.5.x releases via automation May 8, 2020
@billygriffin billygriffin removed this from In Progress Issues in Desktop 2.5 release May 8, 2020
@billygriffin
Copy link
Contributor Author

I shared with @niik today that ideally we would keep the alternative method available until it's actually deprecated so we can use the brownout periods to identify any problems and address them.

He shared that ideally we would add support for detecting when dotcom doesn’t support basic auth any more and dynamically enable/disable the username/password flow based on that. That way we’ll adapt during the brownouts and Desktop will automatically adapt to any changes in the deprecation timeline.

This sounds like a great approach to me assuming the implementation isn't too complex.

@ptoomey3 or @tarebyte: We use the verifiable_password_authentication property on the API for GHES to detect whether it supports basic auth. Do you happen to know if we can reliably use that for dotcom as well? Note: this is a public repo so share public things only here. 😄

@niik Please feel free to add or correct anything I missed, and if anyone else has questions please don't hesitate to ask.

@ptoomey3
Copy link

ptoomey3 commented Jun 2, 2020

TIL verifiable_password_authentication. I think that value would also apply to whether the main web login uses built-in auth or not (I think it is just meant to say that the app verifies passwords vs. deferring to an external source using SAML or something). As such, I'm not sure its value would change after password auth support is removed.

@billygriffin billygriffin moved this from Backlog (unprioritized) to Prioritized backlog (ordered) in Desktop 2.5.x releases Jun 8, 2020
@niik
Copy link
Member

niik commented Jun 8, 2020

I think it is just meant to say that the app verifies passwords vs. deferring to an external source using SAML or something

@ptoomey3 It's possible that the meaning of that property have changed over time but it was originally added specifically for GitHub Desktop (or rather GitHub for Mac/GitHub for Windows) to be able to determine whether we'd have to use the web flow to authenticate.

GHfW/GHfM and GHD have relied on it since 2013 but only for GHES. Our API documentation for the property says

Whether authentication with username and password is supported.

@ptoomey3
Copy link

ptoomey3 commented Jun 8, 2020

👍 - I think the bit that felt ambiguous to me was the "Whether authentication with username and password is supported." documentation. It isn't clear authentication to what? One might think "to the API" is obvious/implicit, but wasn't 100% sure since that meta endpoint returns other data totally unrelated to the API itself (ex. SSH server key fingerprints).

@rafeca rafeca moved this from Prioritized backlog (ordered) to In Progress Issues in Desktop 2.5.x releases Jun 11, 2020
@niik niik self-assigned this Jun 16, 2020
Desktop 2.5.x releases automation moved this from In Progress Issues to Done! Jun 17, 2020
@niik
Copy link
Member

niik commented Jun 18, 2020

@ptoomey3 Yeah, I think ambiguous is an understatement here and if I hadn't been there for the discussions to add it way back when I wouldn't have been able to make heads or tails of it either.

We've just merged #10020 which makes Desktop respect verifiable_password_authentication for GitHub.com as well as GHES. Is there a tracking issue for the brownouts or another good place where we might add a task so that we remember to flip the property as part of the brownouts?

@benbalter
Copy link

benbalter commented Jul 14, 2020

Based on #10020 would it be accurate to say that users using basic password auth should upgrade to GitHub Desktop 2.5.4 or later?

@rafeca
Copy link
Contributor

rafeca commented Jul 15, 2020

Based on #10020 would it be accurate to say that users using basic password auth should upgrade to GitHub Desktop 2.5.4 or later?

Yes, starting in v2.5.4 GitHub Desktop will disable the user/password authentication once GitHub doesn't support it anymore (note that GitHub Desktop has supported browser-based authentication for quite a long time, but the main way users authenticated was still using user/password).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

10 participants