[SSSD] [PATCH] Convert the simple access check to new error codes
Jakub Hrozek
jhrozek at redhat.com
Thu Apr 18 11:44:23 UTC 2013
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.
-------------- next part --------------
>From 292f5a394bdd57a5767fe10c67462dd7196eba1d 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..f313e68d6fb34e042d2c8f15be77b1546684f455 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 = ERR_INTERNAL;
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