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

Refresh Token Grant - scope limitation and additional access tokens #25

Closed
cziegenberg opened this issue Mar 22, 2013 · 8 comments
Closed
Assignees
Milestone

Comments

@cziegenberg
Copy link
Contributor

Following the RFC, the implementation of the Refresh Token Grant isn't correct:

"Refresh tokens are issued [...] to obtain additional access tokens with identical or narrower scope." http://tools.ietf.org/html/rfc6749#section-1.5

  1. So it should be possible to change (limit) the scope of the existing session. At the moment the scope parameter is not used in the Refresh Token Grant. For example this is required for security reasons when you use multiple resource servers and don't want to use the same scope on each server.

  2. The current implementation doesn't create an "additional" access token, but replaces the existing one. I checked the oauth mailing list (http://list-archives.org/2012/10/31/oauth-ietf-org/oauth-wg-access-tokens-refresh-tokens-of-different-scopes/f/4229413633) and "additional" really means additional here, so you should be able to have multiple valid access tokens at the same time. But I'm not sure when to invalidate them - when creating an access token using another grant type?

@alexbilbie
Copy link
Contributor

  1. This is easily fixed

  2. I think the refresh token grant is difficult to work with anyway because both the server and the client have to maintain session state, but having multiple sessions for each user/client ID for each client with different TTLs and different scopes is just asking for trouble.

The reason I created this library in the first place is because so many people have been implementing OAuth incorrectly and inconsistently and I think this is one example where perhaps deviating from the spec slightly is the best course of action

@cziegenberg
Copy link
Contributor Author

The problem is, that point 1 depends on point 2 - you have a set of allowed scopes and can reduce them, but after that you won't have the chance to create a new access token with the original scopes. You need to keep it saved or the client will only have two options - always use full rights (insecure) or ask the user again for his credentials (bad idea).

I tried to understand point 2 and checked the Google implementation. They do it as expected: It's possible to create multiple access tokens using the refresh token (they say that the number of tokens is limited, which is a good idea). You only get a refresh token with the main access token request and this can theoretically stay in the database forever (independent of the expiration time).

I don't think it's too difficult:

  1. Create an "normal" access token and refresh token. -> One entry in the session table.
  2. Create a new access token using the refresh token. -> An additional entry in the session table, possible with the current table structure. You only have to call the "createSession" method and leave out the "deleteSession" method. The only problem is, that you create multiple sessions and - as far as I understand it - the access tokens generated with the refresh token should depend on the main access token - so they should share the same session id.
  3. Destroy all access tokens when a "normal" access token is generated. -> Automatically done with "deleteSession".

BUT I would propose a general correction of the table structure to avoid the mentioned problem and to make it a bit clearer, easier to implement/customize and optimize the performance:

  1. Use one main session table with all basic data (without fields that are only required by special grant types you perhaps never use).
  2. Add tables for the fields used by special grant types. So you only create an entry if you have data and avoid the massive use of NULLable columns (which isn't a good idea from the performance view). You can even delete the tables, which are not required for the own implementation.
  3. For the library the changes would be minimal - as far as I can see, only the call of "associateScope" would have to be changed, because it's not bound to the session id, but the generated access token.

I can work on that and provide optimized scripts for the database - MySQL, SqlServer, Oracle, and perhaps Postgres (learning at the moment) - and a Session class for your server example.

Attached an optimized structure, as I would propose it. I'm not sure about the roles of the fields "stage", "first_requested" and "last_updated". Are they really required? If so, only for specific scopes?

[Old structure replaced by new one, see next comment]

@cziegenberg
Copy link
Contributor Author

I optimized the structure again and created a test implementation.

oauth

The Refresh Token Grant now works as expected and theoretically it's not necessary to change the scripts - I only had to rename some parameters (because the session id is now the session token id in some cases) and this renaming should be done also in the grant scripts.

I'm still not sure about the role of the "stage" field. Currently it seems to be really used with the Auth Code Grant only...

@alexbilbie
Copy link
Contributor

Can you run me off a MySQL dump of this new structure please

@cziegenberg
Copy link
Contributor Author

I just sent you the DB structure and a Session Storage example via mail.

@cziegenberg
Copy link
Contributor Author

I just found another problem with the current implementation:

RFC, section 1.5:

  • "Refresh tokens are issued to the client by the authorization server and are used to obtain a new access token when the current access token becomes invalid or expires, or to obtain additional access tokens with identical or narrower scope"

RFC, section 6:

  • "The authorization server MAY issue a new refresh token, in which case the client MUST discard the old refresh token and replace it with the new refresh token."
  • "The authorization server MAY revoke the old refresh token after issuing a new refresh token to the client."
  • "If a new refresh token is issued, the refresh token scope MUST be identical to that of the refresh token included by the client in the request."

I modified the Session Storage script (sent to you via mail) to handle the creation of new refresh tokens in accordance with the RFC, but the flow of Refresh Token Grant class also needs to be modified.

The Refresh Token Grant must differ between two cases - issue a new refresh token when a new access token is created, or not. This defines the further steps in the flow and the possibilities on the client side. Therefore it must be possible to define the preferred behavior for this Grant:

RefreshToken::useRefreshTokenRotation(true|false)

If TRUE:

  • Return error, if scope parameter set (or if it's not identical to the prior scope).
  • Check credentials and refresh token.
  • Create a new access token.
  • Revoke the old refresh token (as the client MUST replace it anyway in this case).
  • Create a new refresh token.
  • Return the access token and the refresh token.

If FALSE:

  • The scope parameter is optional and can be used to limit the prior scope. If not set, the prior scope is used.
  • Check credentials and refresh token.
  • Create a new access token.
  • The old refresh token remains valid.
  • Return the access token only.

@cziegenberg
Copy link
Contributor Author

I updated the DB structure and the Session Storage class once again, due to the following points...

Security problems:

  1. "A maximum authorization code lifetime of 10 minutes is RECOMMENDED." (RFC, section 4.1.2.) This is not implemented at the moment (could be perhaps handled using the current timestamps, but then it should be renamed and moved to the auth code table). I moved the timestamp and renamed it, so it's purpose is a bit clearer. And I also added the necessary checks to the Session Storage Example.

  2. "The client MUST NOT use the authorization code more than once. If an authorization code is used more than once, the authorization server MUST deny the request and SHOULD revoke (when possible) all tokens previously issued based on that authorization code." (RFC, section 4.1.2.) This is not implemented at the moment. To detect multiple usages of the auth code it must not be deleted. Also the DB structure was missing an assignment of issued tokens to the auth code - to invalidate them as described. (This assignment also replaces the previous "stage" field, because an existing assignment means "granted".)

Other problems:

  1. As a result of the Client Credentials Grant correction (no refresh token allowed), it wasn't possible anymore to create multiple valid tokens with this grant (which is required if you want to access different resource servers with only the required scope for security reasons). I modified the table structure (removed unique key in oauth_session) and the Session Storage script to allow this.

  2. There was an error in the Auth Code handling in the last version of my Session Storage class.

My changes in the Session Storage class:

createSession

  • removed stage parameter (only used for Auth Code Grant and solved more securely, see below); change of the SessionInterface required
  • removed saving of timestamps (not required, can be done in customized implementations if needed)
  • added expiration time for auth code (as recommended in RFC); change of the SessionInterface recommended

updateSession

  • removed unnecessary parameters ("authCode", "stage"); change of the SessionInterface required
  • removed the updates of the timestamps in the session table
  • corrected the handling of the auth code entry (error in my previous script)
  • added assignment of the generated token to the auth code entry (required for security reasons when validating the auth code)

validateAuthCode

  • check for codes added, that have been already used
  • added deletion of previously issued tokens, for the case that the auth code is used multiple times (important security feature)
  • check for expired auth codes added

deleteSession

  • differ between the owner type to delete only expired sessions when the Client Credentials Grant is used.

I sent you the updated files via mail...

oauth

@alexbilbie
Copy link
Contributor

I've implemented all of this in v2.0 and significantly simplified the usage

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

No branches or pull requests

3 participants