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

Authorize by GitHub org and or team membership #205

Merged
merged 21 commits into from
Mar 12, 2020

Conversation

eikehartmann
Copy link
Contributor

Allows configuration for GitHub to set whitelist on teams and or organizations when configured like this:

vouch:
  teamWhitelist:
   - myOrg
   - myOrg/myTeam

As mentioned in #150 this requires additional GitHub API calls to check the membership status.

Also includes the fix for checks of an email against domains: #199.

@bnfinet
Copy link
Member

bnfinet commented Feb 7, 2020

nice stuff @eikehartmann thanks for the contribution. Both @aaronpk and myself are very keen to get this in.

Could you make just a couple adjustments...

  • move GitHub specific logic to its own file (in the handlers package or as a separate package). handlers.go is getting a bit crowded
  • can you consider the GitHub enterprise (on prem) use case, how do you configure the api call?

And thanks for adding such thorough tests to VP

@eikehartmann eikehartmann changed the title Authorize by GitHub org and or team membership WIP: Authorize by GitHub org and or team membership Feb 10, 2020
@eikehartmann
Copy link
Contributor Author

Hi @bnfinet,

thanks for the review and input. I moved the vendor-specific stuff to their own packages and tried to generalise on the function call. GitHub enterprise sample config has also been updated.

Will still improve logging and error handling today and remove the WIP label once done.

@eikehartmann eikehartmann changed the title WIP: Authorize by GitHub org and or team membership Authorize by GitHub org and or team membership Feb 10, 2020
@bnfinet
Copy link
Member

bnfinet commented Feb 11, 2020

much appreciated @eikehartmann

I am engaged to another project at the moment and won't be able to give this my attention for a few more days. I am hopeful of this week but it may slip to next week. Thanks for the code and your patience.

@bnfinet bnfinet merged commit 8dce88f into vouch:master Mar 12, 2020
@bnfinet
Copy link
Member

bnfinet commented Mar 12, 2020

@eikehartmann thanks again for the fine work!

log.Debugf("removing port from %s to test domain %s", s, split[0])
s = split[0]
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

bnfinet added a commit that referenced this pull request Apr 8, 2020
bnfinet added a commit that referenced this pull request May 22, 2020
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.

2 participants