Missing Permissions omnibus bugfix package Nov 2015

[Josh Stompro]

EG 2.7+

There are several(7) bugs having to do with permissions being missing from the default install. This bug aims to combine them all into one update to make them easier to test.

Some of the issues that need to be considered are:
  - Adding permissions that may already have been added locally.
  - Adding permissions to default permission groups.
  - Renumbering locally added permissions to align with what the seed data
     has.
  - Required PGTap tests.

Bugs to include:
  - https://bugs.launchpad.net/evergreen/+bug/1488511
      ADMIN_PROXIMITY_ADJUSTMENT

  - https://bugs.launchpad.net/evergreen/+bug/1512486
      MARK_ITEM_DAMAGED

  - https://bugs.launchpad.net/evergreen/+bug/1299148
      Authorities Control Sets

- Overrides
  - https://bugs.launchpad.net/evergreen/+bug/1254826
     PATRON_EXCEEDS_LOST_COUNT

  - https://bugs.launchpad.net/evergreen/+bug/1320301
     MAX_HOLDS

  - https://bugs.launchpad.net/evergreen/+bug/1456301
     ITEM_DEPOSIT_REQUIRED
     ITEM_DEPOSIT_PAID

  - https://bugs.launchpad.net/evergreen/+bug/1479371
    COPY_STATUS_LOST_AND_PAID

  - Others without bug reports
    ITEM_NOT_HOLDABLE - Allow a user to place a hold when a copy is not
     holdable.
    ACTOR_USER_DELETE_OPEN_XACTS - Allow a user to delete a patron who
     has open transactions.

If there are any that I missed please let me know and I'll add those also.
Josh

Edit: 2015-11-21 - changed formatting for readability. Added Permissions that mmorgan brought up.

[Michele Morgan (mmorgan)]

Here are a few more that are missing:

Overrides:
ITEM_NOT_HOLDABLE - Allow a user to place a hold when a copy is not holdable
ACTOR_USER_DELETE_OPEN_XACTS - Allow a user to delete a patron who has open transactions
ITEM_DEPOSIT_REQUIRED - Allow a user to check out an item requiring a deposit
ITEM_DEPOSIT_PAID - Allow a user to check out items that require a rental fee

[Jane Sandberg (sandbergja)]

Here are some more that are missing:

CREATE_AUTHORITY_CONTROL_SET
UPDATE_AUTHORITY_CONTROL_SET
DELETE_AUTHORITY_CONTROL_SET

[Jane Sandberg (sandbergja)]

I also didn't see CREATE_AUTHORITY_RECORD or UPDATE_AUTHORITY_RECORD in my 2.9.1 install.

[Jason Boyer (jboyer)]

Adding a little comment heat here in case anyone is close with these changes. Since I just had to add ITEM_NOT_HOLDABLE.override to our system this is fresh on my mind. I can look into adding these if no one else does soon.

[Jason Boyer (jboyer)]

I've added the known missing permissions I could find to the stock data and tried to make reasonable choices about where they might be assigned. (Given how much EVERYTHING is thrown around in there I suspect the stock perm groups are rarely used...) The upgrade script is fairly basic, it expects everything to just work, that may be open to improvement, but there's no reason to miss out on fixing the stock data now.

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/jboyer/lp1517137_missing_perms

[Jason Boyer (jboyer)]

OH, also, authority.record_entry had biblio.record_entry perms assigned in fm_IDL.xml, that was also corrected to make use of the authority versions of those perms (which the code had been checking for already...)

[Michele Morgan (mmorgan)]

Another missing perm that came up recently in IRC:

ITEM_RENTAL_FEE_REQUIRED.override - Allows a user to check out an item with a rental fee.

Link to IRC discussion:

http://irc.evergreen-ils.org/evergreen/2017-02-01#i_286680

[Jason Boyer (jboyer)]

That branch has been force pushed with ITEM_RENTAL_FEE_REQUIRED.override added to it. Thanks for the heads up!

[Kathy Lussier (klussier)]

Thanks Jason! The seed data looks good. I'm not sure about the upgrade script. I added a small handful of the permissions to my test system before running the upgrade script. The upgrade script gives a duplicate key error and none of the permissions are applied.

Since most sites have probably added at least one of these permissions manually, I imagine they won't get any of these permissions added once they hit that error. Does anyone have thoughts on a better way to add the permissions through the upgrade script? Or would we be better off adding the permissions to the seed data, but not doing the upgrade script?

[Chris Sharp (chrissharp123)]

I think providing an optional/supplemental upgrade script and raising a notice about it when creating the version upgrade script that contains the change would be the best approach. Something like "The following permissions were not present in stock Evergreen installs before this change: <list of perms>. Please edit add_a_bunch_of_perms.sql to comment out any permissions already present in your database then run the script to add the rest."

[Jason Boyer (jboyer)]

I had a couple thoughts about the upgrade script. (I wanted to get this out asap and so didn't *implement* any of them, but I did have them, heh.)

What came to mind first is that if you added a permission manually that you knew to be missing you could expect to have to deal with conflicts when that fix comes down the line. That's admittedly lazy thinking and not very friendly to less experienced admins. So something else should be done instead.

Which leaves these two options:

In the upgrade create individual insert statements, don't use a transaction, and \echo that it's ok if these things fail. There have been (parts of?) scripts that have done that in the past. This is the simple/safe way to do it although personally I like for code to be able to depend on "system" things being in predictable places so this isn't my preference. (Perms aren't quite as important as things like billing reasons in this regard, but given my recent issues with coded value maps I have become much more picky about such things.)

Or, use a DO block to rename currently conflicting perms with ids > 1000, insert the id < 1000 versions, and replace any mappings under permission.* to use the new perms instead of the old and then remove the local ones. This obviously requires more testing and has more potential for issues than the above, but is entirely DOable (heh).

So what say ye: let the perms fall where they may or tidy up the place?

[Jason Boyer (jboyer)]

Also, re: pgTAP, I think a major permissions test would be the way to do it, with a (very) large script checking for the presence of permissions by feature. (so the C/R/U_AUTHORITY_RECORD would be 1 test for auth records, C/R/U_POP_BADGES/_POP_PARAMATER for popularity badges, etc.) *I'm* not going to have time for that before 2.12 comes out, but I do think it would be more valuable than a test for a specific grab bag of permissions missed in the past.

[Kathy Lussier (klussier)]

Jason,

I would be okay with either of the two options you suggested. The 2nd seems like it would be the 'more correct' way to approach it, but it sounds like it would be a lot more work. IMO, #1 seems to be good enough for our purposes.

[Jason Boyer (jboyer)]

The upgrade script has been upgraded to no longer require any admin intervention. If you've added one of these perms manually it's moved out of the way, the expected permission inserted, your maps updated, and then the old permission is removed. If you haven't added one or more, they're simply inserted and you're off.

[Kathy Lussier (klussier)]

Thank you Jason! Merged to master for inclusion in 2.12.

[Ryan Eby (aadl-ubuntu)]

Note for anyone that comes across this bug while searching: This script does not include ADMIN_PROXIMITY_ADJUSTMENT as stated in the original bug report. That permission appears to still be lacking completely in the current SQL code.