Skip to content
This repository has been archived by the owner on Dec 18, 2020. It is now read-only.

Allow limiting authentication to members of a specific GitHub org #14

Merged
merged 4 commits into from
Feb 19, 2015
Merged

Allow limiting authentication to members of a specific GitHub org #14

merged 4 commits into from
Feb 19, 2015

Conversation

edef1c
Copy link
Contributor

@edef1c edef1c commented Feb 19, 2015

This includes tests.

@@ -81,6 +82,13 @@ AuthenticateGithub.prototype.getAuthorizationToken = function(username, password
if (err) reject(err);
else resolve(res.token);
});
}).then(this.githubOrg && function(token) {
return new Promise(function(resolve, reject) {
github.orgs.getMember({ user: username, org: _this.githubOrg }, function(err, res) {
Copy link
Contributor

Choose a reason for hiding this comment

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

based on this logic, I gather an error is returned if a user is not a member of the organization? You've confirmed this behavior hitting the live API?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have confirmed this behaviour on the live API, yes.
These two authenticating as me, a member of npm:

 ~ ⮀ curl -siLn https://api.github.com/orgs/npm/members/nathan7 | grep '^HTTP'     
HTTP/1.1 204 No Content
 ~ ⮀ curl -siLn https://api.github.com/orgs/npm/members/itzmjauz | grep '^HTTP'
HTTP/1.1 404 Not Found

These two unauthenticated:

 ~ ⮀ curl -siL https://api.github.com/orgs/npm/members/nathan7 | grep '^HTTP' 
HTTP/1.1 302 Found
HTTP/1.1 204 No Content
 ~ ⮀ curl -siL https://api.github.com/orgs/npm/members/itzmjauz | grep '^HTTP' 
HTTP/1.1 302 Found
HTTP/1.1 404 Not Found

The GitHub API client you're using handles the redirect — all "not a member" paths return a 404.

bcoe added a commit that referenced this pull request Feb 19, 2015
Allow limiting authentication to members of a specific GitHub org
@bcoe bcoe merged commit 41e5467 into npm:master Feb 19, 2015
@edef1c edef1c deleted the org-checking branch February 19, 2015 22:16
@@ -11,6 +11,7 @@ function AuthenticateGithub(opts) {
debug: true,
githubHost: config.githubHost,
githubPathPrefix: '/api/v3',
githubOrg: config.githubOrg,
Copy link

Choose a reason for hiding this comment

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

I know it's been a while, but would it be possible to extend it in order to support multiple organizations? Really don't want to come up with a fork of this repo just because of this restriction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

master...nathan7:multiple-github-orgs this would do the trick — any opinion on this, @bcoe?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants