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

Single logout breaks with multiple sessions #83

Open
beheh opened this issue Jul 8, 2019 · 2 comments
Open

Single logout breaks with multiple sessions #83

beheh opened this issue Jul 8, 2019 · 2 comments
Labels

Comments

@beheh
Copy link

beheh commented Jul 8, 2019

Right now, single logout will assemble a list of recent ServiceTickets for the current user which it then invalidates one-by-one. It uses the extremely basic check to only look at tickets created for the user since last login:

def request_sign_out(self, user):
"""
Send a single logout request to each service accessed by a
specified user. This is called at logout when single logout
is enabled.
If requests-futures is installed, asynchronous requests will
be sent. Otherwise, synchronous requests will be sent.
"""
session = Session()
for ticket in self.filter(user=user, consumed__gte=user.last_login):
ticket.request_sign_out(session=session)

This logic breaks as soon as the user signs in from two devices or browsers. When signing in to the second device, the user irreversibly overwrites their last login timestamp. Even if the session based on the first ticket signs out it will only invalidate the second ticket, because the first one was issued before the last login.

This seems like really bad behaviour from mama_cas, as it's quite to likely to miss tickets to invalidate, making the logout process unreliable.


In our application we have adjusted the signout logic as follows:

  • We use a maximum session lifetime of a few hours for all service sessions created by tickets
  • When logging out, we search for all the tickets:
    • that belong to the user signing out
    • that were issued within the (largest) maximum session lifetime

This ensures that all "child" sessions for this user have either been captured because they were from within the last few hours, or we are sure they have expired at the service because their lifetime is limited.

@evilscientress
Copy link

I just discovered the same behavior and agree with @beheh that current behavior is quite bad.

evilscientress added a commit to evilscientress/django-mama-cas that referenced this issue Jul 23, 2019
This addresses jbittel#83 and is a possible fix, but not ideal. The better
way to address the issue would be to track accesss services per session
and then send targeted single sign out requests based on this information.

Signed-off-by: Jenny Danzmayr <mail@evilscientress.at>
evilscientress added a commit to evilscientress/django-mama-cas that referenced this issue Jul 23, 2019
This addresses jbittel#83 and is a possible fix, but not ideal. The better
way to address the issue would be to track accesss services per session
and then send targeted single sign out requests based on this information.

Signed-off-by: Jenny Danzmayr <mail@evilscientress.at>
evilscientress added a commit to evilscientress/django-mama-cas that referenced this issue Jul 23, 2019
This addresses jbittel#83 and is a possible fix, but not ideal. The better
way to address the issue would be to track accesss services per session
and then send targeted single sign out requests based on this information.

Signed-off-by: Jenny Danzmayr <mail@evilscientress.at>
evilscientress added a commit to evilscientress/django-mama-cas that referenced this issue Jul 23, 2019
This addresses jbittel#83 and is a possible fix, but not ideal. The better
way to address the issue would be to track accesss services per session
and then send targeted single sign out requests based on this information.

Signed-off-by: Jenny Danzmayr <mail@evilscientress.at>
@manelclos
Copy link
Collaborator

Hi @beheh, sorry for the late reply.

The idea of checking all user valid tickets looks great, would you like to create a PR o share the relevant code here?

@manelclos manelclos added the bug label Apr 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants