On 06/24/2014 01:26 PM, Petr Viktorin wrote: > On 06/24/2014 12:35 PM, Martin Kosek wrote: >> On 06/24/2014 12:27 PM, Petr Viktorin wrote: >>> On 06/23/2014 05:51 PM, Martin Kosek wrote: >>>> On 06/23/2014 02:59 PM, Petr Viktorin wrote: >>>>> On 06/23/2014 10:07 AM, Martin Kosek wrote: >>>>>> On 06/20/2014 11:17 PM, Martin Kosek wrote: >>>>>>> On 06/20/2014 05:06 PM, Petr Viktorin wrote: >>>>>>>> All these should be independent, except for conflicts in ACI.txt that >>>>>>>> are >>>>>>>> easily solved by running makeaci. >>>>>>> >>>>>>> Umh, now the fun begins as I see :) There will probably need to be some >>>>>>> rebase, >>>>>>> it clashed with some other ACI patches in my tree (namely Hosts which I >>>>>>> acked). >>>>> >>>>> Rebased on top of my patch 0607, please apply that first. >>>>> >>>>> Added a new patch, 0608, which adds missing write permissions. >>>>> >>>>> >>>>>>> 594: we miss permissions for Automount Locations. Permissions for >>>>>>> keys&maps >>>>>>> look ok. >>>>> >>>>> Added in 0608. >>>>> >>>>>>> >>>>>>> 595: "System: Modify Group Membership" is probably waiting for the group >>>>>>> objectclass fix - the filter is different. Otherwise it looks ok. >>>>> >>>>> Right; rebased. >>>>> >>>>>>> 596-598: HBAC is ok >>>>>>> >>>>>>> 599: hostgroup is OK >>>>>>> >>>>>>> 600: there must have been some DS problem on my side as my regular user >>>>>>> could >>>>>>> not see any netgroup >>>>> >>>>> The problem is a bit closer to home this time. >>>>> Fixed in patch 0607. >>>>> >>>>>>> 601: privileges - we miss CRUD ACIs >>>>> >>>>> Added in 0608. >>>>> >>>>> We also miss CRUD permissions on permissions, but since currently these >>>>> need >>>>> pretty much unlimited access to ACIs, it's better to keep them admin-only. >>>>> >>>>>>> 602: roles were ok >>>>>>> >>>>>>> 603: ok >>>>>>> >>>>>>> I got this far today, the rest will need to wait for the next week. >>>>>> >>>>>> 604: ok, I was able to create a service, get a keytab >>>>>> >>>>>> 605: Should we case the permissions as "Sudo Command instead of "Sudo >>>>>> command"? >>>>> >>>>> Yes, fixed >>>>> >>>>>> 606: we also miss Modify Sudo Command permission so that people can >>>>>> modify >>>>>> description. Otherwise ok. >>>>> >>>>> Added in 0608. >>>>> >>>>> >>>> >>>> 1) # ipa-server-install: >>>> ... >>>> Applying LDAP updates >>>> ipa.ipaserver.install.ldapupdate.LDAPUpdate: ERROR Add failure missing >>>> required attribute "objectclass" >>>> ... >>>> >>>> There is a problem in this pending update: >>>> >>>> dn: cn=Modify Group membership,cn=permissions,cn=pbac,$SUFFIX >>>> add:member: 'cn=Modify Group membership,cn=privileges,cn=pbac,$SUFFIX' >>>> >>>> You apparently also need to make this permission also a member of "Modify >>>> Group >>>> membership" privilege. >>> >>> Fixed, thank you. >>> >>>> 2) We may not need "System: Modify Automount Locations" as there is just >>>> the CN >>>> and we do not support renames in automountlocation API. I am not insisting. >>> >>> Removed. >>> >>>> When these 2 issues are resolved, we can push. >>> >>> I've also added a patch that fixes a permission-find test which assumed >>> there >>> are many old permissions. >> >> Are you sure you tested the patches? > > Yes. > Not enough, I'm sure, but I did test them... > >> 1) This change in 595 looks suspicious: >> >> dn: $SUFFIX >> -# Don't allow the default 'manage group membership' to be able to manage the >> -# admins group >> -replace:aci:'(targetattr = "member")(target = >> "ldap:///cn=*,cn=groups,cn=accounts,$SUFFIX";)(version 3.0;acl >> "permission:Modify Group membership";allow (write) groupdn = >> "ldap:///cn=Modify >> Group membership,cn=permissions,cn=pbac,$SUFFIX";)::(targetfilter = >> "(!(cn=admins))")(targetattr = "member")(target = >> "ldap:///cn=*,cn=groups,cn=accounts,$SUFFIX";)(version 3.0;acl >> "permission:Modify Group membership";allow (write) groupdn = >> "ldap:///cn=Modify >> Group membership,cn=permissions,cn=pbac,$SUFFIX";)' >> >> It lefts the update file with >> dn: $SUFFIX >> dn: cn=ipa,cn=etc,$SUFFIX >> add:aci:'(target = >> "ldap:///cn=*,cn=ca_renewal,cn=ipa,cn=etc,$SUFFIX";)(version >> 3.0; acl "Add CA Certific >> ... > > Right. (It's removed in one of the later patches, so I didn't catch it.) > >> 2) About patch 609.3 - this test still looks very fragile. It would break >> just >> with adding one more permission to SUFFIX. > > It follows the general style of declarative tests, which depend on a freshly > installed system. > I think it's a good test to have around, at least until there's a better suite > for permission-find on legacy permissions.
Ok, as you wish. The patch set now looks good to me, tests are also good. ACK to all, I pushed them all to master. This pretty much concludes the ground work on permission/aci refactoring - good job Petr! :-) Martin _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
