[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