Skip to content

Conversation

@m0ark
Copy link
Contributor

@m0ark m0ark commented Apr 24, 2025

The logout function referenced the LogoutState key in the state, which is not present during logouts.

This PR fixes the logout.

@tvdijen
Copy link
Member

tvdijen commented Apr 24, 2025

I don't see how this fixes anything. It just suppresses the exception you're experiencing and then continues as if 'not set', because $state['negotiate:backend'] is never set and therefor always null.

Copy link
Member

@tvdijen tvdijen left a comment

Choose a reason for hiding this comment

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

The problem is clear; state info does not persist over consecutive visits.
This PR however does not actually fix anything, rather than hiding this issue.

@m0ark
Copy link
Contributor Author

m0ark commented Apr 24, 2025

Hi @tvdijen,

during logout the only place where the original $state['LogoutState'] is read from, is this one https://github.com/simplesamlphp/simplesamlphp/blob/c5ff618be75e51d0811ea45c85d4931f080545e0/src/SimpleSAML/Auth/Simple.php#L209
This line explicitly loads the 'LogoutState' key from the session store, merges it with other parameters provided during the function call and finally passes it to the authentication source's logout function (the one I modified).
So the keys originally nested in 'LogoutState' now are at the top level of the array and thus my change should lead to the expected outcome.

I double checked how other modules handle LogoutState and it's the same (e.g. SAML/SP).
https://github.com/simplesamlphp/simplesamlphp/blob/c5ff618be75e51d0811ea45c85d4931f080545e0/modules/saml/src/Auth/Source/SP.php#L1169
https://github.com/simplesamlphp/simplesamlphp/blob/c5ff618be75e51d0811ea45c85d4931f080545e0/modules/saml/src/Controller/ServiceProvider.php#L456

@tvdijen
Copy link
Member

tvdijen commented Apr 24, 2025

My appologies! You are absolutely correct!

@tvdijen tvdijen merged commit 5526069 into simplesamlphp:master Apr 24, 2025
6 of 8 checks passed
@tvdijen
Copy link
Member

tvdijen commented Apr 24, 2025

Tagged v2.3.2

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 16, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants