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

[Microsoft] fix reading OpenID Connect token responses #243

Merged
merged 1 commit into from
May 14, 2019

Conversation

ccutrer
Copy link
Contributor

@ccutrer ccutrer commented Mar 10, 2016

when using Microsoft's OpenID Connect service (as documented at
https://azure.microsoft.com/en-us/documentation/articles/active-directory-v2-protocols/)
if you only request the openid scope, Microsoft only returns an id_token,
with no access_token. so treat that as a valid response.

@@ -136,8 +136,11 @@ def get_token(params, access_token_opts = {}, access_token_class = AccessToken)
opts[:params] = params
end
response = request(options[:token_method], token_url, opts)
error = Error.new(response)
raise(error) if options[:raise_errors] && !(response.parsed.is_a?(Hash) && response.parsed['access_token'])
if options[:raise_errors] && !(response.parsed.is_a?(Hash) &&

Choose a reason for hiding this comment

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

This line is a little tough to read. I'd change to something like this:

response_contains_token = response.parsed.is_a?(Hash) &&
  (response.parsed['access_token'] || response.parsed['id_token'])

error = Error.new(response)
raise(error) if options[:raise_errors] && !(response_contains_token)

Also this method now breaks the lint check, which is failing the build. In a method above there's a comment to disable a few extra Rubocops, I'd add the same comment to this method def as well:

# rubocop:disable CyclomaticComplexity, MethodLength, Metrics/AbcSize

@jeffcarbs
Copy link

I've run into the same issue. I left some comments to help fix the build and then hopefully this could get some attention from 👍

@ccutrer
Copy link
Contributor Author

ccutrer commented May 23, 2016

I've updated the PR with your suggestions.

@xtagon
Copy link

xtagon commented Jan 4, 2018

@ccutrer Are you still interested in getting this merged? Looks like there are just some conflicts now.

@xtagon xtagon added the bug label Jan 4, 2018
@pboling pboling self-assigned this Jan 8, 2018
@pboling pboling changed the title fix reading OpenID Connect token responses from Microsoft [Microsoft] fix reading OpenID Connect token responses Mar 7, 2018
@natebird
Copy link

natebird commented May 13, 2019

Any movement on resolving the conflicts so this can be merged @ccutrer?

@ccutrer
Copy link
Contributor Author

ccutrer commented May 13, 2019

I'd almost forgotten this PR existed. I've just been updating our monkey patch in our app with each bump of this gem. I'll update the PR

@ccutrer
Copy link
Contributor Author

ccutrer commented May 13, 2019

PR updated with conflicts resolved, and specs passing.

@coveralls
Copy link

coveralls commented May 13, 2019

Pull Request Test Coverage Report for Build 827

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at ?%

Totals Coverage Status
Change from base Build 826: 0.0%
Covered Lines:
Relevant Lines: 0

💛 - Coveralls

2 similar comments
@coveralls
Copy link

Pull Request Test Coverage Report for Build 827

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at ?%

Totals Coverage Status
Change from base Build 826: 0.0%
Covered Lines:
Relevant Lines: 0

💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 827

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at ?%

Totals Coverage Status
Change from base Build 826: 0.0%
Covered Lines:
Relevant Lines: 0

💛 - Coveralls

@coveralls
Copy link

coveralls commented May 13, 2019

Pull Request Test Coverage Report for Build 830

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at ?%

Totals Coverage Status
Change from base Build 826: 0.0%
Covered Lines:
Relevant Lines: 0

💛 - Coveralls

@ccutrer ccutrer force-pushed the id_token_for_microsoft branch 2 times, most recently from fff5076 to 142a938 Compare May 13, 2019 15:47
when using Microsoft's OpenID Connect service (as documented at
https://azure.microsoft.com/en-us/documentation/articles/active-directory-v2-protocols/)
if you only request the openid scope, Microsoft only returns an id_token,
with no access_token. so treat that as a valid response.
Copy link
Member

@pboling pboling left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for updating!

@pboling pboling merged commit 78dd83d into oauth-xx:master May 14, 2019
@jonspalmer
Copy link
Contributor

I know this is an old PR but is there any advice on how to use Microsoft responses that only hav id_token in the. This fix make that a valid response but the AccessToken class still expects the access_token in the response hash.

What's the fix? Is it to subclass AccessToken ?

@anvox
Copy link
Member

anvox commented Jul 13, 2020

Thansk @jonspalmer I afraid extending AccessToken is the only option for now. I see you found #511 already, we could move conversation on it for a completely solution for this.

@jonspalmer
Copy link
Contributor

Happy to move it there. I have lots of questions :D perhaps a few suggestions too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants