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 Tokens #90

Merged
merged 8 commits into from
Feb 13, 2022
Merged

Refresh Tokens #90

merged 8 commits into from
Feb 13, 2022

Conversation

dshanske
Copy link
Member

Tried to add in refresh tokens and expiration.

@omz13
Copy link

omz13 commented Sep 3, 2021

I'm not sure if this is mentioned elsewhere, in the section 'Refreshing an Access Token' perhaps mention that a refresh can be requested anytime at the discretion of the client; i.e. it does have to wait for an access code to expire before using its refresh token to request a new one.

@dshanske
Copy link
Member Author

dshanske commented Sep 3, 2021

Good idea...I was trying to summarize the refresh flow and need to cite other sources for guidance. But that one I think should be included

@dshanske dshanske requested a review from aaronpk September 6, 2021 06:06
Copy link

@omz13 omz13 left a comment

Choose a reason for hiding this comment

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

The phrase "or to obtain additional tokens" is a bit misleading... better as "or to obtain a new token" (because when refreshing you get a new token and the old one has its active state removed).

public/source/index.php Outdated Show resolved Hide resolved
<li><code>access_token</code> (required) - the OAuth 2.0 Bearer Token [[!RFC6750]].<li>
<li><code>me</code> (required) - the canonical user profile URL for the user this access token corresponds to.</li>
<li><code>profile</code> (optional) - the user's profile information as defined in <a href="#profile-information">Profile Information</a>.</li>
<li><code>expires_in</code> (recommended) - The lifetime in seconds of the access token. If omitted, the authorization server SHOULD provide the expiration time via other means or document the default value.</li>
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ if omitted, OAuth2 clients would expect this to not expire:

expires_in (recommended) If the access token expires, the server should reply with the duration of time the access token is granted for.

(via)

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct, but that's the definition of expires_in from OAuth2.

Copy link
Member

Choose a reason for hiding this comment

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

I would remove the "If omitted" clause. That really only makes sense when there are API docs for the service you can refer to. Technically it would still be true in IndieAuth, but there are basically no cases where it would happen so there's no need to call it out. IndieAuth is still extending OAuth so someone can refer to the original OAuth text if they really want to learn this.

public/source/index.php Outdated Show resolved Hide resolved
public/source/index.php Outdated Show resolved Hide resolved
&refresh_token=xxxxxxxx
') ?></pre>

<p>If valid and authorized, the authorization server issues an access token as noted in <a href="#access-token-response">Access Token Response</a>. If an authorization server chooses to issue a new refresh token, then the refresh token scope MUST be identical to the one included by the client in the request.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<p>If valid and authorized, the authorization server issues an access token as noted in <a href="#access-token-response">Access Token Response</a>. If an authorization server chooses to issue a new refresh token, then the refresh token scope MUST be identical to the one included by the client in the request.</p>
<p>If valid and authorized, the authorization server issues an access token, and optionally a fresh refresh token, as noted in <a href="#access-token-response">Access Token Response</a>. If an authorization server chooses to issue a new refresh token, then the refresh token scope MUST be identical to the one included by the client in the request. If an authorization server chooses to issue a new refresh token, then the previously used refresh token is expected to no longer be valid.</p>

Copy link
Member Author

Choose a reason for hiding this comment

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

That's not actually required in OAuth2, do we want to require it here? It's certainly recommended.

Copy link
Member

Choose a reason for hiding this comment

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

I would say "optionally a new refresh token"

You could copy the language from OAuth 2.1 around this which includes the recommendation of this refresh token rotation which actually came from the Security BCP

https://datatracker.ietf.org/doc/html/draft-ietf-oauth-v2-1-03#section-4.3.2

Copy link
Contributor

@jamietanna jamietanna left a comment

Choose a reason for hiding this comment

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

Few comments, but looking good 🙌

Copy link
Contributor

@jamietanna jamietanna left a comment

Choose a reason for hiding this comment

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

Also, should we be adding this to the CHANGES section, too?

dshanske and others added 3 commits September 11, 2021 08:10
@dshanske
Copy link
Member Author

Re the CHANGES section, that would be after we merge these, we'd add it.

@reiterate-app
Copy link

My comments on #94 (comment) also apply here. The grant_type parameter by itself is insufficient for an endpoint to determine what action to take in response to a POST request at the token endpoint.

@dshanske
Copy link
Member Author

dshanske commented Sep 18, 2021

@jamietanna In your implementation, you send client_id, which isn't in my PR.

@dshanske
Copy link
Member Author

Noting my description doesn't talk about client authentication, which is used in Oauth2's refresh, and needs discussion.

@jamietanna
Copy link
Contributor

Hmm, yes client_id is only required if client authentication is required, which we won't be doing for public (IndieAuth) clients.

I'll check if my server supports not sending it but agreed that can be optional 👍🏽

@dshanske
Copy link
Member Author

Hmm, yes client_id is only required if client authentication is required, which we won't be doing for public (IndieAuth) clients.

I'll check if my server supports not sending it but agreed that can be optional 👍🏽

I was thinking about this. It can be used to check to see if the client matches the original, but if you have a compromised refresh token, you probably have the client ID it came from.

@aaronpk
Copy link
Member

aaronpk commented Sep 23, 2021

The client_id parameter is required to be sent to the token endpoint if there is no client authentication. This is true both in OAuth 2.0 and 2.1:

The motivation for sending it in 2.0 is out of date, but it is still required and still can benefit the authorization server, such as being able to search a smaller index by first adding the client_id to the DB lookup. But you are correct that it doesn't add any additional security protection of the refresh token.

public/source/index.php Outdated Show resolved Hide resolved
@dshanske
Copy link
Member Author

@aaronpk @jamietanna I think I updated everything requested.

public/source/index.php Outdated Show resolved Hide resolved
Copy link
Contributor

@jamietanna jamietanna left a comment

Choose a reason for hiding this comment

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

Minor improvement, but LGTM otherwise - great stuff 👏

@dshanske dshanske added this to the October 2021 milestone Oct 13, 2021
@dshanske
Copy link
Member Author

Check to see if PR explicitly says that profile information is to be returned in flow.

<p>If the request is valid, then the token endpoint can generate an access token and return the appropriate response. The token response is a JSON [[!RFC7159]] object containing the OAuth 2.0 Bearer Token [[!RFC6750]], as well as a property <code>me</code>, containing the canonical user profile URL for the user this access token corresponds to, and, if the <code>profile</code> scope was requested, the property <code>profile</code> with the user's profile information as defined in <a href="#profile-information">Profile Information</a>. For example:</p>
<p>If the request is valid, then the token endpoint can generate an access token and return the appropriate response. The token response is a JSON [[!RFC7159]] object containing:</p>

<ul>
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation is mismatched from other elements above/below

Copy link
Contributor

@jamietanna jamietanna left a comment

Choose a reason for hiding this comment

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

Indentation aside, LGTM!

@aaronpk aaronpk merged commit 7643bea into main Feb 13, 2022
@aaronpk aaronpk deleted the expiration branch February 13, 2022 03:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants