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.

Sign up for GitHub

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

Jump to bottom

Fix authn delegation behavior #4644

Merged
merged 11 commits into from Jan 27, 2020
Merged

Fix authn delegation behavior #4644

merged 11 commits into from Jan 27, 2020

Conversation

leleuj
Copy link
Contributor

@leleuj leleuj commented Jan 22, 2020

If we force the authn delegation process on an existing SSO session, the current behavior is unappropriate: the credentials should not be put in the webflow for an existing session as they trigger a login process, erasing the previous authenticated user.

I also changed the visibility of two methods from private to protected for customisation purposes.

@claassistantio
Copy link

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@apereocas-bot apereocas-bot added this to the 6.2.0-RC3 milestone Jan 22, 2020
@codecov
Copy link

codecov bot commented Jan 23, 2020

Codecov Report

Merging #4644 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #4644   +/-   ##
=========================================
  Coverage     13.18%   13.18%           
  Complexity     2042     2042           
=========================================
  Files          2609     2609           
  Lines         53422    53422           
  Branches       4261     4261           
=========================================
  Hits           7044     7044           
  Misses        45979    45979           
  Partials        399      399
Flag Coverage Δ Complexity Δ
#Cassandra 100% <0%> (ø) 0% <0%> (ø) ⬇️
#CosmosDb 100% <0%> (ø) 0% <0%> (ø) ⬇️
#CouchDb 100% <0%> (ø) 0% <0%> (ø) ⬇️
#Couchbase 100% <0%> (ø) 0% <0%> (ø) ⬇️
#DynamoDb 100% <0%> (ø) 0% <0%> (ø) ⬇️
#FileSystem 10.69% <0%> (ø) 1543% <0%> (ø) ⬇️
#Groovy 100% <0%> (ø) 0% <0%> (ø) ⬇️
#Ignite 100% <0%> (ø) 0% <0%> (ø) ⬇️
#InfluxDb 100% <0%> (ø) 0% <0%> (ø) ⬇️
#JDBC 100% <0%> (ø) 0% <0%> (ø) ⬇️
#JMS 0% <0%> (ø) 0% <0%> (ø) ⬇️
#LDAP 100% <0%> (ø) 0% <0%> (ø) ⬇️
#MAIL 100% <0%> (ø) 0% <0%> (ø) ⬇️
#MSSQL 100% <0%> (ø) 0% <0%> (ø) ⬇️
#MariaDb 100% <0%> (ø) 0% <0%> (ø) ⬇️
#Memcached 100% <0%> (ø) 0% <0%> (ø) ⬇️
#MongoDb 100% <0%> (ø) 0% <0%> (ø) ⬇️
#MySQL 100% <0%> (ø) 0% <0%> (ø) ⬇️
#OAUTH 100% <0%> (ø) 0% <0%> (ø) ⬇️
#OAUTHUMA 100% <0%> (ø) 0% <0%> (ø) ⬇️
#OIDC 100% <0%> (ø) 0% <0%> (ø) ⬇️
#Oracle 100% <0%> (ø) 0% <0%> (ø) ⬇️
#PostgreSQL 100% <0%> (ø) 0% <0%> (ø) ⬇️
#REST 100% <0%> (ø) 0% <0%> (ø) ⬇️
#Radius 100% <0%> (ø) 0% <0%> (ø) ⬇️
#Redis 10.46% <0%> (ø) 1592% <0%> (ø) ⬇️
#SAML 100% <0%> (ø) 0% <0%> (ø) ⬇️
#Simple 100% <0%> (ø) 0% <0%> (ø) ⬇️
#ZooKeeper 100% <0%> (ø) 0% <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 5a5802d...37a1d56. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Jan 23, 2020

Codecov Report

Merging #4644 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #4644   +/-   ##
=========================================
  Coverage     13.18%   13.18%           
  Complexity     2042     2042           
=========================================
  Files          2609     2609           
  Lines         53422    53422           
  Branches       4261     4261           
=========================================
  Hits           7044     7044           
  Misses        45979    45979           
  Partials        399      399
Flag Coverage Δ Complexity Δ
#Cassandra 100% <0%> (ø) 0% <0%> (ø) ⬇️
#CosmosDb 100% <0%> (ø) 0% <0%> (ø) ⬇️
#CouchDb 100% <0%> (ø) 0% <0%> (ø) ⬇️
#Couchbase 100% <0%> (ø) 0% <0%> (ø) ⬇️
#DynamoDb 100% <0%> (ø) 0% <0%> (ø) ⬇️
#FileSystem 10.69% <0%> (ø) 1543% <0%> (ø) ⬇️
#Groovy 100% <0%> (ø) 0% <0%> (ø) ⬇️
#Ignite 100% <0%> (ø) 0% <0%> (ø) ⬇️
#InfluxDb 100% <0%> (ø) 0% <0%> (ø) ⬇️
#JDBC 100% <0%> (ø) 0% <0%> (ø) ⬇️
#JMS 0% <0%> (ø) 0% <0%> (ø) ⬇️
#LDAP 100% <0%> (ø) 0% <0%> (ø) ⬇️
#MAIL 100% <0%> (ø) 0% <0%> (ø) ⬇️
#MSSQL 100% <0%> (ø) 0% <0%> (ø) ⬇️
#MariaDb 100% <0%> (ø) 0% <0%> (ø) ⬇️
#Memcached 100% <0%> (ø) 0% <0%> (ø) ⬇️
#MongoDb 100% <0%> (ø) 0% <0%> (ø) ⬇️
#MySQL 100% <0%> (ø) 0% <0%> (ø) ⬇️
#OAUTH 100% <0%> (ø) 0% <0%> (ø) ⬇️
#OAUTHUMA 100% <0%> (ø) 0% <0%> (ø) ⬇️
#OIDC 100% <0%> (ø) 0% <0%> (ø) ⬇️
#Oracle 100% <0%> (ø) 0% <0%> (ø) ⬇️
#PostgreSQL 100% <0%> (ø) 0% <0%> (ø) ⬇️
#REST 100% <0%> (ø) 0% <0%> (ø) ⬇️
#Radius 100% <0%> (ø) 0% <0%> (ø) ⬇️
#Redis 10.46% <0%> (ø) 1592% <0%> (ø) ⬇️
#SAML 100% <0%> (ø) 0% <0%> (ø) ⬇️
#Simple 100% <0%> (ø) 0% <0%> (ø) ⬇️
#ZooKeeper 100% <0%> (ø) 0% <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 5a5802d...37a1d56. Read the comment docs.

@mmoayyed
Copy link
Member

I think I may have found a problem with your patch. Could you try the following scenario:

Configure CAS to act as an OAUTH server (or SAML2 IDP server)
Start with an OAUTH client or SAML2 SP
On CAS login, choose delegated authn via pac4j
Do AUP next
Return back to the OAUTH client (SAML2 SP) and you should be in successfully.

I think the patch breaks the final redirect back to the client because it gets confused on how to resolve the service url.

These lines likely should be put back:

        val resolvedService = WebUtils.getService(argumentExtractors, context);
        WebUtils.putServiceIntoFlowScope(context, resolvedService);
        val registeredService = servicesManager.findServiceBy(resolvedService);
        WebUtils.putRegisteredService(context, registeredService);

@leleuj
Copy link
Contributor Author

leleuj commented Jan 27, 2020

@mmoayyed Thanks for the feedback, I will check that today. BTW, I saw that the build has failed because of a PMD check or something for the cas-server-support-x509-core module, you may want to take a look at it...

@leleuj
Copy link
Contributor Author

leleuj commented Jan 27, 2020

@mmoayyed
I have tested both scenarios:

  • SAML server support + authn delegation + AUP
  • OAuth server support + authn delegation + AUP

In both cases, I haven't been able to reproduce your issue. In fact, the prepareRequestContextForSingleSignOn method I removed can never be called in these flows as there is no existing SSO session. The service is properly restored and resolved in the populateContextWithService method.

Maybe I'm missing something in my tests...

@leleuj
Copy link
Contributor Author

leleuj commented Jan 27, 2020

I just saw this PR: #4643 merged. Merging this one also...

@leleuj leleuj merged commit 7c46161 into apereo:master Jan 27, 2020
@leleuj leleuj deleted the authdelegate branch January 27, 2020 14:26
mmoayyed added a commit to mmoayyed/cas that referenced this pull request Aug 18, 2020
@mmoayyed
Merge branch 'master' into webauthn
b0e5b2d 
* master:
  minor dependency bump
  doc updates
  break apart passwordless jpa/ldap modules
  More debug trace instrumentation (apereo#4658)
  allow audit actions to be excluded combine passwordless with mfa fix tests
  OAuth20TokenAuthorizationResponseBuilder returns state and nonc… (apereo#4654)
  Extract issuer dn from certificate as x509 attribute (apereo#4655)
  Fix authn delegation behavior (apereo#4644)
  working on mfa flow orchestration
  clean up tests
  clean up
  clean up
  passwordless webflow module support passwordless with ldap
  clean up tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Authentication Delegated Authentication Pac4J
Projects
None yet
Milestone
6.2.0-RC3
4 participants
@leleuj @claassistantio @mmoayyed @apereocas-bot