Skip to content
This repository has been archived by the owner on Jan 24, 2019. It is now read-only.

Iterate through pages returned by List Your Organizations endpoint #414

Merged
merged 1 commit into from
Dec 4, 2017

Conversation

relaxdiego
Copy link
Contributor

PREAMBLE: This PR leaves much to be desired at the time of submission.
I'm submitting it anyway to get early feedback. I'm also still ramping up
on my golang-fu so please excuse the dust.

=====

For some GHE instances where a user can have more than 100
organizations, traversing the other pages is important otherwise
oauth2_proxy will consider the user unauthorized. This change traverses
the list returned by the API to avoid that.

@jehiah jehiah added this to the v2.3 milestone Oct 23, 2017
@@ -61,36 +62,45 @@ func (p *GitHubProvider) hasOrg(accessToken string) (bool, error) {
Login string `json:"login"`
}

params := url.Values{
"limit": {"100"},
var orgs_page []struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment on naming, in go we typically use camel case/mixed caps rather than underscores - so I'd update this to orgsPage and page_number to pageNum.

https://golang.org/doc/effective_go.html#mixed-caps

For some GHE instances where a user can have more than 100
organizations, traversing the other pages is important otherwise
oauth2_proxy will consider the user unauthorized. This change traverses
the list returned by the API to avoid that.

Update github provider tests to include this case.
@talam
Copy link
Contributor

talam commented Dec 4, 2017

@hlhendy update this PR based on the last set of comments and made some slight updates to the logic. Also included some tests. RFR.

Copy link
Contributor

@hlhendy hlhendy left a comment

Choose a reason for hiding this comment

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

LGTM

@hlhendy hlhendy merged commit d75f626 into bitly:master Dec 4, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

4 participants