On Tue, 2012-01-17 at 08:59 -0500, Rob Crittenden wrote:
> Martin Kosek wrote:
> > On Mon, 2012-01-16 at 22:20 -0500, Rob Crittenden wrote:
> >> Rob Crittenden wrote:
> >>> Martin Kosek wrote:
> >>>> On Mon, 2011-12-12 at 17:14 -0500, Rob Crittenden wrote:
> >>>>> This patch makes all categories and their equivalent members mutually
> >>>>> exclusive like in the HBAC plugin. So if you have usercat='all' you
> >>>>> can't add users.
> >>>>>
> >>>>> Added test cases for these as well.
> >>>>>
> >>>>> I also modified the default list of attributes to include the RunAs
> >>>>> attributes.
> >>>>>
> >>>>> rob
> >>>>
> >>>> NACK. I see several issues in this patch:
> >>>>
> >>>> 1) Error messages should be internationalized since they can be read by
> >>>> a user (this problem is in HBAC too)
> >>>>
> >>>> 2) All constructs like this one can be simplified (and thus made less
> >>>> error prone).
> >>>>
> >>>> + if 'cmdcategory' in _entry_attrs and \
> >>>> + _entry_attrs['cmdcategory'][0].lower() == 'all':
> >>>> + raise errors.MutuallyExclusiveError(reason="commands cannot be added
> >>>> when command category='all'")
> >>>>
> >>>> can be changed to:
> >>>>
> >>>> + if _entry_attrs.get('cmdcategory', [''])[0].lower() == 'all':
> >>>> + raise errors.MutuallyExclusiveError(...)
> >>>>
> >>>> I think the code would be then also more readable.
> >>>>
> >>>> 3) I think this code only works by an accident :-)
> >>>>
> >>>> + if ('ipasudorunasusercategory' or 'ipasudorunasgroupcategory') \
> >>>> + in _entry_attrs and \
> >>>> + (_entry_attrs['ipasudorunasusercategory'][0].lower() == 'all' or \
> >>>> + _entry_attrs['ipasudorunasgroupcategory'][0].lower() == 'all'):
> >>>> + raise errors.MutuallyExclusiveError(reason="users cannot be added
> >>>> when runAs user or runAs group category='all'")
> >>>>
> >>>> ('ipasudorunasusercategory' or 'ipasudorunasgroupcategory') simply
> >>>> returns 'ipasudorunasusercategory'. Thus the check for
> >>>> 'ipasudorunasgroupcategory' in _entry_attrs is not performed at all.
> >>>> Thanks to this bug, user is then able to pass a runAsGroup to sudorule
> >>>> with groupcat == 'all':
> >>>>
> >>>> # ipa sudorule-add foo --runasgroupcat=all
> >>>> ---------------------
> >>>> Added Sudo Rule "foo"
> >>>> ---------------------
> >>>> Rule name: foo
> >>>> Enabled: TRUE
> >>>> RunAs Group category: all
> >>>> # ipa sudorule-add-runasuser foo --groups=admins
> >>>> Rule name: foo
> >>>> Enabled: TRUE
> >>>> RunAs Group category: all
> >>>> Groups of RunAs Users: admins
> >>>> -------------------------
> >>>> Number of members added 1
> >>>> -------------------------
> >>>>
> >>>> A change proposed in 1) could make the change simpler:
> >>>>
> >>>> + if _entry_attrs.get('ipasudorunasusercategory', [''])[0].lower() ==
> >>>> 'all' or \
> >>>> + _entry_attrs.get('ipasudorunasgroupcategory', [''])[0].lower() ==
> >>>> 'all':
> >>>> + raise ...
> >>>>
> >>>> Martin
> >>>>
> >>>>
> >>>
> >>> Updated patch attached. Using the is_all() function instead. Opened
> >>> separate ticket to internationalize HBAC exceptions,
> >>> https://fedorahosted.org/freeipa/ticket/2267
> >>>
> >>> rob
> >>
> >> Rebased against ipa-2-2.
> >>
> >> rob
> >
> > There are still few issues:
> >
> > 1) test_sudorule_plugin.py is missing in the new commit
> >
> > 2) The patch does not work for ipasudorunasusercategory or
> > ipasudorunasgroupcategory as they are not in self.obj.default_attributes
> >
> > 3) I also saw some internal errors:
> >
> > # ipa sudorule-show foo --all
> >    dn:
> > ipauniqueid=a0e84cda-40e5-11e1-a35b-00163e7228ea,cn=sudorules,cn=sudo,dc=idm,dc=lab,dc=bos,dc=redhat,dc=com
> >    Rule name: foo
> >    Enabled: TRUE
> >    User category: all
> >    RunAs Group category: all
> >    Groups of RunAs Users: admins
> >    ipauniqueid: a0e84cda-40e5-11e1-a35b-00163e7228ea
> >    objectclass: ipaassociation, ipasudorul
> >
> > # ipa sudorule-add-user foo --users=admin
> > ipa: ERROR: an internal error has occurred
> >
> > # ipa sudorule-add-user foo --groups=admins
> > ipa: ERROR: an internal error has occurred
> >
> > # ipa sudorule-mod foo --hostcat=all
> > # ipa sudorule-add-host foo --hosts=`hostname`
> > ipa: ERROR: an internal error has occurred
> >
> > Martin
> >
> 
> Somehow sent the wrong version of the patch. This one should be better.
> 
> rob

Yes, its much better. Works fine, ACK.

Pushed to master, ipa-2-2.

Martin

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to