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

Delegated authentication: fix the DistributedJ2ESessionStore #4427

Open
wants to merge 2 commits into
base: master
from

Conversation

@leleuj
Copy link
Contributor

leleuj commented Nov 8, 2019

Currently, the DistributedJ2ESessionStore does not work when you have more than one node.

Let's say that a session is generated on a first node, when you reach the second node, retrieving the session via the Tomcat session manager does not work. A JSESSION_ID cookie value is actually retrieved, but as there is no HTTP session associated, it is discarded and a new session with a new identifier is created.

The only way to deal with that is to directly read the cookie value.

@apereocas-bot apereocas-bot added this to the 6.2.0-RC1 milestone Nov 8, 2019
@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Nov 8, 2019

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.

@hdeadman

This comment has been minimized.

Copy link
Member

hdeadman commented Nov 8, 2019

You can have session ids configured with a name other than JSESSIONID, so shouldn't that cookie name be configurable?

<session-config>
    <cookie-config>
        <name>APP_JSESSIONID</name>
    </cookie-config>
</session-config>

Also, wouldn't using the DistributedJ2ESessionStore assume that you had session replication (or centralized session persistence) configured on the application server?

@leleuj

This comment has been minimized.

Copy link
Contributor Author

leleuj commented Nov 8, 2019

  1. Yes, the cookie name should be configurable. I will change that.

  2. Good point but no: if you have HTTP session replication, you can just use the JEESessionStore (which only relies on the HTTP session). Using the DistributedJ2ESessionStore (session data in
    ticket) assumes that CAS will handle the session replication.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Nov 8, 2019

Codecov Report

Merging #4427 into master will increase coverage by 1.49%.
The diff coverage is 50%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #4427      +/-   ##
============================================
+ Coverage     58.46%   59.95%   +1.49%     
- Complexity     9555     9774     +219     
============================================
  Files          2580     2580              
  Lines         52446    52452       +6     
  Branches       4168     4170       +2     
============================================
+ Hits          30660    31450     +790     
+ Misses        19285    18473     -812     
- Partials       2501     2529      +28
Impacted Files Coverage Δ Complexity Δ
.../integration/pac4j/DistributedJ2ESessionStore.java 80.39% <50%> (-6.28%) 12 <2> (ø)
...ices/DefaultRegisteredServiceExpirationPolicy.java 46.15% <0%> (-30.77%) 4% <0%> (-1%)
...ava/org/apereo/cas/util/io/PathWatcherService.java 82.25% <0%> (-3.23%) 12% <0%> (-3%)
...ereo/cas/monitor/AbstractCacheHealthIndicator.java 70.58% <0%> (-2.95%) 8% <0%> (-1%)
...g/apereo/cas/services/AbstractServicesManager.java 74.57% <0%> (-2.55%) 32% <0%> (-1%)
.../cas/cassandra/DefaultCassandraSessionFactory.java 61.11% <0%> (-1.86%) 5% <0%> (-1%)
...org/apereo/cas/config/CasOAuth20Configuration.java 91.18% <0%> (+0.38%) 67% <0%> (+1%) ⬆️
...points/OAuth20IntrospectionEndpointController.java 64.44% <0%> (+1.11%) 7% <0%> (ø) ⬇️
...ereo/cas/util/cipher/BaseStringCipherExecutor.java 83.19% <0%> (+1.68%) 36% <0%> (ø) ⬇️
...se/OAuth20DefaultAccessTokenResponseGenerator.java 91.3% <0%> (+2.17%) 15% <0%> (+1%) ⬆️
... and 66 more

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 19381cb...e2ef926. Read the comment docs.

@mmoayyed mmoayyed changed the title Fix the DistributedJ2ESessionStore Delegated authentication: fix the DistributedJ2ESessionStore Nov 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.