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

Fix issues related to OAuth Resource owner password grant flow #4238

Merged
merged 17 commits into from Sep 11, 2019

Conversation

@vinayknl
Copy link
Contributor

commented Aug 29, 2019

Summary: When using CAS as OIDC provider, and when we want to use username/password for authentication, CAS was not working as expected.

When we have a service registered as Confidential client giving it a client_id and client_secret, we should be providing both when using grant_type as password.

https://tools.ietf.org/html/rfc6749#section-4.3.2

We found following issues which are fixed in this PR.

  1. When we supply username, password and client_id, CAS provides Access Token without checking for client_secret.

Expected behaviour is to return Unauthorized when the client_secret is provided for the registered service.

  1. When we supply username, password, client_id and client_secret, CAS provides Access Token but for profile of the client. i.e we see a behaviour of grant_type=client_credentials.

Expected behaviour is to return access token for the user authenticated by provided credentials.

As a fix for this, added a check in OAuth20ClientIdClientSecretAuthenticator.java to skip Client authentication if the grant_type is password. And in OAuth20UsernamePasswordAuthenticator.java, client_secret is always checked if the registered service is provided with one.

There are couple of other issues which are fixed related to this flow.

  1. If the OIDC scopes are not defined in registered service, then skip filtering for scopes and allow all OIDC scopes.
  2. Find registered service using client_id instead of service when creating UserProfile
  • Brief description of changes applied
  • Test cases for all modified changes, where applicable
  • The same pull request targetted at the master branch, if applicable
  • Any documentation on how to configure, test
  • Any possible limitations, side effects, etc: None
  • Reference any other pull requests that might be related: None

@apereocas-bot apereocas-bot added this to the 6.1.0-RC5 milestone Aug 29, 2019

vinayknl added 5 commits Aug 29, 2019
@codecov

This comment has been minimized.

Copy link

commented Aug 30, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@b7a1507). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #4238   +/-   ##
=========================================
  Coverage          ?   63.52%           
  Complexity        ?     7478           
=========================================
  Files             ?     1653           
  Lines             ?    36229           
  Branches          ?     3364           
=========================================
  Hits              ?    23015           
  Misses            ?    11020           
  Partials          ?     2194
Impacted Files Coverage Δ Complexity Δ
.../profile/DefaultOAuth20UserProfileDataCreator.java 94.73% <100%> (ø) 4 <1> (?)
...ator/OAuth20ClientIdClientSecretAuthenticator.java 100% <100%> (ø) 7 <0> (?)
...dc/profile/OidcProfileScopeToAttributesFilter.java 75.71% <100%> (ø) 13 <0> (?)
...nticator/OAuth20UsernamePasswordAuthenticator.java 88.37% <100%> (ø) 6 <0> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b7a1507...205b776. Read the comment docs.

@mmoayyed mmoayyed modified the milestones: 6.1.0-RC5, 6.1.0-RC6 Sep 2, 2019

vinayknl added 8 commits Sep 2, 2019
Merge branch 'master' of https://github.com/apereo/cas into fix-oidc-…
…password-grant-flow

# Conflicts:
#	support/cas-server-support-oauth-core-api/src/main/java/org/apereo/cas/support/oauth/authenticator/OAuth20UsernamePasswordAuthenticator.java
#	support/cas-server-support-oauth/src/test/java/org/apereo/cas/support/oauth/authenticator/OAuth20ClientIdClientSecretAuthenticatorTests.java
@vinayknl

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

@mmoayyed Can you please review this PR when you have some time ? Thanks

@mmoayyed

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

Sure. Apologies for the delay. Should be able to finalize before the weekend.

@mmoayyed

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

If the OIDC scopes are not defined in registered service, then skip filtering for scopes and allow all OIDC scopes.

This seems problematic; if someone has not defined scopes for the service, that would mean that the user does not wish to release anything to the relying party; thus the emptiness. An empty collection of scopes basically is a deny-all. Allowing all scopes will not work as that would be rather surprising behavior and inconsistent with the general attribute release policies in CAS; scopes must always be explicitly defined, as should all other attributes in all other protocol implementations.

Perhaps you can make this optional with some sort of flag, but even then, I think you'd be introducing additional complexity.

The rest looks pretty good, thank you!

@mmoayyed
Copy link
Member

left a comment

LGTM; Comments on the behavior of undefined OIDC scopes per service.

vinayknl added 2 commits Sep 11, 2019
@vinayknl

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

@mmoayyed Thanks for looking into this, and reverted change related to oidc service scopes.

@mmoayyed mmoayyed merged commit 129596c into apereo:master Sep 11, 2019

4 of 5 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Summary 2 potential rules
Details
WIP Ready for review
Details
license/cla Contributor License Agreement is signed.
Details
@vinayknl vinayknl referenced this pull request Sep 17, 2019
6 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.