Support new option "github-user" #421

yyoshiki41 · 2020-02-28T07:37:24Z

Description

Add new option github-user.

Motivation and Context

That allowed the specified github users that is not on team or org.
e.g.) Username list (comma-separated)
"github-user:octocat,yyoshiki41"

fixed below issue.
#342

How Has This Been Tested?

My develop environment works this option.

Checklist:

My change requires a change to the documentation or CHANGELOG.
I have updated the documentation/CHANGELOG accordingly.
I have created a feature (non-master) branch for my PR.

This change is

tg44 · 2020-02-29T09:44:45Z

I'm not a pro, nor a maintainer, but this seems to be ok.

JoelSpeed

Generally looks ok, added a comment about trying to make the configuration more consistent though

JoelSpeed · 2020-03-01T15:26:02Z

main.go

@@ -64,6 +64,7 @@ func main() {
 	flagSet.String("bitbucket-repository", "", "restrict logins to user with access to this repository")
 	flagSet.String("github-org", "", "restrict logins to members of this organisation")
 	flagSet.String("github-team", "", "restrict logins to members of this team")
+	flagSet.String("github-user", "", "restrict logins to these users")


We have a number of options that allow the flag to be specified multiple times, eg whitelist-domain, would it be possible to follow the same pattern?

fixed by using flagSet.StringSlice.

github-actions · 2020-05-15T00:04:29Z

This pull request has been inactive for 60 days. If the pull request is still relevant please comment to re-activate the pull request. If no action is taken within 7 days, the pull request will be marked closed.

yyoshiki41 · 2020-05-18T05:41:31Z

@JoelSpeed

Sorry for late reply.
I fixed some issues and pushed.

Thanks!

JoelSpeed · 2020-05-21T18:18:38Z

options.go

@@ -56,6 +56,7 @@ type Options struct {
 	GitHubTeam               string   `flag:"github-team" cfg:"github_team" env:"OAUTH2_PROXY_GITHUB_TEAM"`
 	GitHubRepo               string   `flag:"github-repo" cfg:"github_repo" env:"OAUTH2_PROXY_GITHUB_REPO"`
 	GitHubToken              string   `flag:"github-token" cfg:"github_token" env:"OAUTH2_PROXY_GITHUB_TOKEN"`
+	GitHubUsers              []string `flag:"github-user" cfg:"github_user" env:"OAUTH2_PROXY_GITHUB_USER"`


To be consistent with other list options, will also need to update the docs

Suggested change

GitHubUsers []string `flag:"github-user" cfg:"github_user" env:"OAUTH2_PROXY_GITHUB_USER"`

GitHubUsers []string `flag:"github-user" cfg:"github_users" env:"OAUTH2_PROXY_GITHUB_USERS"`

8952cfb
and updated docs: be7f969.

JoelSpeed · 2020-05-21T18:19:27Z

providers/github.go

+	for _, user := range users {
+		us := strings.Split(user, ",")
+		for _, u := range us {
+			// Validate empty string (e.g. --github-user="octcat,,")
+			if len(u) > 0 {
+				p.Users = append(p.Users, u)
+			}
+		}
+	}


This will be covered by viper, the configuration loader

JoelSpeed · 2020-05-21T18:22:03Z

providers/github.go

+	}
+
+	if p.isVerifiedUser(user.Login) {
+		log.Printf("Found Github User: %q", user.Login)


Will this log line become noisy? Same for the one below?

JoelSpeed · 2020-05-21T18:23:30Z

providers/github.go

+		} else if p.Repo != "" {
+			if p.Token == "" { // If we have a token we'll do the collaborator check in GetUserName


Why split these two up?

yyoshiki41 · 2020-05-21T21:45:57Z

@JoelSpeed
Thanks for your review!
I fixed all your review points and pushed.

JoelSpeed · 2020-05-23T13:50:29Z

options.go

@@ -56,6 +56,7 @@ type Options struct {
 	GitHubTeam               string   `flag:"github-team" cfg:"github_team" env:"OAUTH2_PROXY_GITHUB_TEAM"`
 	GitHubRepo               string   `flag:"github-repo" cfg:"github_repo" env:"OAUTH2_PROXY_GITHUB_REPO"`
 	GitHubToken              string   `flag:"github-token" cfg:"github_token" env:"OAUTH2_PROXY_GITHUB_TOKEN"`
+	GitHubUsers              []string `flag:"github-users" cfg:"github_users" env:"OAUTH2_PROXY_GITHUB_USERS"`


Sorry, I wasn't very clear in my previous comment. The flag should be singular, while the other two should be pluralised (yes this is very confusing, but it's what we have for now).

Suggested change

GitHubUsers []string `flag:"github-users" cfg:"github_users" env:"OAUTH2_PROXY_GITHUB_USERS"`

GitHubUsers []string `flag:"github-user" cfg:"github_users" env:"OAUTH2_PROXY_GITHUB_USERS"`

Thanks for your explanation, I understood!
5dc1192

JoelSpeed · 2020-05-23T13:51:24Z

providers/github.go

+	if p.isVerifiedUser(user.Login) {
+		return true, nil
+	}
+	log.Printf("Missing Github User: %q in %v", user.Login, p.Users)


I think we should drop this log too, could get noisy

Remove logging 19f9b34

JoelSpeed · 2020-05-25T13:35:12Z

@yyoshiki41 Could you please rebase this and then I'll give it another review

yyoshiki41 · 2020-05-25T21:35:37Z

@JoelSpeed
rebased and moved options to follow bellow pr
#489

Thanks!

JoelSpeed

I've added some suggestions to improve readability in the docs, and some other small things through the code, happy for those to all be commited, though I added one question to one of them which I'd like an answer on before merging if that's ok

JoelSpeed · 2020-05-31T14:51:53Z

docs/2_auth.md

@@ -103,6 +103,8 @@ Note: When using the Azure Auth provider with nginx and the cookie session store

 The GitHub auth provider supports two additional ways to restrict authentication to either organization and optional team level access, or to collaborators of a repository. Restricting by these options is normally accompanied with `--email-domain=*`

+NOTE: When `--github-user` is set, the specified users are allowed logins even if they do not belong to the specified org and team or collaborators.


This will read better this way

Suggested change

NOTE: When `--github-user` is set, the specified users are allowed logins even if they do not belong to the specified org and team or collaborators.

NOTE: When `--github-user` is set, the specified users are allowed to login even if they do not belong to the specified org and team or collaborators.

JoelSpeed · 2020-05-31T14:52:45Z

docs/2_auth.md

@@ -119,6 +121,10 @@ If you'd like to allow access to users with **read only** access to a **public**

    -github-token="": the token to use when verifying repository collaborators

+To allow logins by usernames even if they do not belong to the specified org and team or collaborators, separated by a comma


Suggested change

To allow logins by usernames even if they do not belong to the specified org and team or collaborators, separated by a comma

To allow a user to login with their username even if they do not belong to the specified org and team or collaborators, separated by a comma

JoelSpeed · 2020-05-31T14:53:31Z

docs/configuration/configuration.md

@@ -56,6 +56,7 @@ An example [oauth2-proxy.cfg]({{ site.gitweb }}/contrib/oauth2-proxy.cfg.example
 | `--github-team` | string | restrict logins to members of any of these teams (slug), separated by a comma | |
 | `--github-repo` | string | restrict logins to collaborators of this repository formatted as `orgname/repo` | |
 | `--github-token` | string | the token to use when verifying repository collaborators (must have push access to the repository) | |
+| `--github-user` | string | To allow logins by username even if they do not belong to the specified org and team or collaborators, separated by a comma | |


JoelSpeed · 2020-05-31T14:54:06Z

pkg/apis/options/options.go

@@ -256,6 +257,7 @@ func NewFlagSet() *pflag.FlagSet {
 	flagSet.String("github-team", "", "restrict logins to members of this team")
 	flagSet.String("github-repo", "", "restrict logins to collaborators of this repository")
 	flagSet.String("github-token", "", "the token to use when verifying repository collaborators (must have push access to the repository)")
+	flagSet.StringSlice("github-user", []string{}, "allowed usernames even if they do not belong to the specified org and team or collaborators (may be given multiple times)")


Suggested change

flagSet.StringSlice("github-user", []string{}, "allowed usernames even if they do not belong to the specified org and team or collaborators (may be given multiple times)")

flagSet.StringSlice("github-user", []string{}, "allowed users with these usernames to login even if they do not belong to the specified org and team or collaborators (may be given multiple times)")

JoelSpeed · 2020-05-31T14:55:14Z

providers/github.go

@@ -80,6 +82,11 @@ func (p *GitHubProvider) SetRepo(repo, token string) {
 	p.Token = token
 }

+// SetUsers configures allowed usernames
+func (p *GitHubProvider) SetUsers(users []string) {
+	p.Users = append(p.Users, users...)


Why not just overwrite here? That is what I think I would have expected

Suggested change

p.Users = append(p.Users, users...)

p.Users = users

Sorry, I missed this points.
fixed by 86cadbf

JoelSpeed · 2020-05-31T15:00:08Z

providers/github_test.go

+
+	bURL, _ := url.Parse(b.URL)
+	p := testGitHubProvider(bURL.Host)
+	p.SetUsers([]string{"mbland", "octcat"})


Suggested change

p.SetUsers([]string{"mbland", "octcat"})

p.SetUsers([]string{"mbland", "octocat"})

JoelSpeed · 2020-05-31T15:00:24Z

providers/github_test.go

+
+	bURL, _ := url.Parse(b.URL)
+	p := testGitHubProvider(bURL.Host)
+	p.SetUsers([]string{"octcat"})


Suggested change

p.SetUsers([]string{"octcat"})

p.SetUsers([]string{"octocat"})

yyoshiki41 · 2020-06-01T09:48:48Z

@JoelSpeed

fixed to improve readability in the docs
0333f9f

sorry, I missed your review.
I fixed it by 86cadbf

I fixed all your review points .
Thanks.

pkg/apis/options/options.go

JoelSpeed

I fixed up one last little thing, LGTM

Thanks @yyoshiki41

yyoshiki41 requested review from JoelSpeed, steakunderscore and loshz as code owners February 28, 2020 07:37

yyoshiki41 requested a review from a team February 28, 2020 07:37

yyoshiki41 mentioned this pull request Feb 28, 2020

Support username list #342

Closed

JoelSpeed reviewed Mar 1, 2020

View reviewed changes

github-actions bot added the Stale label May 15, 2020

yyoshiki41 force-pushed the github-user branch from 79f9755 to 30d99c7 Compare May 18, 2020 05:34

JoelSpeed removed the Stale label May 21, 2020

JoelSpeed reviewed May 21, 2020

View reviewed changes

JoelSpeed reviewed May 23, 2020

View reviewed changes

yyoshiki41 added 8 commits May 26, 2020 06:18

feat(github): support new option "github-user"

bef5850

feat(github): rename github-user to github-users

e0ea2d2

feat(github): update docs for github-users option

e317abe

feat(github): remove unneeded code

c8efa84

feat(github): remove logging

b87eb21

feat(github-user): use github-user as flagset options

724b311

feat(github-user): remove optionns.go

b6d9b99

feat(github-user): add github-user flagset

dc45eb3

yyoshiki41 force-pushed the github-user branch from 5dc1192 to dc45eb3 Compare May 25, 2020 21:29

Merge branch 'master' into github-user

d41ef14

JoelSpeed reviewed May 31, 2020

View reviewed changes

feat(github): improve readability in the docs

0333f9f

feat(github-user): refactored SetUsers method

86cadbf

JoelSpeed reviewed Jun 1, 2020

View reviewed changes

pkg/apis/options/options.go Outdated Show resolved Hide resolved

Update flag description

1bbd9e1

JoelSpeed approved these changes Jun 1, 2020

View reviewed changes

JoelSpeed merged commit d8d43bb into oauth2-proxy:master Jun 1, 2020

yyoshiki41 deleted the github-user branch June 2, 2020 00:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support new option "github-user" #421

Support new option "github-user" #421

yyoshiki41 commented Feb 28, 2020 •

edited

tg44 commented Feb 29, 2020

JoelSpeed left a comment

JoelSpeed Mar 1, 2020

yyoshiki41 May 18, 2020

github-actions bot commented May 15, 2020

yyoshiki41 commented May 18, 2020 •

edited

JoelSpeed May 21, 2020

yyoshiki41 May 21, 2020

JoelSpeed May 21, 2020

JoelSpeed May 21, 2020

JoelSpeed May 21, 2020

yyoshiki41 commented May 21, 2020

JoelSpeed May 23, 2020

yyoshiki41 May 23, 2020

JoelSpeed May 23, 2020

yyoshiki41 May 23, 2020

JoelSpeed commented May 25, 2020

yyoshiki41 commented May 25, 2020 •

edited

JoelSpeed left a comment

JoelSpeed May 31, 2020

JoelSpeed May 31, 2020

JoelSpeed May 31, 2020

JoelSpeed May 31, 2020

JoelSpeed May 31, 2020

yyoshiki41 Jun 1, 2020

JoelSpeed May 31, 2020

JoelSpeed May 31, 2020

yyoshiki41 commented Jun 1, 2020

JoelSpeed left a comment

	GitHubUsers []string `flag:"github-user" cfg:"github_user" env:"OAUTH2_PROXY_GITHUB_USER"`
	GitHubUsers []string `flag:"github-user" cfg:"github_users" env:"OAUTH2_PROXY_GITHUB_USERS"`

		} else if p.Repo != "" {
		if p.Token == "" { // If we have a token we'll do the collaborator check in GetUserName

	GitHubUsers []string `flag:"github-users" cfg:"github_users" env:"OAUTH2_PROXY_GITHUB_USERS"`
	GitHubUsers []string `flag:"github-user" cfg:"github_users" env:"OAUTH2_PROXY_GITHUB_USERS"`

		@@ -103,6 +103,8 @@ Note: When using the Azure Auth provider with nginx and the cookie session store

		The GitHub auth provider supports two additional ways to restrict authentication to either organization and optional team level access, or to collaborators of a repository. Restricting by these options is normally accompanied with `--email-domain=*`

		NOTE: When `--github-user` is set, the specified users are allowed logins even if they do not belong to the specified org and team or collaborators.

		@@ -119,6 +121,10 @@ If you'd like to allow access to users with read only access to a public

		-github-token="": the token to use when verifying repository collaborators

		To allow logins by usernames even if they do not belong to the specified org and team or collaborators, separated by a comma

	To allow logins by usernames even if they do not belong to the specified org and team or collaborators, separated by a comma
	To allow a user to login with their username even if they do not belong to the specified org and team or collaborators, separated by a comma

	\| `--github-user` \| string \| To allow logins by username even if they do not belong to the specified org and team or collaborators, separated by a comma \| \|
	\| `--github-user` \| string \\| list \| To allow users to login by username even if they do not belong to the specified org and team or collaborators \| \|

	flagSet.StringSlice("github-user", []string{}, "allowed usernames even if they do not belong to the specified org and team or collaborators (may be given multiple times)")
	flagSet.StringSlice("github-user", []string{}, "allowed users with these usernames to login even if they do not belong to the specified org and team or collaborators (may be given multiple times)")

	p.SetUsers([]string{"mbland", "octcat"})
	p.SetUsers([]string{"mbland", "octocat"})

	p.SetUsers([]string{"octcat"})
	p.SetUsers([]string{"octocat"})

Support new option "github-user" #421

Support new option "github-user" #421

Conversation

yyoshiki41 commented Feb 28, 2020 • edited

Description

Motivation and Context

How Has This Been Tested?

Checklist:

tg44 commented Feb 29, 2020

JoelSpeed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 15, 2020

yyoshiki41 commented May 18, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yyoshiki41 commented May 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoelSpeed commented May 25, 2020

yyoshiki41 commented May 25, 2020 • edited

JoelSpeed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yyoshiki41 commented Jun 1, 2020

JoelSpeed left a comment

Choose a reason for hiding this comment

yyoshiki41 commented Feb 28, 2020 •

edited

yyoshiki41 commented May 18, 2020 •

edited

yyoshiki41 commented May 25, 2020 •

edited