Hello,
I just renamed constant in previous patch.
PR
On Fri, 2014-01-10 at 17:32 +0100, Pavel Reichl wrote:
Hi Jakub,
thank you for comments. New patches addressing issues are attached.
PR
On Fri, 2014-01-10 at 15:08 +0100, Jakub Hrozek wrote:
> On Wed, Jan 08, 2014 at 06:46:55PM +0100, Pavel Reichl wrote:
> > Hello,
> >
> > please see attached patches.
> >
> > PR
>
> Hi,
>
> I haven't tested the patches yet, just read the code.
>
> > From d5fbd3c3ef70f9d890f4eb73a97adb840a64470e Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl <pavel.reichl(a)redhat.com>
> > Date: Wed, 8 Jan 2014 15:46:57 +0000
> > Subject: [PATCH 1/2] simple access: match objects using flat name
>
> Looks good to me.
>
> > From dc613eca1c7b0ce0821f5d01bb2e5afae8a8b401 Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl <pavel.reichl(a)redhat.com>
> > Date: Wed, 8 Jan 2014 16:03:08 +0000
> > Subject: [PATCH 2/2] simple access: refresh master domain info
> >
> > To correctly decide if an object is a member of the main sssd domain, a flat
name
> > is needed. However, the information may not be available when the module is
> > inited so it may be necessary to refresh this data later while processing a
> > request.
> >
> > Resolves:
> >
https://fedorahosted.org/sssd/ticket/2189
> > ---
> > src/providers/simple/simple_access.c | 75
++++++++++++++++++++++++++++--------
> > src/providers/simple/simple_access.h | 2 +
> > 2 files changed, 60 insertions(+), 17 deletions(-)
> >
> > diff --git a/src/providers/simple/simple_access.c
b/src/providers/simple/simple_access.c
> > index
f2bfe755039fd7a370749fd3ce94a47c62c216bc..60179e3e4ac144b265ae7193b7b1b1305108be61 100644
> > --- a/src/providers/simple/simple_access.c
> > +++ b/src/providers/simple/simple_access.c
> > @@ -33,6 +33,7 @@
> > #define CONFDB_SIMPLE_DENY_GROUPS "simple_deny_groups"
> >
> > static void simple_access_check(struct tevent_req *req);
> > +static int sssm_simple_access_obtain_filter_lists(struct simple_ctx *ctx);
>
> I don't think the function should be prefixed with sssm_. Only functions
> dynamically loaded by sssd_be in the backends should have this prefix.
>
> >
> > void simple_access_handler(struct be_req *be_req)
> > {
> > @@ -40,13 +41,15 @@ void simple_access_handler(struct be_req *be_req)
> > struct pam_data *pd;
> > struct tevent_req *req;
> > struct simple_ctx *ctx;
> > + int ret;
> >
> > pd = talloc_get_type(be_req_get_data(be_req), struct pam_data);
> >
> > pd->pam_status = PAM_SYSTEM_ERR;
> >
> > if (pd->cmd != SSS_PAM_ACCT_MGMT) {
> > - DEBUG(4, ("simple access does not handles pam task %d.\n",
pd->cmd));
> > + DEBUG(SSSDBG_CONF_SETTINGS,
> > + ("simple access does not handles pam task %d.\n",
pd->cmd));
>
> While you're changing the debug message already, can you also fix the
> typo (s/handles/handle/)
>
> > pd->pam_status = PAM_MODULE_UNKNOWN;
> > goto done;
> > }
> > @@ -54,6 +57,15 @@ void simple_access_handler(struct be_req *be_req)
> > ctx = talloc_get_type(be_ctx->bet_info[BET_ACCESS].pvt_bet_data,
> > struct simple_ctx);
> >
> > + if (ctx->refresh_filter_lists) {
>
> I was actually thinking that becaues the sysdb_update is just a read
> from cache, we could do the update either on every request or use some
> very low grace period (5 seconds) to avoid unnecessary reads from sysdb
> during login bursts.
>
> > + ret = sssm_simple_access_obtain_filter_lists(ctx);
> > + if (ret != EOK) {
> > + DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to refresh filter
lists\n"));
>
> I think this can be a MINOR failure, given that the request doesn't
> terminate. Also the other place treats EAGAIN as not a failure, would it
> make sense to do the same here?
>
> > + } else {
> > + ctx->refresh_filter_lists = false;
> > + }
> > + }
> > +
> > req = simple_access_check_send(be_req, be_ctx->ev, ctx, pd->user);
> > if (!req) {
> > pd->pam_status = PAM_SYSTEM_ERR;
>
> [snip]
>
> > + ret = sysdb_master_domain_update(bectx->domain);
> > + if (ret != EOK) {
> > + DEBUG(SSSDBG_FUNC_DATA, ("Update of master domain failed [%d]:
%s.\n",
> > + ret, sss_strerror(ret)));
> > + goto failed;
> > + }
> > +
> > + if (bectx->domain->flat_name == NULL) {
> > + /* Cache is probably empty. */
> > + DEBUG(SSSDBG_FUNC_DATA,
> > + ("Flat name of master domain is not supposed to be
NULL.\n"));
> > + ctx->refresh_filter_lists = true;
> > + }
>
> This assumption is only valid for AD. Pure LDAP domains or IPA don't have
> flat name at all. If you're going to update the list on every request
> (or with some short grace period), then I think this check should be
> just dropped.
>
> > +
> > for (i = 0; lists[i].name != NULL; i++) {
> > ret = confdb_get_string_as_list(bectx->cdb, ctx,
bectx->conf_path,
> > lists[i].option,
&lists[i].orig_list);
> _______________________________________________
> sssd-devel mailing list
> sssd-devel(a)lists.fedorahosted.org
>
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel