[Freeipa-devel] [PATCH] 552-557 Permissions v2 Web UI

[Misnyovszki Adam]

On Wed, 19 Mar 2014 10:52:10 +0100
Petr Viktorin <pviktori at redhat.com> wrote:

> On 03/18/2014 04:56 PM, Petr Vobornik wrote:
> > On 18.3.2014 15:07, Petr Viktorin wrote:
> >> On 03/18/2014 01:09 PM, Petr Vobornik wrote:
> >>> New revision for patch patch #557 attached.
> >>>
> >>> On 17.3.2014 15:22, Petr Viktorin wrote:
> >>>> On 03/14/2014 06:47 PM, Petr Vobornik wrote:
> >>>>> Main ACI UI changes are in patch #557. The rest are
> >>>>> prerequisites.
> >>>>
> >>>> With this UI it is impossible to change from "Type-based"
> >>>> permissions to
> >>>> "General" ones. This seems to be remaining from the old model
> >>>> where permissions were type/filter/subtree/targetgroup were
> >>>> "classes" of a permission rather than co-existing as attributes.
> >>>>
> >>>> Rather the Target section should IMO look the same for all
> >>>> (non-managed)
> >>>> permissions, with the first items being:
> >>>>      Type:    [drop-down with a None option]
> >>>>      Subtree: [textbox that is disabled when a Type is selected]
> >>>>
> >>>> The Subtree should be a one-line textbox. It would be acceptable
> >>>> if the whole DN doesn't always fit, it's the first part that's
> >>>> important.
> >>>>
> >>>> Remember to only send Subtree if Type is (staying as | being set
> >>>> to) None.
> >>>>
> >>>> Also, the Add dialog should use this instead of the "Define by".
> >>>
> >>> Done
> >>>
> >>>>
> >>>>
> >>>>
> >>>> With managed permissions, if I try to change both
> >>>> included/excluded attribute list and the effective attributes, I
> >>>> get a validation error, which is good in CLI but it doesn't work
> >>>> well for the UI.
> >>>>
> >>>> I think it would be better to move "Managed permission
> >>>> overrides" below "Target", and make it read-only. And perhaps
> >>>> rename it to something like
> >>>> "Attribute breakdown".
> >>>> Managing the included/excluded lists directly is only useful for
> >>>> upgrades with a heavily customized policy, and for upgrades you
> >>>> need the
> >>>> CLI anyway. Normally, having only the attribute list editable
> >>>> should be fine.
> >>>
> >>> Done
> >>>
> >>>>
> >>>>
> >>>>
> >>>> For SYSTEM permissions (those which only have the SYSTEM flag),
> >>>> such as 'Add Automember Rebuild Membership Task', Permissions
> >>>> should not be editable.
> >>>> For old-style permissions (those without any flags), nothing is
> >>>> editable
> >>>> but everything should be. The attributelevelrights are missing
> >>>> because the entry doesn't have the ipaPermissionV2 objectclass
> >>>> yet (although it's being reported, which is "my" bug -- #4257).
> >>>
> >>> Fields were set to be editable if attributes level rights are
> >>> missing.
> >>
> >> That solves things for normal legacy permissions, but not for the
> >> SYSTEM ones - those should be completely read-only.
> >>
> >> Also, changing the Permisisons checkboxes on these permissions
> >> doesn't mark them dirty.
> >>
> >> Otherwise the patches work great!
> >>
> >
> > Fixed
> >
> > New versions of 556 and 557 attached.
> >
> 
> Great, thanks!
> ACK for the functionality, I can't really judge the javascript though.
> 

ACK for the code and the test, besides these two issues(don't know if it
has to be corrected):
555:                                                                            
- typo in commit message(~delimeter)
557:                                                                            
- install/ui/test/aci_tests.js tab error at first row 

Greets:
Adam