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

SAML re-authorization dialog #8910

Merged
merged 17 commits into from
Jan 15, 2020
Merged

SAML re-authorization dialog #8910

merged 17 commits into from
Jan 15, 2020

Conversation

niik
Copy link
Member

@niik niik commented Jan 13, 2020

Description

This introduces special case handling of a particular set of access denied errors related to SAML and SSO. When a user is signed in but their token hasn't yet been authorized for a particular SAML organization fetches, pushes, and pulls will fail with a specific error message from the server informing the user that they need to re-authorize (sign out and back in again).

Screenshots

Before

Screen Shot 2020-01-08 at 14 52 58

After

image

Flow

(null) 2020-01-13 18_53_26

Testing notes (requires membership in a SAML org)

  1. In your primary browser, sign out of GitHub.com completely
  2. In GitHub Desktop, sign out of your GitHub.com account.
  3. Sign in using the web flow in GitHub Desktop but do not authorize the SAML org

image

4. Attempt to fetch, push, or pull a private repository owned by your SAML org

Expected result is a dialog that allows you to sign in again using the Web flow where you have the ability to authorize the SAML org.

Release notes

Notes:

@niik niik added this to In Progress PRs in Desktop 2.3 release via automation Jan 13, 2020
@niik niik moved this from In Progress PRs to Awaiting Review in Desktop 2.3 release Jan 13, 2020
@tierninho tierninho self-requested a review January 13, 2020 20:46
Copy link
Contributor

@tierninho tierninho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing notes:

  • Overall this works well and will hopefully help with some of the auth issues coming through 👍
  • I can foresee some minor confusion with "authorize" and "continue" on the SSO page. We do not explicitly say you have to click "authorize", just "grant access". I think most will get it though. If the user clicks continue, we surface a similar warning but only in the console logs:
install.ts:27 fetchCommit: returned an error 'github/quality@de8bc7a12fc8208e9e21ff47777bee9e13905d26'
Error: Resource protected by organization SAML enforcement. You must grant your personal token access to this organization.

No strong feels on doing anything to change the current fix, just wanted to point it out.

Copy link
Contributor

@kuychaco kuychaco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks great! Thanks @niik ✨. And thanks @tierninho for testing 🙏

@kuychaco kuychaco moved this from Awaiting Review to Review In Progress in Desktop 2.3 release Jan 14, 2020
@kuychaco kuychaco merged commit 32ee147 into development Jan 15, 2020
Desktop 2.3 release automation moved this from Review In Progress to PRs For Next Beta Jan 15, 2020
@kuychaco kuychaco deleted the saml-reauth-dialog branch January 15, 2020 00:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Desktop 2.3 release
PRs For Next Beta
Development

Successfully merging this pull request may close these issues.

None yet

3 participants