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

Fix #165 by using Authorization header instead #166

Merged
merged 2 commits into from
Mar 12, 2020

Conversation

jackodsteel
Copy link
Contributor

@jackodsteel jackodsteel commented Feb 9, 2020

Fixes #165 by using the Authorization header instead of the access_token query param.

To test:

  1. Create a new GitHub OAuth App at https://github.com/settings/developers

  2. Run a server with this new version first, and login with GitHub. Verify that you did not get a deprecation notice email

  3. Run the current master server, and login with GitHub. You'll see you did get a deprecation notice for the current server

@coveralls
Copy link

coveralls commented Feb 9, 2020

Coverage Status

Coverage increased (+0.008%) to 91.431% when pulling 4d8e38a on jackodsteel:fix-165-deprecated-auth-param into b759a6a on tarent:master.

@g-w g-w self-requested a review February 10, 2020 10:58
Copy link
Contributor

@g-w g-w left a comment

Choose a reason for hiding this comment

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

Hello @jackodsteel,

thank you for your contribution!

Could you please have a look into my comment? Why did you not use the basic auth scheme as suggested by github?

oauth2/github.go Outdated Show resolved Hide resolved
@jackodsteel
Copy link
Contributor Author

Why did you not use the basic auth scheme as suggested by github?

I followed the example flow as described: https://developer.github.com/apps/building-oauth-apps/authorizing-oauth-apps/#3-use-the-access-token-to-access-the-api

I think the basic scheme is really only intended to be used if your app only supports basic auth, as per:

This approach is useful if your tools only support Basic Authentication

Since we can control the Authorization header we should use the token version of the header.

@jackodsteel jackodsteel requested a review from g-w February 22, 2020 06:16
@jackodsteel
Copy link
Contributor Author

Bump @g-w, should be good to go now

@g-w g-w merged commit 3915364 into qvest-digital:master Mar 12, 2020
@jackodsteel jackodsteel deleted the fix-165-deprecated-auth-param branch March 14, 2020 14:18
@sbamin
Copy link

sbamin commented Jun 22, 2020

Hi @jackodsteel, wonder if this fix was also applied to caddy plugin. I am using caddy v1.0.4 but still get an email from github about deprecation of access_token.

Thanks

login {
	github client_id=XXXXXX,client_secret=XXXXXX
	redirect_check_referer false
	cookie_domain beta.example.com
	success_url /login
	logout_url /login
  	jwt_expiry 24h
  	cookie_expiry 2h
}

jwt {
	path /
	redirect https://beta.example.com:4000/login?backTo=https%3A%2F%2F{host}{rewrite_uri_escaped}
	allow sub foo
}

@jackodsteel
Copy link
Contributor Author

Hi @jackodsteel, wonder if this fix was also applied to caddy plugin. I am using caddy v1.0.4 but still get an email from github about deprecation of access_token.

Hey @sbamin. This fix does apply to the Caddy plugin, however it requires the maintainer to release a new version with the fix, which hasn't happened yet. See the releases page where 1.3.1 is the latest, which was pre this PR.

It's up to @smancke or other maintainers to do a new release, which I don't know their plans.

Otherwise if you have programming experience you can compile the new version straight from master based off these docs.

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

Successfully merging this pull request may close these issues.

GitHub OAuth2 login deprecation notice: using the access_token query parameter is deprecated.
4 participants