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

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

Open
wants to merge 27 commits into
base: master
Choose a base branch
from

Conversation

maxisme
Copy link

@maxisme maxisme commented Aug 1, 2020

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
Copy link
Owner

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

@thomseddon thomseddon added enhancement New feature or request under review labels Aug 22, 2020
remove UserID type
rename comma delimited to comma separated
@maxisme
Copy link
Author

maxisme commented Aug 22, 2020

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
Copy link

ccoenen commented Oct 8, 2020

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
Copy link
Author

maxisme commented Oct 8, 2020

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
Copy link

ccoenen commented Oct 8, 2020

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

README.md Outdated
--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]
Copy link

Choose a reason for hiding this comment

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

I believe this should now also be

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

Copy link
Author

Choose a reason for hiding this comment

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

Nice, good catch!

Copy link
Author

Choose a reason for hiding this comment

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

fixed

Dockerfile Outdated
@@ -1,4 +1,4 @@
FROM golang:1.13-alpine as builder
FROM golang:1.14-alpine as builder
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

reverted

@ccoenen
Copy link

ccoenen commented Oct 8, 2020

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
Copy link

ccoenen commented Dec 1, 2020

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

@maxisme
Copy link
Author

maxisme commented Jan 12, 2021

@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?

@ccoenen
Copy link

ccoenen commented Feb 6, 2021

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.

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)

Choose a reason for hiding this comment

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

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

@tomlankhorst
Copy link

tomlankhorst commented Oct 17, 2021

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
Copy link

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
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request under review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants