refactor and simplify 2.4 module structure

All tests pass now, group behavior has been restored to match previous
functionality.

src/mod_sso/mod_sso.c

@@ -52,16 +52,37 @@ typedef struct {
   // Note: public_key is a binary buffer (non zero-terminated).
   const unsigned char *public_key;
 
-  // All known groups.
+  // All known groups (2.4: unused).
   apr_array_header_t *groups;
 } modsso_config;
 
-typedef struct {
-  apr_array_header_t *user_groups;
-} modsso_request_notes;
-
 typedef const char *(*CMD_HAND_TYPE) ();
 
+static char *groups_array_to_commasep_string(apr_pool_t *p, apr_array_header_t *groups)
+{
+  return apr_array_pstrcat(p, groups, ',');
+}
+
+static char *groups_charp_to_string(apr_pool_t *p, const char **groups) {
+  apr_array_header_t *arr = apr_array_make(p, 1, sizeof(char *));
+  const char **gptr;
+  for (gptr = groups; *gptr; gptr++) {
+    *(const char **)apr_array_push(arr) = *gptr;
+  }
+  return groups_array_to_commasep_string(p, arr);
+}
+
+static char **groups_array_to_charpp(apr_pool_t *p, apr_array_header_t *groups) {
+  int i;
+  char **pp, **ptr;
+  pp = (char **)apr_palloc(p, sizeof(char *) * (groups->nelts + 1));
+  for (ptr = pp, i = 0; i < groups->nelts; i++) {
+    *ptr++ = APR_ARRAY_IDX(groups, i, char *);
+  }
+  *ptr = NULL;
+  return pp;
+}
+
 /**
  * Create a modsso_config structure.
  *
@@ -155,25 +176,6 @@ static const char *set_modsso_public_key_file(cmd_parms *parms, void *mconfig, c
   return NULL;
 }
 
-static apr_array_header_t *parse_commasep_groups(apr_pool_t *pool, const char *commaseplist) {
-  apr_array_header_t *arr = apr_array_make(pool, 1, sizeof(const char *));
-  char *tokenizerCtx = NULL, *group;
-  char *tmp = apr_pstrdup(pool, commaseplist);
-  group = apr_strtok(tmp, ",", &tokenizerCtx);
-  do {
-    *(const char **)apr_array_push(arr) = group;
-    group = apr_strtok(NULL, ",", &tokenizerCtx);
-  } while (group != NULL);
-  return arr;
-}
-
-static const char *set_modsso_groups(cmd_parms *parms, void *mconfig, const char *arg)
-{
-  modsso_config *s_cfg = (modsso_config *)mconfig;
-  s_cfg->groups = parse_commasep_groups(parms->pool, arg);
-  return NULL;
-}
-
 static const command_rec mod_sso_cmds[] =
 {
   AP_INIT_TAKE1("SSOLoginServer", (CMD_HAND_TYPE) set_modsso_login_server, 
@@ -188,9 +190,6 @@ static const command_rec mod_sso_cmds[] =
   AP_INIT_TAKE1("SSOPublicKeyFile", (CMD_HAND_TYPE) set_modsso_public_key_file, 
 		NULL, RSRC_CONF,
 		"SSOPublicKeyFile (string) Location of the login server public key"),
-  AP_INIT_TAKE1("SSOGroups", (CMD_HAND_TYPE) set_modsso_groups,
-                NULL, RSRC_CONF,
-                "SSOGroups (string) comma-separated list of all the groups that we might want to check membership for"),
   {NULL}
 };
 
@@ -427,9 +426,6 @@ static int mod_sso_method_handler(request_rec *r)
      ap_get_module_config(r->per_dir_config, &sso_module);
   uri = r->uri;
 
-  ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
-      "sso: handler \"%s\"", r->handler);
-
   // Return immediately if there's nothing to do (check the AuthType)
   type = ap_auth_type(r);
   if (!type || strcasecmp(type, "SSO") != 0) {
@@ -502,13 +498,13 @@ static int mod_sso_method_handler(request_rec *r)
   return DECLINED;
 }
 
+#if MODULE_MAGIC_NUMBER_MAJOR < 20100714
 struct modsso_auth_req {
   apr_array_header_t *groups;
   apr_array_header_t *users;
   int any_user;
 };
 
-#if MODULE_MAGIC_NUMBER_MAJOR < 20100714
 static int array_contains(apr_array_header_t *arr, const char *s)
 {
   int i;
@@ -588,28 +584,11 @@ static void mod_sso_parse_requirements(request_rec *r,
 }
 #endif
 
-static char *encode_groups(apr_pool_t *p, apr_array_header_t *groups)
-{
- /**
-  apr_array_header_t *arr = apr_array_make(p, (groups->nelts - 1) * 2 - 1, sizeof(const char *));
-  int i;
-
-  for (i = 0; i < groups->nelts; i++) {
-    if (i > 0) {
-      *(const char **)apr_array_push(arr) = ",";
-    }
-    *(const char **)apr_array_push(arr) = ((const char **)groups->elts)[i];
-  }
-  return apr_array_pstrcat(p, arr, 0);
- **/
-  return apr_array_pstrcat(p, groups, ',');
-}
-
 static int redirect_to_login_server(request_rec *r,
                                     const char *login_server,
                                     const char *service_host,
                                     const char *service,
-                                    apr_array_header_t *groups)
+                                    const char **groups)
 {
   char *dest, *login_url;
   dest = full_uri(r, service_host);
@@ -620,11 +599,11 @@ static int redirect_to_login_server(request_rec *r,
                           "&d=",
                           modsso_url_encode(r->pool, dest),
                           NULL);
-  if (!apr_is_empty_array(groups)) {
+  if (groups) {
     login_url = apr_pstrcat(r->pool,
                             login_url,
                             "&g=",
-                            encode_groups(r->pool, groups),
+                            groups_charp_to_string(r->pool, groups),
                             NULL);
   }
   ap_log_error(APLOG_MARK, APLOG_INFO, 0, r->server,
@@ -634,6 +613,7 @@ static int redirect_to_login_server(request_rec *r,
   return http_redirect(r, login_url);
 }
 
+#if 0
 static char *pkey_to_string(const unsigned char *pkey, char *buf) {
   static const char *hex = "0123456789ABCDEF";
   char *o = buf;
@@ -646,6 +626,7 @@ static char *pkey_to_string(const unsigned char *pkey, char *buf) {
   *o = '\0';
   return buf;
 }
+#endif
 
 /**
  * Apache authentication handler for mod_sso.
@@ -691,23 +672,13 @@ static int mod_sso_check_access_ex(request_rec *r)
   return DECLINED;
 }
 
-static char *parse_ticket_groups(apr_pool_t *pool, char **groups) {
-  apr_array_header_t *arr = apr_array_make(pool, 1, sizeof(char *));
-  if (groups) {
-    while (*groups) {
-      *(char **)apr_array_push(arr) = apr_pstrdup(pool, *groups);
-      groups++;
-    }
-  }
-  return apr_array_pstrcat(pool, arr, ',');
-}
-
 static int mod_sso_check_user_id(request_rec *r)
 {
   const char *type, *sso_cookie_name, *sso_cookie;
   const char *service = NULL, *service_host = NULL,
     *service_path = NULL;
   int retval, err, do_redirect = 1;
+  const char **required_groups;
   modsso_config *s_cfg = (modsso_config *)
     ap_get_module_config(r->per_dir_config, &sso_module);
   //apr_array_header_t *sso_validate_groups = NULL;
@@ -729,9 +700,6 @@ static int mod_sso_check_user_id(request_rec *r)
     }
   }
 
-  ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
-       "sso (check_user_id): handler '%s' uri '%s'", r->handler, r->uri);
-
   sso_cookie_name = get_cookie_name(r);
 
   // Check if the required parameters are defined.
@@ -746,11 +714,15 @@ static int mod_sso_check_user_id(request_rec *r)
     return HTTP_BAD_REQUEST;
   }
 
+  // Fetch the list of desired groups set (eventually) by group_check_authorization.
+  required_groups = (const char **)apr_table_get(r->notes, "SSO_REQUIRED_GROUPS");
+  
   // Test for valid cookie
   sso_cookie = get_cookie(r, sso_cookie_name);
   if (sso_cookie != NULL) {
     sso_ticket_t t;
 
+#if 0
     // Print some debugging information about the service
     {
       char pkeybuf[512];
@@ -763,26 +735,20 @@ static int mod_sso_check_user_id(request_rec *r)
          r->uri, service, s_cfg->service, host_hdr, sso_cookie,
          pkey_to_string(s_cfg->public_key, pkeybuf));
     }
+#endif
 
     err = sso_ticket_open(&t, sso_cookie, s_cfg->public_key);
     if (err != SSO_OK) {
       ap_log_error(APLOG_MARK, APLOG_WARNING, 0, r->server,
                    "sso: ticket decoding error: %s", sso_strerror(err));
     } else {
-      // TODO: remove this so as to skip group membership check in sso_validate.
-      /* if (s_cfg->groups != NULL) { */
-      /*   sso_validate_groups = apr_array_copy(r->pool, s_cfg->groups); */
-      /*   *(const char **)apr_array_push(sso_validate_groups) = NULL; */
-      /* } */
-
-      err = sso_validate(t, s_cfg->service, s_cfg->domain,
-                         //apr_is_empty_array(s_cfg->groups) ? NULL : (const char **)sso_validate_groups
-                         NULL);
+      err = sso_validate(t, s_cfg->service, s_cfg->domain, required_groups);
       if (err != SSO_OK) {
         ap_log_error(APLOG_MARK, APLOG_WARNING, 0, r->server,
                      "sso: validation error: %s", sso_strerror(err));
       } else {
-        apr_table_setn(r->notes, "SSO_GROUPS", parse_ticket_groups(r->pool, t->groups));
+        // Don't do this: t->groups wil be freed by sso_ticket_free
+        // apr_table_setn(r->notes, "SSO_GROUPS", (char *)(t->groups));
         apr_table_setn(r->subprocess_env, "SSO_SERVICE",
                        apr_pstrdup(r->pool, service));
         r->user = apr_pstrdup(r->pool, t->user);
@@ -801,7 +767,7 @@ static int mod_sso_check_user_id(request_rec *r)
   }
 
   // Redirect to login server
-  return redirect_to_login_server(r, s_cfg->login_server, service_host, service, s_cfg->groups);
+  return redirect_to_login_server(r, s_cfg->login_server, service_host, service, required_groups);
 }
 #else
 static int mod_sso_check_user_id(request_rec *r)
@@ -912,7 +878,7 @@ static int mod_sso_check_user_id(request_rec *r)
   }
 
   // Redirect to login server
-  return redirect_to_login_server(r, s_cfg->login_server, service_host, service, auth.groups);
+  return redirect_to_login_server(r, s_cfg->login_server, service_host, service, apr_is_empty_array(auth.groups) ? NULL : (const char **)auth.groups->elts);
 }
 #endif /* apache 2.2 */
 
@@ -920,78 +886,69 @@ static int mod_sso_check_user_id(request_rec *r)
  * Apache authorization check callback for mod_sso.
  */
 #if MODULE_MAGIC_NUMBER_MAJOR >= 20100714
-static authz_status sso_check_authorization(request_rec *r, const char *require_args, const void *parsed_require_args)
-{
-  const char *uri, *type;
-  const char *service = NULL, *service_host = NULL,
-    *service_path = NULL;
-  char *sso_logout_path, *sso_login_path;
-  modsso_config *s_cfg;
+static apr_array_header_t *required_groups_array(request_rec *r, const void *parsed_require_args) {
+  const ap_expr_info_t *expr = parsed_require_args;
+  const char *err = NULL;
+  const char *require, *w, *t;
+  apr_array_header_t *grouparr = apr_array_make(r->pool, 1, sizeof(char *));
   
-  // We already did everything in mod_sso_check_user_id(),
-  // so just succeed (if SSO is active).
-  type = ap_auth_type(r);
-  if (type && !strcasecmp(type, "SSO") && r->user) {
-    return OK;
+  require = ap_expr_str_exec(r, expr, &err);
+  if (err) {
+    return NULL;
   }
 
-  // XXX check groups via e.g. 'Require sso-groups' or 'Require valid-group'
-  s_cfg = (modsso_config *)
-    ap_get_module_config(r->per_dir_config, &sso_module);
-  uri = r->uri;
-  if (parse_service(r, s_cfg, &service, &service_host, &service_path) != 0) {
-    ap_log_error(APLOG_MARK, APLOG_ERR, 0, r->server,
-		 "sso (auth_checker): could not parse service \"%s\"",
-		 s_cfg->service);
-    return HTTP_BAD_REQUEST;
+  t = require;
+  while ((w = ap_getword_conf(r->pool, &t)) && w[0]) {
+    *(const char **)apr_array_push(grouparr) = w;
   }
-
-  sso_logout_path = apr_pstrcat(r->pool, service_path, "sso_logout", NULL);
-  sso_login_path = apr_pstrcat(r->pool, service_path, "sso_login", NULL);
-  if (!strcmp(uri, sso_logout_path) || !strcmp(uri, sso_login_path)) {
-    return OK;
+  return grouparr;
 }
 
-  return DECLINED;
+static char **required_groups_charpp(request_rec *r, const void *parsed_require_args) {
+  apr_array_header_t *arr = required_groups_array(r, parsed_require_args);
+  if (!arr) {
+    return NULL;
+  }
+  return groups_array_to_charpp(r->pool, arr);
 }
 
+// This function will be called twice: first, before any authn
+// handlers are executed, and we return AUTHZ_DENIED_NO_USER to tell
+// Apache that we need a user. This should cause it to invoke
+// mod_sso_check_user_id, and then call this function again.
 static authz_status group_check_authorization(request_rec *r, const char *require_args, const void *parsed_require_args) {
-  const ap_expr_info_t *expr = parsed_require_args;
-  const char *err = NULL;
-  const char *group_str, *require, *w, *t;
-  apr_array_header_t *user_groups = NULL;
-  int i;
-  
-  // Retrieve the comma-separated group list from r->notes.
-  group_str = apr_table_get(r->notes, "SSO_GROUPS");
-  if (!group_str) {
-    return DECLINED;
+  // Do we have a user? All ok then! We assume that the request was
+  // validated by mod_sso_check_user_id using the value of
+  // SSO_REQUIRED_GROUPS we set earlier.
+  if (r->user) {
+    return AUTHZ_GRANTED;
   }
-  user_groups = parse_commasep_groups(r->pool, group_str);
 
-  require = ap_expr_str_exec(r, expr, &err);
-  if (err) {
-    ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02592)
-                  "mod_sso authorize: require group: Can't "
-                  "evaluate require expression: %s", err);
-    return AUTHZ_DENIED;
+  // Set the list of groups in the request notes, so that the
+  // sso authn handler can retrieve it and validate the ticket.
+  apr_table_setn(r->notes, "SSO_REQUIRED_GROUPS",
+                 (char *)required_groups_charpp(r, parsed_require_args));
+  return AUTHZ_DENIED_NO_USER;
 }
 
-  ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
-       "sso (check_group): user '%s' user_groups '%s'", r->user, group_str);
+static const char *group_parse_config(cmd_parms *cmd, const char *require_line,
+                                      const void **parsed_require_line)
+{
+  const char *expr_err = NULL;
+  ap_expr_info_t *expr;
 
-  t = require;
-  while ((w = ap_getword_conf(r->pool, &t)) && w[0]) {
-    // Check if w is in user_groups.
-    for (i = 0; i < user_groups->nelts; i++) {
-      char *el = APR_ARRAY_IDX(user_groups, i, char *);
-      if (!strcasecmp(w, el)) {
-        return AUTHZ_GRANTED;
-      }
-    }
+  expr = ap_expr_parse_cmd(cmd, require_line, AP_EXPR_FLAG_STRING_RESULT,
+                           &expr_err, NULL);
+
+  if (expr_err) {
+    return apr_pstrcat(cmd->temp_pool,
+                       "Cannot parse expression in require line: ",
+                       expr_err, NULL);
   }
 
-  return DECLINED;
+  *parsed_require_line = expr;
+
+  return NULL;
 }
 
 #else
@@ -1031,16 +988,10 @@ static int mod_sso_auth_checker(request_rec *r)
 #endif
 
 #if MODULE_MAGIC_NUMBER_MAJOR >= 20100714
-static const authz_provider authz_sso_provider =
-{
-      &sso_check_authorization,
-      NULL,
-};
-
 static const authz_provider authz_sso_group_provider =
 {
       &group_check_authorization,
-      NULL,
+      &group_parse_config,
 };
 #endif
 
@@ -1056,9 +1007,8 @@ static void mod_sso_register_hooks (apr_pool_t *p)
 {
   ap_hook_handler(mod_sso_method_handler, NULL, NULL, APR_HOOK_FIRST);
 #if MODULE_MAGIC_NUMBER_MAJOR >= 20100714
-  ap_hook_check_authn(mod_sso_check_user_id, NULL, NULL, APR_HOOK_MIDDLE, AP_AUTH_INTERNAL_PER_CONF);
+  ap_hook_check_authn(mod_sso_check_user_id, NULL, NULL, APR_HOOK_FIRST, AP_AUTH_INTERNAL_PER_CONF);
   ap_hook_check_access_ex(mod_sso_check_access_ex, NULL, NULL, APR_HOOK_MIDDLE, AP_AUTH_INTERNAL_PER_CONF);
-  ap_register_auth_provider(p, AUTHZ_PROVIDER_GROUP, SSO_REQUIRE_NAME, "0", &authz_sso_provider, AP_AUTH_INTERNAL_PER_CONF);
   ap_register_auth_provider(p, AUTHZ_PROVIDER_GROUP, "group", "0", &authz_sso_group_provider, AP_AUTH_INTERNAL_PER_CONF);
 #else
   static const char * const authzSucc[] = { "mod_sso.c", NULL };

src/mod_sso/test/httpd_integration_test.py

@@ -141,9 +141,10 @@ class HttpdIntegrationTest(unittest.TestCase):
         def mkcookie(tkt):
             return "SSO_test=%s" % tkt
 
-        # For Apache 2.2, set this to the empty string (we do not use the
-        # SSOGroup directive, so only the requested groups are generated).
-        extra_groups = "&g=group1,group2,group3"
+        # Set to a non-empty string when testing the SSOGroups directive
+        # (normally only the requested groups are generated).
+        #extra_groups = "&g=group1,group2,group3"
+        extra_groups = ''
 
         # Tests have a name so that we can recognize failures.
         checks = [
@@ -185,6 +186,10 @@ class HttpdIntegrationTest(unittest.TestCase):
               "cookie": mkcookie(self._ticket()),
               "status": 200,
               "body": "ok"}),
+            #("protected-group with cookie wrong group -> unauthorized",
+            # {"url": "/protected-group/index.html",
+            #  "cookie": mkcookie(self._ticket(group="group2")),
+            #  "status": 401}),
             ("protected-group with cookie wrong group -> redirect",
              {"url": "/protected-group/index.html",
               "cookie": mkcookie(self._ticket(group="group2")),
@@ -213,7 +218,7 @@ class HttpdIntegrationTest(unittest.TestCase):
             ]
         for name, check in checks:
             for i in xrange(10):
-                print 'CHECKING', check
+                print 'CHECKING %s (%d of 10)' % (name, i), check
                 status, body, location = _query(check["url"],
                                                 host=check.get("http_host"),
                                                 cookie=check.get("cookie"))

src/mod_sso/test/test-httpd-2.4.conf

@@ -17,7 +17,7 @@ LogLevel debug
 SSOLoginServer login.example.com
 SSODomain example.com
 SSOPublicKeyFile ${TESTROOT}/public.key
-SSOGroups group1,group2,group3
+#SSOGroups group1,group2,group3
 
 DocumentRoot ${TESTROOT}/htdocs
 <Directory "${TESTROOT}/htdocs">
@@ -25,6 +25,8 @@ DocumentRoot ${TESTROOT}/htdocs
 	AuthName test
 	SSOService service.example.com/
 	require valid-user
+
+	AllowOverride All
 </Directory>
 
 <Location "/other-service">