[SSSD] [PATCH] Convert the simple access check to new error codes

Jakub Hrozek jhrozek at redhat.com
Fri Apr 19 13:54:34 UTC 2013


On Fri, Apr 19, 2013 at 09:22:11AM -0400, Stephen Gallagher wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 04/19/2013 08:25 AM, Pavel Březina wrote:
> > On 04/18/2013 01:44 PM, Jakub Hrozek wrote:
> >> On Thu, Apr 18, 2013 at 10:59:11AM +0200, Pavel Březina wrote:
> >>>> On 04/17/2013 11:39 AM, Jakub Hrozek wrote:
> >>>>>> Hi,
> >>>>>> 
> >>>>>> I took a look into converting the simple access provider
> >>>>>> to use
> >>>> the new
> >>>>>> error codes and I think a patch might not even be needed.
> >>>>>> The
> >>>> current
> >>>>>> request returns errno on error and boolean for access
> >>>> granted/denied.
> >>>>>> 
> >>>>>> I think the boolean is actually nice in the case and
> >>>>>> converting to ERR_ACCESS_DENIED instead of false doesn't
> >>>>>> make much sense.
> >>>>>> 
> >>>>>> The attached patch just makes the top-level request
> >>>>>> called by the DP handler return ERR_ACCOUNT_UNKNOWN in
> >>>>>> case the user passed via
> >>>> struct
> >>>>>> pam_data does not exist in sysdb and ERR_INTERNAL instead
> >>>>>> of EIO for internal errors. It would make the error
> >>>>>> reporting somewhat nicer, I think, but even if we drop
> >>>>>> the patch I would like to close 
> >>>>>> https://fedorahosted.org/sssd/ticket/453
> >>>> 
> >>>> Nack.
> >>>> 
> >>>> You need to use sss_strerror() when dealing with new error
> >>>> codes.
> >>>> 
> >>>>>> @@ -629,11 +631,15 @@ struct tevent_req
> >>>> *simple_access_check_send(TALLOC_CTX *mem_ctx,
> >>>>>> DEBUG(SSSDBG_FUNC_DATA, ("Simple access check for %s\n",
> >>>> username));
> >>>>>> 
> >>>>>> ret = simple_check_users(ctx, username,
> >>>> &state->access_granted);
> >>>>>> -    if (ret != EAGAIN) { -        /* Both access denied
> >>>>>> and an error */ +    if (ret != EOK && ret != EAGAIN) { +
> >>>>>> ret = ERR_INTERNAL; +        goto immediate; +    } else
> >>>>>> if (ret == EOK) { goto immediate; }
> >>>> 
> >>>> How about if (ret == EOK) { goto immediate; } else if (ret !=
> >>>> EAGAIN) { ret = ERR_INTERNAL; goto immediate; }
> >> Thank you, a new patch is attached.
> > 
> >> subreq = simple_check_get_groups_send(state, ev, ctx, username); 
> >> if (!subreq) { -        ret = EIO; +        ret = ERR_INTERNAL; 
> >> goto immediate; }
> > 
> > Hi, I think we should stick with ENOMEM for tevent request - when
> > _send returns NULL it pretty much means that it couldn't create
> > request because some malloc failed. But if you want to use other
> > code for it, I'd rather use something else than ERR_INTERNAL.
> > 
> 
> I agree. If simple_check_get_groups_send() can fail for any reason
> other than ENOMEM, it should be doing so in a tevent_req_immediate()
> failure handler, not returning NULL for the subreq. If it's doing
> that, it's a style violation.

This is true for any new code, many old requests still blindly return
NULL on any error.

That doesn't make Pavel's recommendation any less true, so a new patch
is attached.
-------------- next part --------------
>From 202519fbec14460377735e7831362eb93f54d9f1 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Wed, 17 Apr 2013 11:33:41 +0200
Subject: [PATCH] Convert the simple access check to new error codes

https://fedorahosted.org/sssd/ticket/453

It makes sense to keep using the boolean for access granted/denied, but
when the user/group is not found, the request would now return
ERR_ACCOUNT_UNKNOWN
---
 src/providers/simple/simple_access_check.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/src/providers/simple/simple_access_check.c b/src/providers/simple/simple_access_check.c
index 663b7ceacf8e23a4505313be706f75d6a89f950e..dc5f3676484c3748186cbe69fd4260ee329568a6 100644
--- a/src/providers/simple/simple_access_check.c
+++ b/src/providers/simple/simple_access_check.c
@@ -190,7 +190,8 @@ simple_resolve_group_send(TALLOC_CTX *mem_ctx,
         goto done;
     } else if (ret != EAGAIN) {
         DEBUG(SSSDBG_OP_FAILURE,
-              ("Cannot check if group was already updated\n"));
+              ("Cannot check if group was already updated [%d]: %s\n",
+               ret, sss_strerror(ret)));
         goto done;
     }
     /* EAGAIN - still needs update */
@@ -245,14 +246,14 @@ simple_resolve_group_check(struct simple_resolve_group_state *state)
     if (ret != EOK) {
         DEBUG(SSSDBG_OP_FAILURE,
                ("Could not look up group by gid [%lu]: [%d][%s]\n",
-               state->gid, ret, strerror(ret)));
+               state->gid, ret, sss_strerror(ret)));
         return ret;
     }
 
     state->name = ldb_msg_find_attr_as_string(group, SYSDB_NAME, NULL);
     if (!state->name) {
         DEBUG(SSSDBG_OP_FAILURE, ("No group name\n"));
-        return ENOENT;
+        return ERR_ACCOUNT_UNKNOWN;
     }
 
     if (is_posix(group) == false) {
@@ -371,11 +372,12 @@ simple_check_get_groups_send(TALLOC_CTX *mem_ctx,
                                     username, attrs, &user);
     if (ret == ENOENT) {
         DEBUG(SSSDBG_MINOR_FAILURE, ("No such user %s\n", username));
+        ret = ERR_ACCOUNT_UNKNOWN;
         goto done;
     } else if (ret != EOK) {
         DEBUG(SSSDBG_OP_FAILURE,
               ("Could not look up username [%s]: [%d][%s]\n",
-              username, ret, strerror(ret)));
+              username, ret, sss_strerror(ret)));
         goto done;
     }
 
@@ -563,7 +565,7 @@ simple_check_get_groups_primary(struct simple_check_groups_state *state,
     if (ret != EOK) {
         DEBUG(SSSDBG_OP_FAILURE,
                ("Could not look up primary group [%lu]: [%d][%s]\n",
-               gid, ret, strerror(ret)));
+               gid, ret, sss_strerror(ret)));
         /* We have to treat this as non-fatal, because the primary
          * group may be local to the machine and not available in
          * our ID provider.
@@ -629,11 +631,15 @@ struct tevent_req *simple_access_check_send(TALLOC_CTX *mem_ctx,
     DEBUG(SSSDBG_FUNC_DATA, ("Simple access check for %s\n", username));
 
     ret = simple_check_users(ctx, username, &state->access_granted);
-    if (ret != EAGAIN) {
-        /* Both access denied and an error */
+    if (ret == EOK) {
+        goto immediate;
+    } else if (ret != EAGAIN) {
+        ret = ERR_INTERNAL;
         goto immediate;
     }
 
+    /* EAGAIN -- check groups */
+
     if (!ctx->allow_groups && !ctx->deny_groups) {
         /* There are no group restrictions, so just return
          * here with whatever we've decided.
@@ -648,7 +654,7 @@ struct tevent_req *simple_access_check_send(TALLOC_CTX *mem_ctx,
      */
     subreq = simple_check_get_groups_send(state, ev, ctx, username);
     if (!subreq) {
-        ret = EIO;
+        ret = ENOMEM;
         goto immediate;
     }
     tevent_req_set_callback(subreq, simple_access_check_done, req);
@@ -692,7 +698,9 @@ static void simple_access_check_done(struct tevent_req *subreq)
     ret = simple_check_groups(state->ctx, state->group_names,
                               &state->access_granted);
     if (ret != EOK) {
-        tevent_req_error(req, ret);
+        DEBUG(SSSDBG_OP_FAILURE, ("Could not check group access [%d]: %s\n",
+              ret, sss_strerror(ret)));
+        tevent_req_error(req, ERR_INTERNAL);
         return;
     }
 
-- 
1.8.1.4



More information about the sssd-devel mailing list