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

Save sub or user_id if not provided; remove extemporaneous ID token attributes #469

Merged
merged 1 commit into from
May 29, 2018

Conversation

joshcanhelp
Copy link
Contributor

  • Make sure we have both a user_id and sub saved for both login flows
  • Unset extra ID token attributes not required for profile data

@joshcanhelp joshcanhelp added this to the 3.6.0 milestone May 25, 2018

// Populate legacy user_id property, if not provided.
if ( ! isset( $decoded_token->user_id ) ) {
$decoded_token->user_id = $decoded_token->sub;
Copy link
Contributor

Choose a reason for hiding this comment

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

why? if you ensure you always have a sub you can make your code rely on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, standardizing between the two. Implicit has both and "make your code rely on that" is a little more involved than it might seem.

Copy link
Contributor

Choose a reason for hiding this comment

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

If this targets a new major I'd argue that you should keep only 1. sub in this case, since it's what the standard uses. But since there shouldn't be breaking changes on this minor release, let's keep both. Just remember for the next one this should be changed.

// Populate legacy userinfo property.
$decoded_token->user_id = $decoded_token->sub;
// Remove unneeded ID token attributes.
foreach ( array( 'iss', 'aud', 'iat', 'exp', 'nonce', 'clientID' ) as $attr ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the reason to remove them? how will you know if a token works for a given audience or if the token is still valid (without making a network request)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just saving userinfo, we don't use it as a token. I'm trying to get the information that we're saving to be more consistent between the two login flows. Also, this isn't saved as id_token so it's a bit confusing to use it as such.

@joshcanhelp joshcanhelp changed the title Save or if not provided; remove extemporaneous ID token sttributes Save sub or user_id if not provided; remove extemporaneous ID token attributes May 29, 2018
Copy link
Contributor

@lbalmaceda lbalmaceda left a comment

Choose a reason for hiding this comment

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

🇹🇩

@joshcanhelp joshcanhelp merged commit 6dc9a01 into dev May 29, 2018
@joshcanhelp joshcanhelp deleted the fix-id-token-attr-saved branch May 29, 2018 16:57
@joshcanhelp joshcanhelp mentioned this pull request Jun 5, 2018
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants