Remove UPDATE_ORG_SETTING permission

[Susan Morrison]

Confirmed the UPDATE_ORG_SETTING permission is unused, so it can be removed.

[Terran McCanna (tmccanna)]

I've made a stab at this. If I've done it right, the upgrade script should first remove the setting from any of the tables where it could have been referenced (according to the database schema), and then remove the setting itself.

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/mccanna/lp2043218_remove_perm_update_org_setting

To test on a new install:
1) Verify that the setting does not appear under Admin > Server Admin > Permissions

To test on an existing install:
1) Apply the setting to a user permission group and to an individual user (or identify ones where it is already applied)
2) Run the upgrade script
3) Verify that the permission has been removed from the permission group, from the user, and from the list of possible permissions

(you could also try manually creating a new permission with the same name and then running the upgrade script to be sure it deletes it)

Did I miss anything? Feedback desired!

[Jeff Davis (jdavis-sitka)]

Hi Terran! Thanks for the patch and the test plan.

I don't think the permission is unused; I think it's intended as the generic update_perm for org unit settings. In my environment, we only really want library admins to be able to update most org settings, so we assign UPDATE_ORG_SETTING to our library admin permission groups and then use that as the update_perm for those settings. For settings where we need more granular permission control, we use a different update_perm, but it's handy to have a generic option. So in my opinion, we should keep the perm.

Looking at the patch itself, I see one small problem and one bigger one.

The small problem is that we generally don't modify past upgrade scripts, so I think the changes to Open-ILS/src/sql/Pg/upgrade/0026.data.reserve_perm_list_id_range.sql and Open-ILS/src/sql/Pg/version-upgrade/1.6.1-2.0-upgrade-db.sql should be omitted.

The bigger problem is these three lines in the upgrade script:

DELETE FROM config.org_unit_setting_type WHERE view_perm = uosid OR update_perm = uosid;
DELETE FROM config.remoteauth_profile WHERE perm = uosid;
DELETE FROM config.z3950_source WHERE use_perm = uosid;

The intention here is to stop using the perm anywhere it's in use, but what these statements are actually doing is deleting any rows from those tables that happen to use the perm. For example, if I have an org setting that uses UPDATE_ORG_SETTING as an update_perm, that first line won't remove the update_perm, it will delete the setting altogether! Rather than deleting rows from these tables, you would need to set the permission to some different value. You could set it to NULL, but that would probably leave things in an undesirable state (if I was using UPDATE_ORG_SETTING as an update_perm, setting update_perm to null means now anybody can update those settings, which isn't what I want).

The remaining DELETE statements, from permission.grp_perm_map and so on, are OK because you're just deleting mappings for the perm; there are no unwanted side effects in that case.

Hope that's helpful! Let me know if I can clarify anything.

[Terran McCanna (tmccanna)]

Thank you, Jeff, that is really helpful!

Just so I understand better, can you tell me where the code still references the perm as a default? We were unable to find it still in use anywhere, so maybe we are overlooking one of the places we should be looking for it.

[Terran McCanna (tmccanna)]

Never mind, I am studying the Org Unit Setting Type table and see what you mean now!

[Terran McCanna (tmccanna)]

Upon further review of the 3 config tables, I can confirm that a new installation is setting UPDATE_ORG_SETTING as the update perm for circ.permit_renew_when_exceeds_fines (in Server Admin > Org Unit Setting Types), so if this perm were to be removed, the update perm for that setting would need to be changed to something else. I don't see any other instances where it is being set for new installs, but I could be missing something.

For existing installs, I can see where it would be a problem for Jeff's use case. We don't use the setting at all in PINES, but it makes sense for different scenarios. In looking further, there is also an UPDATE_ORG_UNIT_SETTING_ALL permission that is used more widely. I wonder if the fact there are two settings that sound like they should do the same thing was intentional or accidental? If accidental, then a solution could be to have the upgrade script swap out any existing usage of UPDATE_ORG_SETTING for UPDATE_ORG_UNIT_SETTING_ALL.

At this time,

[Terran McCanna (tmccanna)]

Darn accidental submit ...

At this time, I won't move forward with anything, I'll just put a needsdiscussion on this