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

Github connector now returns a full group list when no org is specified #1184

Closed
wants to merge 18 commits into from

Conversation

jwntrs
Copy link
Contributor

@jwntrs jwntrs commented Feb 6, 2018

Fixes #1102

If you asked for the groups scope but didn't specify any orgs, the github connector would return an empty set of groups. This should now be fixed.

Based on this change we now return a list of groups that looks like the following:

[
    "org-1:team-1",
    "org-1:team-2",
    "org-1:team-3",
    "org-2:team-4",
    "org-3"
]

If teams exist, they are scoped to an org, otherwise the org is listed on its own.

@jwntrs
Copy link
Contributor Author

jwntrs commented Feb 6, 2018

Looks like I'm using features in httptest server that weren't available in golang 1.8.

@joshadambell
Copy link

This looks great! I was just about to implement this.

@joshadambell
Copy link

@ericchiang Can we get this reviewed?

@@ -596,3 +600,76 @@ func (c *githubConnector) teamsForOrg(ctx context.Context, client *http.Client,

return groups, nil
}

Choose a reason for hiding this comment

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

Function comment needed.

return groups, err
}

func (c *githubConnector) userOrgs(ctx context.Context, client *http.Client) ([]string, error) {

Choose a reason for hiding this comment

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

Same here.


return groups, nil
}

Choose a reason for hiding this comment

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

Same here.

groups = append(groups, org)
} else {
for _, team := range teams {
groups = append(groups, org+":"+team)

Choose a reason for hiding this comment

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

Do you want to put this and line 357 as a function so formatting is spread out?

Copy link

@joshadambell joshadambell left a comment

Choose a reason for hiding this comment

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

A couple suggestions.

@jwntrs
Copy link
Contributor Author

jwntrs commented Mar 9, 2018

@joshadambell updated as per your suggestions. thanks.

@mrIncompetent
Copy link

@ericchiang Anything missing on this PR?

@vito vito deleted the master branch August 14, 2018 21:05
@vito
Copy link
Contributor

vito commented Aug 14, 2018

I ended up deleting the branch that this PR points to.

We've ended up with a bunch of work on our fork of Dex so I spent a lot of time cleaning things up so that we can at least more easily keep up to date with upstream and keep tabs on any PR-able work on separate branches instead. More info here: concourse#1

We'll probably re-submit this PR (along with our other pr/* branches) in the future, against https://github.com/concourse/dex/tree/pr/github-teams-no-org-and-slugs

Rui Yang and others added 4 commits September 6, 2018 10:38
…pr/add-oauth-connector', 'origin/pr/add-web-host-url', 'origin/pr/github-teams-no-org-and-slugs', 'origin/pr/gitlab-broken-auth-headers', 'origin/pr/gitlab-include-user-scope-for-groups-inspection', 'origin/pr/oidc-groups-and-tls', 'origin/pr/optional-prometheus-logger', 'origin/pr/postgres-unix-sockets' and 'origin/pr/unset-pg-serializable-transaction-level'
@jwntrs jwntrs closed this Sep 10, 2018
alexmt pushed a commit to alexmt/dex that referenced this pull request Nov 14, 2018
srenatus added a commit that referenced this pull request Nov 16, 2018
Issue #1184 - Github connector now returns a full group list when no org is specified
mmrath pushed a commit to mmrath/dex that referenced this pull request Sep 2, 2019
mmrath pushed a commit to mmrath/dex that referenced this pull request Sep 2, 2019
Issue dexidp#1184 - Github connector now returns a full group list when no org is specified
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.

Github: Groups not getting loaded when no org is specified in config
4 participants