Add single sign out support for cas server

Marked #1270952: CAS Server module: Logout does not work as expected as a duplicate.

Hy,

First, excuse me for my english.
Secondly, excuse me because it is a solution for version 6.x-3.x... but that should work with version 7.x-1.x.

I used the hook_user (logout operator) to get services and tickets in the database, and initiate http requests to warn disconnection.

Unfortunately I had to remove the deletion of ticket in _cas_server_validate function.
I think it just add a field in the table cas_server_tickets, to mark a ticket unavailable.

What do you think?

Overall, I think we are definitely on the right track here. Thanks for taking the initiative. You are correct that the code should be mostly identical for the 6.x and 7.x versions.

I have a few comments... let me know what you think of these ideas:

+++ cas-new/cas_server.module	2012-03-17 20:28:38.397187891 +0100
@@ -195,7 +195,8 @@
-  db_query("DELETE FROM {cas_server_tickets} WHERE ticket='%s'", array($ticket));
+  //@TODO: implement another one time use login ticket.
+  //db_query("DELETE FROM {cas_server_tickets} WHERE ticket='%s'", array($ticket));

We'll need to either:

1) Add a field indicating whether the ticket has been used; or
2) Have two separate tables, one for used and one for unused tickets.

I think the first option would be better.

+++ cas-new/cas_server.module	2012-03-17 20:28:38.397187891 +0100
@@ -242,3 +242,43 @@
+    $result = db_query("SELECT service, ticket FROM {cas_server_tickets} WHERE uid=%d", $account->uid);
+    if ($result !== FALSE) {
+      while ($client = db_fetch_object($result)) {
+        // Send POST request
+        $reponse = drupal_http_request(
+          $client->service,
+          array('Content-Type' => 'application/x-www-form-urlencoded'),
+          'POST',
+          _cas_server_logout_request($client->ticket)
+        );
+        // Remove ticket

I think it'd be worth turning this into a function, so that others can call it if necessary. (e.g., maybe we'll implement hook_cron to automatically log out sessions after a certain period of time?) Or maybe an admin interface for logging folks out?

+++ cas-new/cas_server.module	2012-03-17 20:28:38.397187891 +0100
@@ -242,3 +242,43 @@
+/**
+ * Generate the Single Sign Out request.
+ *
+ * @param unknown_type $ticket
+ */
+function _cas_server_logout_request($ticket) {
+

This should probably be a theme function, similar to how we handle other sorts of login/logout responses.

Totally agree with you.

Thanks.

Dave: I think you need to update your repository.

For the new database field, 'valid', should the default value be 0 or 1? I see arguments both ways, and I'm okay with 0, but since insertions will generally be when we create a new ticket, it would also make sense for it to be 1.

+++ cas_server.moduleundefined
@@ -210,4 +214,38 @@
+        list($url, $query) = @explode('?', $client->service);

Is this necessary?

+++ cas_server.moduleundefined
@@ -210,4 +214,38 @@
+            'query' => $query,

drupal_http_request doesn't use the 'query' parameter... Does $response = drupal_http_request($client->service, ...) not work?

I think it is a best practice to do as few DB queries as possible. In this case, we should consider deleting all of the tickets in one go:

$expired_tickets = array();
for($result as $client) {
  // ...
  $expired_tickets[] = $client->ticket;
}
db_delete('cas_server_tickets')
  ->condition('ticket', $expired_tickets, 'IN')
  ->execute();

I've attached David's patch in #6, cleaned up slightly (removed tabs, extra whitespace, and added a docstring).

I was thinking 0 because at install time any tickets in the system would be invalid. (Otherwise would've been deleted already).

Wouldn't the existing tickets be valid?

I set the status to "Needs Review", but it's going to fail since $ticket->valid isn't defined.

Overall this seems to be coming together nicely. Good catches with:
1) Not calling db_delete unless $expired_tickets is non-empty.
2) Remembering to invalidate existing tickets.

Quick backport attempt. Hasn't been tested yet.

At least one small typo in latest patches, using variable $reponse, should be $response.

Thanks for the catch. Patch for 7.x-1.x attached, revised 6.x-3.x patch to follow.

Patch in #25 (reponse -> response) committed to 7.x-1.x.

Revised backport to 6.x-3.x attached.

This looks pretty good!

I just wanted to answer on reply #3, maybe a function is not a bad idea.
I was just thinking about the possibility to have a user interface to see all the services one is logged into.
If it was a function one could log out of the different clients individually as well.
Do you think that could be interesting?

See you

Committed #26 to 6.x-3.x in 226a5ec0066db72013a932a6654784a6aba0f284. Thanks.