Skip to content

fix issue with issuer and google oidc #524

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

Closed
wants to merge 3 commits into from

Conversation

cgawron
Copy link

@cgawron cgawron commented Feb 23, 2019

(iss in the token contains 'accounts.google.com', i.e. no 'https://')

(iss in the token contains 'accounts.google.com', i.e. no 'https://')
@cgawron
Copy link
Author

cgawron commented Feb 23, 2019

Fixes #523

@jeroenheijmans
Copy link
Collaborator

jeroenheijmans commented Feb 24, 2019

I presume you meant "Fixes #523"?

To comment on the change itself, I see a few problems:

  • You've changed a lot of lines with whitespace only, I suggest reverting all of them because it will cause a lot of pain either in merging or for others in PR's
  • The comment will be very unclear to future readers: it seems to be about the entire if condition or perhaps even the entire block
  • This behavior should be opt in via configuration, as it deviates explicitly from the OpenID spec AFAICT

I do think the idea is okay though, and I guess for better or for worse, Google is a big enough player to warrant such a workaround/hack for specific situations.

Reverting due to unnecessary whitespace changes.

This reverts commit 535b4fe.
@cgawron
Copy link
Author

cgawron commented Feb 24, 2019

I have reverted the whitespace changes (sorry for that) and changed the logic so that it only applies to accounts.google.com.
Regarding opt in: I would prefer restricting the additional condition to accounts.google.com rather than adding an additional option (unless there are other issuers for which this is needed - which I'm not aware of).
@jeroenheijmans What else do I need to do so this PR can be merged?

@jeroenheijmans
Copy link
Collaborator

Was pinged in another PR, and just realized: perhaps #527 is a more generic/better way to solve the same thing.

@manfredsteyer
Copy link
Owner

Thanks for this PR. Let's go with #527

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