On 05/23/2010 12:14 PM, 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
Pushed to sssd-1-2.
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.
--
Stephen Gallagher
RHCE 804006346421761
Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/