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

Rename JWT claims for JWT Access Tokens: #338 #460

Merged

Conversation

jaredk2g
Copy link

Hey all,

Thank you for creating such a fantastic library. It has been very useful to me. This pull request is my attempt to finish addressing #338 for JWT Access Tokens. A couple of notes here:

  • renamed client_id to aud
  • renamed expires to exp
  • renamed user_id to sub
  • added the iss parameter from an optional issuer configuration value
  • added the iat parameter for the time the JWT access token was issued

I also added a method that maps the decoded access token from the JWT claims format to the format expected by other parts of this library. Hope this is helpful!

  • Jared

// converts a JWT access token into an OAuth2-friendly format
protected function convertJwtToOauth2($tokenData)
{
$keyMapping = [
Copy link
Owner

Choose a reason for hiding this comment

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

Unfortunately, we will have to use the php5.3-compatible array(...) syntax here

@bshaffer
Copy link
Owner

This is a great change! Thank you for doing this. I have one comment, and it would also be great to write tests for this. Otherwise, 👍

@jaredk2g
Copy link
Author

Both excellent points. My bad on the array syntax slip. I will make those changes and resubmit.

@jaredk2g
Copy link
Author

I added a test that verifies the JWT claims are set properly in the resulting access token from createAccessToken()

@@ -57,4 +58,23 @@ public function setAccessToken($oauth_token, $client_id, $user_id, $expires, $sc
return $this->tokenStorage->setAccessToken($oauth_token, $client_id, $user_id, $expires, $scope);
}
}

// converts a JWT access token into an OAuth2-friendly format
protected function convertJwtToOauth2($tokenData)
Copy link
Owner

Choose a reason for hiding this comment

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

This is really picky, but for consistency with the rest of the library, this function should be convertJwtToOAuth2 (uppercase the A in OAuth2)

@bshaffer
Copy link
Owner

This PR brings a tear to my eye. If you could fix my single suggestion and rebase it into a single commit for a cleaner commit history (use rebase -i HEAD~4), I will merge it post-haste 🏇 !

@jaredk2g jaredk2g force-pushed the issue-338-rename-jwt-access-token-claims branch from 1e71643 to ee912be Compare September 16, 2014 17:28
@bshaffer
Copy link
Owner

gah, your only mistake! My name is Brent! :)

bshaffer added a commit that referenced this pull request Sep 16, 2014
…oken-claims

Rename JWT claims for JWT Access Tokens: #338
@bshaffer bshaffer merged commit 82caca6 into bshaffer:develop Sep 16, 2014
@jaredk2g
Copy link
Author

Ah, sorry Brent! I must have picked up that other name in the issues.

@bshaffer
Copy link
Owner

I forgive you on account of your awesome PR

@Maks3w
Copy link
Contributor

Maks3w commented Jan 27, 2015

This changes need to be forwarded to https://github.com/bshaffer/oauth2-server-php-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.

3 participants