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

Extend Github Authentication to support seamless SSO #209482

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

imarban
Copy link

@imarban imarban commented Apr 3, 2024

What does this do?

Updates the Device Code and URL sign-in flows to support specifying a SSO Tenant ID in Settings.

If specified, the sign in flow redirects to https://github.com/enterprises/<id>/sso before sending back the user to the Oauth or Device Login flows.

The logic validates that only one of URI and SSO ID are set during extension activation time. It also intends to keep the current behavior by giving priority to the URI setting if set.

Why is this needed?

This is useful as it allows organizations that manage settings for users to redirect them to the Enterprise SSO login flow.
This way organizations can somewhat ensure that users sign in with their enterprise Github account when connecting from managed environments and limit their chances of using a personal account.

How to test?

Package the extension and verify that:

  • User is able to set the new setting
  • Setting both Enterprise URI and Enterprise SSO ID throws an error
  • If Enterprise SSO ID is set then user is redirected to the SSO login page
    • User is able to login in the browser
    • User is redirected back to VS Code
    • Authentication extensions gets a valid session
  • If Enterprise URI is set then the current behavior is preserved

@imarban
Copy link
Author

imarban commented Apr 3, 2024

@imarban please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree company="Uber Technologies Inc"

@imarban imarban force-pushed the remote-branch-enterprise branch from 5f73272 to 4fa8746 Compare April 5, 2024 05:27
@imarban imarban marked this pull request as ready for review April 5, 2024 05:27
@TylerLeonhardt
Copy link
Member

Hi @imarban, is this flow documented somewhere so I can learn more about it?

@imarban
Copy link
Author

imarban commented Apr 5, 2024

Hi @imarban, is this flow documented somewhere so I can learn more about it?

Hey @TylerLeonhardt , these Github docs explain the URL and how SAML SSO works.

@TylerLeonhardt
Copy link
Member

I think the ergonomics of this are a little rough. We're asking the user to configure a setting ahead of time, so that when they sign in, they are taken through the SAML flow. Couple issues:

  1. discoverability... settings are not always found, so really only a few users will find themselves actually configuring this and using this feature. That to me, already doesn't seem worth it considering the alternative is that the user authorizes during the OAuth flow:
image
  1. If this is to prevent users from messing up during the OAuth flow, then to me, I think what would be better is to put the power in the hands of the extensions handling said error. If an extension, like GitHub Pull Request, hits a SAML related issue, then we could handle it then. I'm thinking we allow another scope like sso:ORG_NAME and if this scope is present, then we do what you have here and take the user through that SAML flow.

GitHub PR would first ask for:

vscode.authentication.getSession('github', ['repo'], { createIfNone: true });

then they'll realize that this particular user needs SAML for a particular org when they fetch a resource... at this point they can ask:

vscode.authentication.getSession('github', ['repo', 'sso:ORG_NAME'], { forceNewSession: true });

which will then force them through the flow again, this time we will throw away this fake scope and use it to mean "take me through the SSO flow for that org.

@imarban
Copy link
Author

imarban commented Apr 5, 2024

I agree, ergonomics are rough if we have users to configure the setting on their own. Although, I would argue that's the case too for the github-enterprise.uri setting that we already have.

That being said, these enterprise specific settings will likely be managed by the organization directly without users even knowing. The intention for this new setting (similar to the existing one) is that users get redirected to the sign-in page that is more appropriate for their managed development environment.

While I agree that extensions may be able to take care of this, I can see benefits on having this flow centralized into a single place and avoid having to replicate this across multiple extensions (some of them which are closed source e.g. Github Copilot).

@imarban imarban changed the title Extend Github Authentication to support seamless SSO sign-in Extend Github Authentication to support seamless SSO Apr 5, 2024
@TylerLeonhardt
Copy link
Member

The github-enterprise.uri comparison doesn't hold up because that setting is required to be set to get any functionality. A GHES user will undoubtably ask the question "how do I use this with GHES" and figure it out.

While this setting you're suggesting isn't really required and so the user doesn't know they could improve the flow slightly by setting a setting.

Additionally, it's important to note that this isn't great as a user setting, because you don't need SAML all the time. If I got a machine that I only worked on open source outside of my organization, I don't need to SAML for any org and do my job just fine.

I would rather an extension tell the GH Auth Extension what it requires, and then take the user through that should it be necessary.

The annoying part in this, that I blame GitHub for, is that we have a chicken and egg problem. You need Auth to see if a user needs SAML... but you whether you need SAML is contingent on who is signed in... and we can only do SAML ahead of time.

@imarban
Copy link
Author

imarban commented Apr 12, 2024

Hey Tyler,

The github-enterprise.uri comparison doesn't hold up because that setting is required to be set to get any functionality.

Wouldn't it be the same for this setting? Given that github-enterprise.uri and github-enterprise.sso-id are only relevant when using the Github Enterprise auth provider, a user of Enterprise would find both uri or sso-id when trying to configure extensions.

Additionally, it's important to note that this isn't great as a user setting, because you don't need SAML all the time. If I got a machine that I only worked on open source outside of my organization, I don't need to SAML for any org and do my job just fine.

I do agree on this and I see the setting more of a Managed User Setting. Let me just be more transparent on what we are intending to use this for at Uber: We are hoping to set the setting to uber for our development environments. These environments can be considered Uber's internal version of Codespaces and similar to GitHub Codespaces, we aim to reduce the likelihood of environment-related problems and just give our engineers a ready-to-use environment tailored to Uber's internal repositories.

I do see benefits on having a configuration like this available for companies managing development environments either internally or through Codespaces.

@JamyDev
Copy link

JamyDev commented Apr 12, 2024

While this setting you're suggesting isn't really required and so the user doesn't know they could improve the flow slightly by setting a setting.

What would your recommendation be for settings that are specified by the organization if not a user setting? Environment variables? What if another organization runs into this issue, the setting is very discoverable, and this is generally how we configure other aspects of the IDE.

@TylerLeonhardt
Copy link
Member

Hi sorry for the delay, last week I was at a conference and wasn't around much.

Wouldn't it be the same for this setting? Given that github-enterprise.uri and github-enterprise.sso-id are only relevant when using the Github Enterprise auth provider, a user of Enterprise would find both uri or sso-id when trying to configure extensions.

Maybe we're not on the same page...

A GitHub Enterprise Server user has to configure github-enterprise.uri or else they will not be able to log in with their GitHub Enterprise Server account. Because they have to do it, they will undoubtably figure out how to do it... and extensions like GH Pull Request actually update this setting for the user so they don't have to think about it at all, but without that setting, they can't log in to a GitHub Enterprise Server account at all.

A SSO user, which is on GitHub.com, can still log in just fine, they just need to ensure that they Authorize the org that is important to whatever task they are about to do and they're prompted to do so in the auth flow today:
image

Thus, the setting you're proposing isn't required to complete the auth flow, unlike the .uri setting. This would be just to have a better guarantee that the user authorizes the right org in the flow.

Am I missing something that makes what you're suggesting absolutely required for the user to do in order to log in?

The user setting you are proposing isn't really intended for users to set but rather enterprise management so that you can influence how VS Code logs in to GitHub.com accounts. Right?

@shirhatti
Copy link

Hi @TylerLeonhardt!

Revisiting this thread in the hope of convincing you otherwise in light of the new improved support for multiple accounts.

A SSO user, which is on GitHub.com, can still log in just fine

Unfortunately this isn't always straightforward if the same email address is associated with both accounts. There's indeterminate behavior on which account gets used.

And developers may not know their GitHub EMU username to login using that. Due to GitHub's special casing of the _ character (among other characters) for use a suffix to identify EMU users, there's a translation in usernames for folks who organization uses an IdP that allows for these characters in usernames. In Uber's case, we allow . in usernames but GitHub does not. Most of these classes of concerns are easily solved if we can specify enterprise the sso id apriori and redirect users accordingly.

The user setting you are proposing isn't really intended for users to set but rather enterprise management so that you can influence how VS Code logs in to GitHub.com accounts. Right?

Yes.

@vivodi
Copy link

vivodi commented Dec 29, 2024

Is this still relevant?

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

Successfully merging this pull request may close these issues.

5 participants