Bug 32036 - Cannot set single sub-permission without also setting the top level flag
Summary: Cannot set single sub-permission without also setting the top level flag
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff interface (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-10-28 14:35 UTC by Lucas Gass (lukeg)
Modified: 2022-11-18 02:10 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Lucas Gass (lukeg) 2022-10-28 14:35:36 UTC
In permissions, if a top level flag only has a single sub-permission you cannot set the submersion without also setting the top-level permission.

For instance, you cannot set the sub-permission 'manage_recalls' without also having the 'recalls' flag set. 

I think you should be able to set these independently.
Comment 1 Katrin Fischer 2022-11-01 16:13:16 UTC
I believe this is intentional and not a bug if there is only one sub-permission, it equals the module permissions level.
Comment 2 David Cook 2022-11-01 23:59:46 UTC
Choosing the top level flag is the same thing as selecting all sub-permissions. If you go to "Check out and check in items", choose all the sub-permissions and you'll see that the top level flag gets set. (Alternatively, if you just choose the top level flag, it will set all the sub-permissions. Uncheck one sub-permission, and you the top level flag is unchecked.)

If you go to https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=31389&attachment=140563 and look at "Permissions.t", it might be clearer.

You get CAN_user_circulate if you're superlibrarian, if you have the top level "circulate" permission, or if you have *any* circulation sub-permission. 

When you just set "circulate" without specifying individual subpermissions, it automatically gives you all sub-permissions. 

So setting the "manage_recalls" sub-permission is functionally equivalent to setting the "recalls" top level flag, since there is only . You'll get the exact same authorization.

I hope that makes sense.
Comment 3 Nick Clemens (kidclamp) 2022-11-02 17:53:54 UTC
I think this causes confusion then - either we should always have a single subpermission, or we should only have them if we have two?

For OPAC problem reports - we only seem to ever check the top permission
For recalls we check the subpermission every time, except once

Having two settings with the same behavior seems wrong to me

Maybe we need a guideline?
Comment 4 David Cook 2022-11-02 22:19:42 UTC
(In reply to Nick Clemens from comment #3)
> I think this causes confusion then - either we should always have a single
> subpermission, or we should only have them if we have two?

Confusion for whom?

> For OPAC problem reports - we only seem to ever check the top permission
> For recalls we check the subpermission every time, except once
>
> Having two settings with the same behavior seems wrong to me

What do you mean by "two settings"?

For "OPAC problem reports management" and "Recalls", you get the same thing whether you click the top level or the sub-permission, and that's clearly indicated visually.
Comment 5 Nick Clemens (kidclamp) 2022-11-03 09:51:00 UTC
(In reply to David Cook from comment #4)
> (In reply to Nick Clemens from comment #3)
> > I think this causes confusion then - either we should always have a single
> > subpermission, or we should only have them if we have two?
> 
> Confusion for whom?

For developers, or users. Why does problem reports have a subpermission, and edit authorities does not?

> 
> > For OPAC problem reports - we only seem to ever check the top permission
> > For recalls we check the subpermission every time, except once
> >
> > Having two settings with the same behavior seems wrong to me
> 
> What do you mean by "two settings"?

flagsrequired   => { recalls => "manage_recalls" }
flagsrequired => { recalls => 1 }

These are checking two different things, but provide the same answer

> 
> For "OPAC problem reports management" and "Recalls", you get the same thing
> whether you click the top level or the sub-permission, and that's clearly
> indicated visually.

Sure, but then why is the subpermission added at all? If it is the same as clicking the top level, let's not provide two checkboxes when one will do
Comment 6 David Cook 2022-11-04 00:30:22 UTC
(In reply to Nick Clemens from comment #5)
> (In reply to David Cook from comment #4)
> > Confusion for whom?
> 
> For developers, or users. Why does problem reports have a subpermission, and
> edit authorities does not?

Ahh, I understand now. That is confusing. My only thought is that the original developer thought that additional sub-permissions might be added?

Aleisha could probably speak to that for recalls. 
 
> > What do you mean by "two settings"?
> 
> flagsrequired   => { recalls => "manage_recalls" }
> flagsrequired => { recalls => 1 }
> 
> These are checking two different things, but provide the same answer

That's true (where there is only one sub-permission like recalls). 

If additional sub-permissions were added, they would mean different things. I suppose that is both a pro and a con...

> > For "OPAC problem reports management" and "Recalls", you get the same thing
> > whether you click the top level or the sub-permission, and that's clearly
> > indicated visually.
> 
> Sure, but then why is the subpermission added at all? If it is the same as
> clicking the top level, let's not provide two checkboxes when one will do

I think that's the crux of the matter. 

I suppose one option could be removing unnecessary sub-permissions? 

Alternatively, we could also grey-out sub-permissions where there's only 1 sub-permission available. That might make it less confusing visually?
Comment 7 Katrin Fischer 2022-11-17 10:01:29 UTC
(In reply to David Cook from comment #6)
> (In reply to Nick Clemens from comment #5)

> > > For "OPAC problem reports management" and "Recalls", you get the same thing
> > > whether you click the top level or the sub-permission, and that's clearly
> > > indicated visually.
> > 
> > Sure, but then why is the subpermission added at all? If it is the same as
> > clicking the top level, let's not provide two checkboxes when one will do
> 
> I think that's the crux of the matter. 
> 
> I suppose one option could be removing unnecessary sub-permissions? 
> 
> Alternatively, we could also grey-out sub-permissions where there's only 1
> sub-permission available. That might make it less confusing visually?

I think you are right, having a module permission with only one sub-permission makes no sense. Recalls should be a top-level permission without sub-permissions and then when something requires a specific permission, a 'remaining recalls permissions' should be added as a second sub-permission, like we have elsewhere (circulate for an example)
Comment 8 Nick Clemens (kidclamp) 2022-11-17 15:01:00 UTC
(In reply to Katrin Fischer from comment #7)
> (In reply to David Cook from comment #6)
> > (In reply to Nick Clemens from comment #5)
> 
> > > > For "OPAC problem reports management" and "Recalls", you get the same thing
> > > > whether you click the top level or the sub-permission, and that's clearly
> > > > indicated visually.
> > > 
> > > Sure, but then why is the subpermission added at all? If it is the same as
> > > clicking the top level, let's not provide two checkboxes when one will do
> > 
> > I think that's the crux of the matter. 
> > 
> > I suppose one option could be removing unnecessary sub-permissions? 
> > 
> > Alternatively, we could also grey-out sub-permissions where there's only 1
> > sub-permission available. That might make it less confusing visually?
> 
> I think you are right, having a module permission with only one
> sub-permission makes no sense. Recalls should be a top-level permission
> without sub-permissions and then when something requires a specific
> permission, a 'remaining recalls permissions' should be added as a second
> sub-permission, like we have elsewhere (circulate for an example)

Just to add, I don't like 'remaining' permisssions - it is very unclear to the end user what exactly these are - I know they are 'everything not listed elsewhere' but it comes off as somewhat ambiguous
Comment 9 David Cook 2022-11-18 02:03:20 UTC
(In reply to Nick Clemens from comment #8)
> Just to add, I don't like 'remaining' permisssions - it is very unclear to
> the end user what exactly these are - I know they are 'everything not listed
> elsewhere' but it comes off as somewhat ambiguous

Agreed. Circulation could benefit from more granular permissions, or even a rename of that one I suppose.
Comment 10 David Cook 2022-11-18 02:10:55 UTC
(In reply to Nick Clemens from comment #8)
> Just to add, I don't like 'remaining' permisssions - it is very unclear to
> the end user what exactly these are - I know they are 'everything not listed
> elsewhere' but it comes off as somewhat ambiguous

Of course, Koha's permission system could use a complete overhaul anyway.

Personally, I'd like a policy based access control that evaluated subject, object, operation, and perhaps environment.