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

[maxisme]

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": "[email protected]",
  "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)

[thomseddon]

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 :)

[maxisme]

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]

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]

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]

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

[ccoenen]

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]

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

[maxisme]

@ccoenen @thomseddon would you be able to approve https://github.com/maxisme/traefik-forward-auth/pull/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?

[ccoenen]

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.

[tomlankhorst]

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]

Hey @maxisme I would love to this merged. Please take in account this comment with proposal: https://github.com/thomseddon/traefik-forward-auth/pull/159#discussion_r590366372

Maybe @thomseddon could help us pushing this?

[hjalmarlucius]

@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