qvest-digital / loginsrv

JWT login microservice with plugable backends such as OAuth2, Google, Github, htpasswd, osiam, ..
MIT License
1.92k stars 150 forks source link

Fix #165 by using Authorization header instead #166

Closed jackodsteel closed 4 years ago

jackodsteel commented 4 years ago

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 commented 4 years ago

Coverage Status

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

jackodsteel commented 4 years ago

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 commented 4 years ago

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

sbamin commented 4 years ago

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 commented 4 years ago

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.