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

SOAP binding / backend logout work in progress proposal #613

Open
wants to merge 2 commits into
base: MOODLE_39_STABLE
Choose a base branch
from
Open

SOAP binding / backend logout work in progress proposal #613

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
56383be
SOAP binding / backend logout work in progress proposal
schlupmann Oct 9, 2021
8d4bf12
Revised backchannel logout proposal
schlupmann Dec 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion .extlib/simplesamlphp/lib/SimpleSAML/Session.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to try an eliminate this change to the internal library which I think is possible, see comments below

return;
} else {
call_user_func(['\auth_saml2\api', 'logout_from_idp_back_channel'], $this->authData[$authority]['Attributes']['username'][0], $this->sessionId);
}
}
foreach ($this->authData[$authority]['LogoutHandlers'] as $handler) {
// verify that the logout handler is a valid function
Expand Down
4 changes: 3 additions & 1 deletion .extlib/simplesamlphp/modules/saml/www/sp/saml2-logout.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@
$dst = $idpMetadata->getEndpointPrioritizedByBinding(
'SingleLogoutService',
[
\SAML2\Constants::BINDING_SOAP,
\SAML2\Constants::BINDING_HTTP_REDIRECT,
\SAML2\Constants::BINDING_HTTP_POST
]
Expand All @@ -143,8 +144,9 @@
$dst = $dst['Location'];
}
$binding->setDestination($dst);
} else {
$lr->setDestination($dst['Location']);
}
$lr->setDestination($dst);

$binding->send($lr);
} else {
Expand Down
13 changes: 13 additions & 0 deletions classes/api.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

global $DB;
if (isset($sessionId)) {
$DB->delete_records('auth_saml2_kvstore', array('k' => $sessionId));
}

$userid = $DB->get_record('user', array('username' => $username), 'id');
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

$mdsessionids = $DB->get_records('sessions', array('userid' => $userid->id), 'sid DESC', 'sid');
foreach ($mdsessionids as $mdsessionid) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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);

$DB->delete_records('sessions', array('sid' => $mdsessionid->sid));
}
}

/**
* SP logout callback. Called in case of normal Moodle logout.
* {@see auth::logoutpage_hook}
Expand Down
19 changes: 19 additions & 0 deletions sp/saml2-logout.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

} else {
try {
$binding = \SAML2\Binding::getCurrentBinding();
} catch (\Exception $e) {
// TODO: look for a specific exception
// This is dirty. Instead of checking the message of the exception, \SAML2\Binding::getCurrentBinding() should throw
// an specific exception when the binding is unknown, and we should capture that here
if ($e->getMessage() === 'Unable to find the current binding.') {
throw new \SimpleSAML\Error\Error('SLOSERVICEPARAMS', $e, 400);
} else {
throw $e; // do not ignore other exceptions!
}
}
$message = $binding->receive();

if ($message instanceof \SAML2\LogoutRequest) {
//Todo : parse xmlbody, get sp sessionid/moodle userid, and register logoutHandler from here ?
//For the time being the call to the specific logout function is done with 3 lines in extlib.. session.php
}
}

require('../.extlib/simplesamlphp/modules/saml/www/sp/saml2-logout.php');
Expand Down