Allow custom key to be used for whitelist and X-Forwarded-User instead of the hardcoded email #159

maxisme · 2020-08-01T14:09:11Z

The callback of GitHubs USER_URL returns the json:

{
  "avatar_url": "https://avatars0.githubusercontent.com/u/16902919?v=4",
  "bio": "Studying Computer Science with Artificial Intelligence at Sussex Uni. Passionate about product building.rn",
  "blog": "max.me.uk",
  "company": null,
  "created_at": "2016-01-26T17:01:55Z",
  "email": "max@max.me.uk",
  "events_url": "https://api.github.com/users/maxisme/events{/privacy}",
  "followers": 20,
  "followers_url": "https://api.github.com/users/maxisme/followers",
  "following": 42,
  "following_url": "https://api.github.com/users/maxisme/following{/other_user}",
  "gists_url": "https://api.github.com/users/maxisme/gists{/gist_id}",
  "gravatar_id": "",
  "hireable": false,
  "html_url": "https://github.com/maxisme",
  "id": 16902919,
  "location": "London, United Kingdom",
  "login": "maxisme",
  "name": "Maximilian Mitchell",
  "node_id": "MDQ6VXNlcjE2OTAyOTE5",
  "organizations_url": "https://api.github.com/users/maxisme/orgs",
  "public_gists": 1,
  "public_repos": 49,
  "received_events_url": "https://api.github.com/users/maxisme/received_events",
  "repos_url": "https://api.github.com/users/maxisme/repos",
  "site_admin": false,
  "starred_url": "https://api.github.com/users/maxisme/starred{/owner}{/repo}",
  "subscriptions_url": "https://api.github.com/users/maxisme/subscriptions",
  "twitter_username": "maxmeuk",
  "type": "User",
  "updated_at": "2020-08-01T12:56:35Z",
  "url": "https://api.github.com/users/maxisme"
}

With this PR you can now customize the value used for whitelisting and X-Forwarded-User. Before it was hardcoded to be the email tag (now default).

This solves the problem of needing to expose your email in GitHub as well 🙂

Warning: have only tested fully with GH with `GenericOAuth` and no other providers (I left `OIDC` as is) but should work with `Google`.

Working Docker image at maxisme/traefik-forward-auth

I also:

Upgraded to Go 1.14
Added to the README that you can comma separate the whitelists and domains as that isn't immediately obvious (and added test).
Added a GitHub action to test and deploy to docker (lmk if you want me to remove that)

update error message

This reverts commit 40bd110 It prevents GO 1.12 from working 1.13 + 1.14 still work however.

This reverts commit 40bd110

thomseddon · 2020-08-22T13:39:43Z

Hi, thanks for the PR - I like this and I think it makes sense, I don't love that we're so tied to email addresses right now, so this is a nice step forward.

I like the implementation and there isn't anything I want to change other than a few variable names and docs:

I find "UserID" a little confusing as I expect this to be a database key, please could we use "user" (I also don't think we need a struct for this, using a string is fine for me
Could we use "UserPath" over "UserIDPath"
Please specify that the use of this parameter is currently only supported for the generic-oauth provider
Please use "Comma Separated" over "Comma Delimited" as I think "separated" is more consistent with naming elsewhere

And yes, please could you drop the github action out - we're looking and using an action to build the cross platform image, but I'm happy with docker hub's build at the moment

Thanks again for your work on this, it will be a great addition :)

remove UserID type rename comma delimited to comma separated

maxisme · 2020-08-22T19:01:28Z

Hey, no problem! I needed it myself :)

Would you be able to help out with:

Please specify that the use of this parameter is currently only supported for the generic-oauth provider

Would be cool to add example JSONs from the USER_URLs of different providers so that users don't have to work it out by themselves.

ccoenen · 2020-10-08T11:11:51Z

If I can somehow get my hands on a build of this version, I could verify it against nextcloud, if anyone is interested. (related to #191, where there's an example json including relevant fields for nextcloud).

Also it would be amazing if other properties could be read (and configured) as well. Things I use with other OAuth clients (with nextcloud being the OAuth provider) are id, email, groups and display-name. I am not sure if this pr should be extended or if this is a whole other matter.

maxisme · 2020-10-08T11:19:48Z

Hey this is my docker-compose with the docker image of this build - maxisme/traefik-forward-auth:

  traefik-forward-auth:
    image: maxisme/traefik-forward-auth
    environment:
      - "DEFAULT_PROVIDER=generic-oauth"
      - "PROVIDERS_GENERIC_OAUTH_AUTH_URL=${PROVIDERS_GENERIC_OAUTH_AUTH_URL:?err}"
      - "PROVIDERS_GENERIC_OAUTH_TOKEN_URL=${PROVIDERS_GENERIC_OAUTH_TOKEN_URL:?err}"
      - "PROVIDERS_GENERIC_OAUTH_USER_URL=${PROVIDERS_GENERIC_OAUTH_USER_URL:?err}"
      - "PROVIDERS_GENERIC_OAUTH_CLIENT_ID=${PROVIDERS_GENERIC_OAUTH_CLIENT_ID:?err}"
      - "PROVIDERS_GENERIC_OAUTH_CLIENT_SECRET=${PROVIDERS_GENERIC_OAUTH_CLIENT_SECRET:?err}"
      - "WHITELIST=maxisme"
      - "USER_ID_PATH=login"
      - "SECRET=${OAUTH_SECRET:?err}"
      - "COOKIE_DOMAIN=${DOMAIN}"
      - "AUTH_HOST=auth.${DOMAIN}"

so the USER_ID_PATH would be ocs.data.id in the example from #191.

ccoenen · 2020-10-08T14:24:15Z

I can confirm that your fork (and USER_ID_PATH set to ocs.data.id) work as intended in combination with nextcloud:

ccoenen · 2020-10-08T14:44:03Z

README.md

  --lifetime=                                           Lifetime in seconds (default: 43200) [$LIFETIME]
  --logout-redirect=                                    URL to redirect to following logout [$LOGOUT_REDIRECT]
  --url-path=                                           Callback URL Path (default: /_oauth) [$URL_PATH]
  --secret=                                             Secret used for signing (required) [$SECRET]
-  --whitelist=                                          Only allow given email addresses, can be set multiple times [$WHITELIST]
+  --whitelist=                                          Only allow given email addresses, comma separated, can be set multiple times [$WHITELIST]


I believe this should now also be

-Only allow given email addresses +Only allow given user id

Nice, good catch!

ccoenen · 2020-10-08T14:44:32Z

Dockerfile

@@ -1,4 +1,4 @@
-FROM golang:1.13-alpine as builder
+FROM golang:1.14-alpine as builder


strictly speaking, these version updates would normally not be part of a pull request.

How do you mean? Only @thomseddon should make these changes?

that's completely up to the maintainer, but in many projects I contributed to, a PR was really just about one specific feature or bugfix.

ccoenen · 2020-10-08T14:53:43Z

after reviewing, I think the only thing missing is a test case for plain user ids. There's an extensive test case for user emails, but nothing just for user names. It should also be considered if the current email test in internal/auth_test.go should also be renamed, for example like

-func TestAuthValidateEmail(t *testing.T) {
+func TestAuthValidateUser(t *testing.T) {

But I would also not mind if the two were kept separate.

ccoenen · 2020-12-01T17:17:21Z

what would be the way forward for getting this merged and released?

…ails

…d-auth � Conflicts: � internal/auth.go � internal/auth_test.go � internal/server.go

maxisme · 2021-01-12T21:21:14Z

@ccoenen @thomseddon would you be able to approve maxisme#1 to fix the conflicts. I haven't written Go in a few months and it all seems very alien haha. I will then merge into this PR and then should be good to go?

Fix conflicts

ccoenen · 2021-02-06T23:34:45Z

I don't feel entirely qualified to review this, I'm sorry. I'm just some random user with basic knowledge of the Go language.

dabeck · 2021-03-09T13:26:32Z

internal/server.go

-			logger.WithField("email", email).Warn("Invalid email")
-			http.Error(w, "Not authorized", 401)
+			logger.WithField("user", user).Warn("Invalid user")
+			http.Error(w, fmt.Sprintf("User '%s' is not authorized", user), 401)


Suggested change

http.Error(w, fmt.Sprintf("User '%s' is not authorized", user), 401)

http.Error(w, "Not authorized", 401)

tomlankhorst · 2021-10-17T09:30:36Z

Thanks to everyone involved in this change. This PR seems valuable to many users. Thus it would be nice to merge (or decline) it soon, especially since it seems fully functional and reviewed.

One issue that I spot, is that it doesn't seem possible to combine a non-email user identified with domain validation.
Might be wise to pass around User objects with both an email and an id field. Then validate the domain based on email and validate the user based on id.

gcaracuel · 2022-01-18T13:32:43Z

Hey @maxisme I would love to this merged. Please take in account this comment with proposal: #159 (comment)

Maybe @thomseddon could help us pushing this?

hjalmarlucius · 2022-02-05T15:02:48Z

@maxisme I appears to me that this line is missing from the readme.txt. https://github.com/maxisme/traefik-forward-auth/blob/a98e568f6fe8bef192180200b69fe76329e5647a/internal/config.go#L41

maxisme added 16 commits August 1, 2020 13:55

init commit

1b7d054

add github workflow

bcadb3a

fix naming

b290b21

fix missing param

f041759

upgrade Go version to 1.14

0e2bb23

tmp remove of tests

b0f7b17

update error message

add more specific error message

c792263

put back tests

189e4a1

rename User ID Key to User ID Path

399f3da

upgrade dependencies

40bd110

Revert "upgrade dependencies"

2a2d542

This reverts commit 40bd110 It prevents GO 1.12 from working 1.13 + 1.14 still work however.

Revert "upgrade dependencies"

22aa772

This reverts commit 40bd110

mention the user that is not authorized

d9c2ec2

mention the user that is not authorized

cb02259

tidy error message

c77e649

tidy error message

4b554e7

thomseddon added enhancement New feature or request under review labels Aug 22, 2020

maxisme added 3 commits August 22, 2020 18:49

remove actions

c7f5f0a

rename UserIDPath to UserID

fb70085

remove UserID type rename comma delimited to comma separated

rename GetUsedID function to GetUser

42b3750

thomseddon mentioned this pull request Oct 8, 2020

Generic OAuth2: decoding the user json #191

Open

ccoenen reviewed Oct 8, 2020

View reviewed changes

Max Mitchell added 6 commits January 12, 2021 20:38

revert docker golang version to 1.13

a33a869

change whitelist comment to indicate userIDs instead of explicitly em…

9ea7d98

…ails

revert go version

49439c7

Merge branch 'master' of https://github.com/thomseddon/traefik-forwar…

4906a18

…d-auth � Conflicts: � internal/auth.go � internal/auth_test.go � internal/server.go

fix conflicts

4091bb1

add tests

58d555c

Max Mitchell and others added 2 commits January 13, 2021 13:57

push to docker for testing

dc20081

Merge pull request #1 from maxisme/fix-conflicts

a98e568

Fix conflicts

dabeck reviewed Mar 9, 2021

View reviewed changes

thmo mentioned this pull request Apr 20, 2021

Allow to customize result header name #241

Open

thomseddon mentioned this pull request Jun 24, 2021

GitHub provider whitelist with username? #256

Closed

thmo mentioned this pull request Dec 2, 2021

Can username be customized other than email? #266

Open

jordemort mentioned this pull request Nov 5, 2022

Allow custom key to be used for whitelist and X-Forwarded-User instead of the hardcoded email jordemort/traefik-forward-auth#1

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow custom key to be used for whitelist and X-Forwarded-User instead of the hardcoded email #159

Allow custom key to be used for whitelist and X-Forwarded-User instead of the hardcoded email #159

maxisme commented Aug 1, 2020 •

edited

thomseddon commented Aug 22, 2020

maxisme commented Aug 22, 2020

ccoenen commented Oct 8, 2020 •

edited

maxisme commented Oct 8, 2020

ccoenen commented Oct 8, 2020

ccoenen Oct 8, 2020

maxisme Oct 9, 2020

maxisme Jan 12, 2021

ccoenen Oct 8, 2020

maxisme Oct 9, 2020

ccoenen Oct 9, 2020

maxisme Jan 12, 2021

ccoenen commented Oct 8, 2020

ccoenen commented Dec 1, 2020

maxisme commented Jan 12, 2021

ccoenen commented Feb 6, 2021

dabeck Mar 9, 2021

tomlankhorst commented Oct 17, 2021 •

edited

gcaracuel commented Jan 18, 2022

hjalmarlucius commented Feb 5, 2022

		@@ -1,4 +1,4 @@
		FROM golang:1.13-alpine as builder
		FROM golang:1.14-alpine as builder

	http.Error(w, fmt.Sprintf("User '%s' is not authorized", user), 401)
	http.Error(w, "Not authorized", 401)

Allow custom key to be used for whitelist and X-Forwarded-User instead of the hardcoded email #159

Are you sure you want to change the base?

Allow custom key to be used for whitelist and X-Forwarded-User instead of the hardcoded email #159

Conversation

maxisme commented Aug 1, 2020 • edited

Warning: have only tested fully with GH with GenericOAuth and no other providers (I left OIDC as is) but should work with Google.

thomseddon commented Aug 22, 2020

maxisme commented Aug 22, 2020

ccoenen commented Oct 8, 2020 • edited

maxisme commented Oct 8, 2020

ccoenen commented Oct 8, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccoenen commented Oct 8, 2020

ccoenen commented Dec 1, 2020

maxisme commented Jan 12, 2021

ccoenen commented Feb 6, 2021

Choose a reason for hiding this comment

tomlankhorst commented Oct 17, 2021 • edited

gcaracuel commented Jan 18, 2022

hjalmarlucius commented Feb 5, 2022

maxisme commented Aug 1, 2020 •

edited

Warning: have only tested fully with GH with `GenericOAuth` and no other providers (I left `OIDC` as is) but should work with `Google`.

ccoenen commented Oct 8, 2020 •

edited

tomlankhorst commented Oct 17, 2021 •

edited