[SSSD] [PATCH] Use new schema for HBAC service checks

Sumit Bose sbose at redhat.com
Mon May 24 09:29:24 UTC 2010


On Sun, May 23, 2010 at 12:14:05PM -0400, Stephen Gallagher wrote:
> On 05/21/2010 02:46 PM, Sumit Bose wrote:
> > Hi,
> >
> > here are two patches for the IPA access provider. The first one checks
> > if the rule is enable or not the second should fix #475 by using the new
> > service attribute and object in the IPA schema. Although the code should
> > handle service groups it currently does not work, because the IPA server
> > does not set the memberOf attribute to the service objects. If this is
> > fixed on the server side service groups should work, too.
> >
> > There are some areas of the code which are begging for refactoring, but
> > I think this is a post-1.2 task.
> 
> 
> Patch 0001: Ack
> 
> Patch 0002: Ack
> 
> This patch is functional, compiles cleanly and appears to work as 
> advertised. I have some concerns (listed below), but they are not 
> serious enough to warrant a Nack, given our time constraints for the 
> SSSD 1.2.0 release.
> 
> I'm comfortable with pushing this patch as-is to meet mandatory 
> feature-completion and fixing the remains later.
> 
> 
> Fixes for the future (post 1.2.0 - don't spend time on it now):
> 
> IPA_UNIQUE_ID exists as a patch on the FreeIPA development list. Please 
> fix the assorted "TODO: I would prefer IPA_UNIQUE_ID but currently it is 
> not available" spots post 1.2.0. (Qualifies as a bugfix).
> 
> There is a complete lack of comments in the entire procedure. This made 
> it extremely difficult to review. I would much prefer some comments 
> explaining each step, but this isn't a nack in itself (under the current 
> time constraints).
> 
> If the online auth fails because of a lost connection during processing, 
> we should retry from the cache before reporting an error.
> 
> Please wrap the SYSDB cache update into its own complete tevent_req, so 
> that a failure to update the cached service data is not a complete 
> failure for the access-control. If the cache update request fails, the 
> transaction should be cancelled, but the online operation should proceed
> based on the currently-available live data. A log message should be 
> reported that the cache updated failed at level 0, but otherwise it's 
> acceptable to return live data.
> 
> Also, if the operation is proceeding offline, it seems useless to 
> refresh the cache with the data we just got FROM the cache.

The cache is not updated in this case:

+    if ((state->services_reply_count == 0 &&
+         state->servicegroups_reply_count == 0)|| state->offline) {
+        tevent_req_done(req);
+        return;
+    }
+

I agree that this is hard to see without any comments :-).

Thanks for the review.

I will address the comments and rebase/rewrite the patch for master.

bye,
Sumit

> 
> -- 
> Stephen Gallagher
> RHCE 804006346421761
> 
> Delivering value year after year.
> Red Hat ranks #1 in value among software vendors.
> http://www.redhat.com/promo/vendor/
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/sssd-devel



More information about the sssd-devel mailing list