SOAP binding / backend logout work in progress proposal #613
Conversation
|
no worries - can be hard to work out how to do this the first time round! - nice work on that patch it's much easier to review in that form - thanks! - I'll leave @brendanheywood to comment further. |
| @@ -736,7 +736,11 @@ private function callLogoutHandlers(string $authority): void | |||
| assert(isset($this->authData[$authority])); | |||
|
|
|||
| if (empty($this->authData[$authority]['LogoutHandlers'])) { | |||
| return; | |||
| if (empty($this->authData[$authority]['Attributes']['username'][0])) { | |||
There was a problem hiding this comment.
It would be good to try an eliminate this change to the internal library which I think is possible, see comments below
| @@ -54,6 +54,25 @@ | |||
| // user out in Moodle. | |||
| if (!is_null($session->getAuthState($saml2auth->spname))) { | |||
| $session->registerLogoutHandler($saml2auth->spname, '\auth_saml2\api', 'logout_from_idp_front_channel'); | |||
There was a problem hiding this comment.
This is how the logut handler is registered for the case where we have a session, with some wrangling I'm hoping we can also use tehe same method to register a logout handler for the soap side of the equation.
classes/api.php
Outdated
| @@ -39,6 +39,19 @@ public static function logout_from_idp_front_channel(): void { | |||
| require_logout(); | |||
| } | |||
|
|
|||
| public static function logout_from_idp_back_channel($username, $sessionId): void { | |||
There was a problem hiding this comment.
Can you flesh out the phpdoc here and clarify if the sessionid is the moodle session id or the SSP session id? Renaming the vars would be helpful too
There was a problem hiding this comment.
Also I would assume that the username here is not necessarily the same as the username of the moodle user and it may need to go through the same attribute mapping logic as here:
https://github.com/catalyst/moodle-auth_saml2/blob/MOODLE_39_STABLE/classes/auth.php#L72-L73
classes/api.php
Outdated
| $DB->delete_records('auth_saml2_kvstore', array('k' => $sessionId)); | ||
| } | ||
|
|
||
| $userid = $DB->get_record('user', array('username' => $username), 'id'); |
There was a problem hiding this comment.
maybe add MUST_EXIST or handle the case where the user doesn't exist? ie if they have been deleted while logged in in the interim (small edge case)
classes/api.php
Outdated
|
|
||
| $userid = $DB->get_record('user', array('username' => $username), 'id'); | ||
| $mdsessionids = $DB->get_records('sessions', array('userid' => $userid->id), 'sid DESC', 'sid'); | ||
| foreach ($mdsessionids as $mdsessionid) { |
There was a problem hiding this comment.
So I have a feeling these ~10 lines might be best re-ordered. This appears to be logging out all sessions of this users which isn't 100% correct. I would expect that you actually need to look at the values in the auth_saml2_kvstore to get a reverse mapping from the SSP session to the Moodle Session id and then just remove the single session. Also I would not touch any of the moodle session tables directly, instead use the moodle api for this:
\core\session\manager::kill_session($sid);
|
hi @schlupmann this looks like a really promising start. I've added a couple high level comments. Let me know when you are ready for more feedback |
|
Hello, We submitted our take at the SOAP binding issue for the auth_saml2 plugin as a pull request to this fork. We strongly based it on this proposal and payed attention to the reviewers comment. Doing so we tried to limitate any changes taking place in .exitlib/ as much as possible and got this solution to work without killing every session for the user but only the one targeted by the SAML logout Best regards, |
This is a work in progress / experimental proposal for a backend channel logout. It works in our environment.
I understand that any added code and the call to the specific logout function needs to be pulled out from the simplesamphp library. But to be able to get sessionids and register a logouthandler in sp/saml2-logout.php, we need to parse the xml message and pull quite a bit of the code from the extlib saml2-logout.php and logoutStore.php into sp/saml2-logout.php. It somehow defeats the purpose of keeping the plugin independent from the simplesamphp library.
I will keep on looking for a better solution.
(and sorry for the earlier mess... please remove the unnecessary zip files i posted on the subject)