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

Documentation: github org redirect caveat #1019

Merged
merged 1 commit into from
Aug 11, 2017

Conversation

estroz
Copy link
Contributor

@estroz estroz commented Aug 8, 2017

This PR updates GitHub connector docs to notify users of our current workaround relating to #920. Part of the issue, users outside of the organization being able to log into dex-authenticated apps, was solved by #1013.

Test cases for the above PR:

  1. User not in a config org: fails
  2. User in a config org but not in a config team: fails
  3. User in a config org with no config teams: passes
  4. User in a config org with no config teams, but already granted the app permission: fails <- issue
  5. User in a config org and in a config team: passes

@ericchiang
Copy link
Contributor

User in an org but not in any teams: fails

This should be, passes, right?

Also this diff conflicts with #1018. Let's get that merged first.

@@ -8,7 +8,8 @@ When a client redeems a refresh token through dex, dex will re-query GitHub to u

## Caveats

* Please note that in order for a user to be authenticated via GitHub, the user needs to mark their email id as public on GitHub. This will enable the API to return the user's email to Dex.
* A user added to a GitHub organization __after__ they have granted an application access their GitHub information via dex will not be redirected to a 'request organization access' page. This issue arises because GitHub's OAuth redirect URI does not return a 'request organization access' page. The current workaround is as follows: the user should log into their GitHub account, go to Settings -> Authorized OAuth Apps, click 'Revoke' to revoke the application's grant, and restart the dex login process. Dex will issue a new grant to the application with proper organization permissions.
Copy link
Contributor

@rithujohn191 rithujohn191 Aug 8, 2017

Choose a reason for hiding this comment

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

nit: they have granted an application access to their GitHub....

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should add an introductory line stating that "the github api call requires the user to have their org membership to be public. this can be done by requesting an org for access....." something along those line

Copy link
Contributor

Choose a reason for hiding this comment

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

Also the last line "Dex will issue a new grant to the application with proper organization permissions." is not correct. Once the app is revoked, when the user tries to login via GitHub....Github explicitly asks the user to grant permission...no permissions are automatically assigned.

@estroz
Copy link
Contributor Author

estroz commented Aug 8, 2017

@ericchiang updated test case descriptions to be more specific. If a user is in an org but not in any of the teams listed in the config, they should not get access.

@estroz estroz force-pushed the doc-updates branch 3 times, most recently from a6ce241 to 8a844d8 Compare August 11, 2017 01:01
@@ -6,6 +6,11 @@ One of the login options for dex uses the GitHub OAuth2 flow to identify the end

When a client redeems a refresh token through dex, dex will re-query GitHub to update user information in the ID Token. To do this, __dex stores a readonly GitHub access token in its backing datastore.__ Users that reject dex's access through GitHub will also revoke all dex clients which authenticated them through GitHub.

## Caveats

* The GitHub API calls dex uses requires a user to have their organization membership to be public. This can be done by [requesting access from an organization](github-request-org-access). A user added to a GitHub organization __after__ they have granted an application access to their GitHub information via dex will not be redirected to a 'request organization access' page. This issue arises because GitHub's OAuth redirect URI does not return a 'request organization access' page. The current workaround is as follows: the user should log into their GitHub account, go to Settings -> Authorized OAuth Apps, click 'Revoke' to revoke the application's grant, and restart the dex login process. Dex will request the user give the application permission to access their information with proper organization permissions.
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets replace " This can be done by requesting access from an organization. A user added to a GitHub organization after they have granted an application access to their GitHub information via dex will not be redirected to a 'request organization access' page. This issue arises because GitHub's OAuth redirect URI does not return a 'request organization access' page. "

with " This can be done on the "request access from org" page which github will skip if client app is an existing authorized application."

Replace "Dex will request the user give the application permission to access their information with proper organization permissions."

With "This will force the "request access from org" page to be shown, allowing the user to request the organization owner to make the membership public"

Copy link
Contributor

@rithujohn191 rithujohn191 left a comment

Choose a reason for hiding this comment

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

LGTM

@rithujohn191 rithujohn191 merged commit e361bc6 into dexidp:master Aug 11, 2017
mmrath pushed a commit to mmrath/dex that referenced this pull request Sep 2, 2019
Documentation: github org redirect caveat
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.

3 participants