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

Single Sign On Support. #1373

Merged
merged 56 commits into from Jan 25, 2018

Conversation

Projects
6 participants
@grokys
Contributor

grokys commented Dec 11, 2017

This PR implements SSO/OAuth support for both GitHub.com and Enterprise instances.

DotCom

When the user goes to login, the GitHub tab now shows an option to log in with a browser:

image

When clicked the user's system browser will be opened, and the user logged in.

Enterprise

Note: Enterprise support will not be available until version 2.12.1. To enable support for SSO on enterprise, we first need to check the entered URL to see if OAuth/Username/Password is supported. This results in a slightly different flow for enterprise:

2017-12-11_12-34-47

Here you can see that because 2.12.1 isn't yet out, we're not enabling SSO, but we do detect that username/password login isn't supported and instead prompt the user to enter a token. We'll only be able to test SSO support on Enterprise when 2.12.1 is out.

Fixes #1323
Fixes #977

grokys added some commits Nov 14, 2017

Cancel OAuth listener if necessary.
Cancel OAuth listener when login dialog is closed, or the user switches between .com and enterprise while waiting for an OAuth callback.
Continue listening for oauth response.
Don't error our on first non-matching response, keep listening until the correct response is receved or the operation is cancelled (i.e. the login dialog is closed).
Check scopes on login.
When performing a login, check that the scopes returned are what we expect, otherwise throw an exception causing a new login to be required.
Merge branch 'fixes/1332-validate-scopes' into feature/sso
 Conflicts:
	src/DesignTimeStyleHelper/App.config
	src/GitHub.Api/ApiClientConfiguration.cs
	src/GitHub.Api/LoginManager.cs
	src/GitHub.StartPage/app.config
	src/GitHub.VisualStudio/app.config
	test/UnitTests/GitHub.Api/LoginManagerTests.cs
Allow logging back in when scopes incorrect.
- Make sure the appropriate login tab shows when there's an errored connection
- Make sure an existing connection is removed before trying to log in
- Fix exception messages in ConnectionManager (HostAddress doesn't override `ToString`)
Don't show TE section when login failed.
When a login failed, don't show the connection in Team Explorer, instead show the same view as presented when the user has no connections.

We should really be displaying an error or something, but that will need to be a separate change.
Fix tests.
Something went wrong with a merge...
Remove EnterpriseProbe(Task).
These classes are now present in Octokit, so simply export one of these.
Update enterprise login dialog.
- Move URL to be first field and hide the other fields until a valid enterprise URL is entered
- Don't show username/password if not supported by enterprise instance
Merge branch 'master' into feature/sso
 Conflicts:
	src/DesignTimeStyleHelper/App.xaml.cs
	src/DesignTimeStyleHelper/DesignTimeStyleHelper.csproj
	src/DesignTimeStyleHelper/packages.config
	src/GitHub.App/ViewModels/Dialog/LoginCredentialsViewModel.cs
	src/GitHub.App/ViewModels/Dialog/LoginTabViewModel.cs
	src/GitHub.Exports.Reactive/Extensions/ConnectionManagerExtensions.cs
	src/GitHub.VisualStudio/UI/WindowController.xaml
	src/GitHub.VisualStudio/Views/Dialog/LoginCredentialsView.xaml.cs
Fix enterprise login.
Was throwing a `NullReferenceException` after recent merge.
Display progress of enterprise probe.
Added an `IconContent` property to `PromptTextBox` to display an icon on the right-hand-side of the textbox. In this area, display the status of the enterprise probe when trying to log in.
WIP: Check allowed login methods of enterprise instance
Unfortunately my idea of checking whether the `redirect_url` is correctly set by sending a request with the required `redirect_url` with `HttpClient` and listening for an error if it doesn't match that on the server doesn't seem to work.
Remove Rothko submodule.
Usean updated Rothko nuget package. Also had to add an export wrapper as I prefer to define the export in `github/VisualStudio` rather than in our Rothko fork.
Update rotko.
`HttpListenerWrapper` is now non-sealed so we don't need to wrap it to export it.
Allow signing into enterprise with token.
Also added some unit tests for enterprise login view model.
@meaghanlewis

This comment has been minimized.

Contributor

meaghanlewis commented Jan 2, 2018

Good point @grokys. Perhaps allow the user to enter the token as well and not force them to use SSO similarly to how sign in with GitHub offers both options.

grokys added some commits Jan 4, 2018

Correctly display token panel.
Display token panel when the bit is set in `SupportedLoginMethods` alongside other bits.
Updated enterprise OAuth version.
Won't be able to use OAuth with enterprise instances until 2.12.2.
@donokuda

This comment has been minimized.

Member

donokuda commented Jan 9, 2018

It looks like I have to specify the protocol being used (e.g., https://) in order to kick off this flow. If I don't, nothing happens as a result, and it feels like something is broken even though it's waiting for me to input a valid URL.

Would it makes sense to turn this into a multi-step process where the user has to explicitly submit information for each step? That way, if the protocol is omitted, then we could still try to use what's supplied.

The "or" text probably shouldn't appear when there's only one choice:

👍 Agree. We could consider replacing the "Sign in" button with a "Sign in with your browser" submit button.

sso

For 2-1, we might want to consider guiding the user on where to find the token to log in.

Polish login view
* Move sign-in action button closer to login fields
* Adjust lines between "or" text to a lighter gray
@donokuda

This comment has been minimized.

Member

donokuda commented Jan 9, 2018

Just a heads up, there's another login modal when you try to push to a repo on GitHub and you're not logged in:

grokys added some commits Jan 11, 2018

Fix enterprise logins.
- Ported octokit.net#1726 to our fork
- Ensure we're not trying to access the `meta` endpoint with invalid credentials
Allow enterprise urls without a scheme.
Don't insist on `https://enterprise.me`, allow `enterprise.me`. This defaults to `http` rather than `https` but there should be a redirect in place.
@grokys

This comment has been minimized.

Contributor

grokys commented Jan 11, 2018

@donokuda:

It looks like I have to specify the protocol being used (e.g., https://) in order to kick off this flow.

I've tweaked the logic to not need https://, hopefully that's better.

We could consider replacing the "Sign in" button with a "Sign in with your browser" submit button.

There won't actually ever only be one choice now: there's always going to be either user/pass or token sign in available for enterprise.

Just a heads up, there's another login modal when you try to push to a repo on GitHub and you're not logged in:

Yeah, this isn't actually us... Not sure what we can do about this. Might be worth a separate issue.

@meaghanlewis

This comment has been minimized.

Contributor

meaghanlewis commented Jan 11, 2018

@grokys nice job getting this working with enterprise it's working great! 🎉

Just one thing to mention. When I sign in incorrectly using https://ghe.io, I see bad credentials displayed in the dialog, and in the log I get the error:
(2018-01-11 16:48:14.951 INFO [01] LoginTabViewModel Error logging into '"https://ghe.io/"' as '' Octokit.AuthorizationException: Bad credentials)

Signing in to enterprise incorrectly using just ghe.iodoesn't give an error in the dialog or an error in the log. This is unexpected.

@grokys grokys added this to In progress in 2.4.0 Jan 16, 2018

meaghanlewis and others added some commits Jan 19, 2018

Allow BaseUris without scheme.
We now allow the enterprise URL to be entered without a scheme, but `new Uri(string)` requires a scheme. Use `new UriBuilder(string).Uri` instead which assumes http.
@grokys

This comment has been minimized.

Contributor

grokys commented Jan 23, 2018

@meaghanlewis good catch! That should now be fixed.

@meaghanlewis

This comment has been minimized.

Contributor

meaghanlewis commented Jan 23, 2018

thanks @grokys this looks great to me 👍

@grokys grokys added the feature label Jan 24, 2018

@shana

This comment has been minimized.

Collaborator

shana commented Jan 25, 2018

This needs to be tested against a GitHub for Business team with different authentication settings - GitHub for Business is hosted by us but allows enterprises to do their own authentication, including SSO, but I'm not sure that requires them to have a separate domain for the org/repos - which means that our assumption that github.com logins are always oauth may be wrong and we may be locking out GitHub for Business users.

@shana

This comment has been minimized.

Collaborator

shana commented Jan 25, 2018

LoginManager is currently being created as a service, but it doesn't have to be, since it's not required during package initialization. The 2fa handler setup that's being done in https://github.com/github/VisualStudio/pull/1373/files#diff-0cd7c470cbea5722f66d871d121f1d3bR221 should be happening in the LoginManager constructor so that instances initialized via MEF are valid (as it stands currently, they're not).

I don't think ITwoFactorChallengeHandler needs to be created lazily, it's a MEF shared instance and the login code sets the viewmodel accordingly when it needs to. We can just import it in the constructor like we do every other dependency so MEF creates it, and we don't have to worry about threading nor have special initialization code for this.

@shana

shana approved these changes Jan 25, 2018

👍 🎉 🚀

@shana shana merged commit 352d551 into master Jan 25, 2018

3 checks passed

GitHub CLA @grokys has accepted the GitHub Contributor License Agreement.
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

2.4.0 automation moved this from In progress to Done Jan 25, 2018

@shana shana deleted the feature/sso branch Jan 25, 2018

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