Single Sign On Support. #1373
Single Sign On Support. #1373
Conversation
When using SSO.
Cancel OAuth listener when login dialog is closed, or the user switches between .com and enterprise while waiting for an OAuth callback.
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).
When performing a login, check that the scopes returned are what we expect, otherwise throw an exception causing a new login to be required.
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
- 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`)
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.
Something went wrong with a merge...
These classes are now present in Octokit, so simply export one of these.
- 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
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
Was throwing a `NullReferenceException` after recent merge.
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.
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.
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.
`HttpListenerWrapper` is now non-sealed so we don't need to wrap it to export it.
Also added some unit tests for enterprise login view model.
|
@meaghanlewis it looks like ghe.io has been updated to 2.12.1 but my PR to set the callback URL hasn't been included. I'll get in touch with the enterprise folks. |
|
The "or" text probably shouldn't appear when there's only one choice:
Or should we still allow the user to enter a token as well? |
|
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. |
Display token panel when the bit is set in `SupportedLoginMethods` alongside other bits.
Won't be able to use OAuth with enterprise instances until 2.12.2.
|
It looks like I have to specify the protocol being used (e.g., 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.
👍 Agree. We could consider replacing the "Sign in" button with a "Sign in with your browser" submit button.
For 2-1, we might want to consider guiding the user on where to find the token to log in. |
* Move sign-in action button closer to login fields * Adjust lines between "or" text to a lighter gray
|
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:
|
- Ported octokit.net#1726 to our fork - Ensure we're not trying to access the `meta` endpoint with invalid credentials
Polish login view
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.
I've tweaked the logic to not need
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.
Yeah, this isn't actually us... Not sure what we can do about this. Might be worth a separate issue. |
|
@grokys nice job getting this working with enterprise it's working great! 🎉 Just one thing to mention. When I sign in incorrectly using Signing in to enterprise incorrectly using just |
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.
|
@meaghanlewis good catch! That should now be fixed. |
|
thanks @grokys this looks great to me 👍 |
|
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. |
|
I don't think |
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:
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:
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