[FOLIO-1153] Permissions consistency enforcement breaks CI builds Created: 21/Mar/18 Updated: 12/Nov/18 Resolved: 27/Mar/18 |
|
| Status: | Closed |
| Project: | FOLIO |
| Components: | None |
| Affects versions: | None |
| Fix versions: | None |
| Type: | Bug | Priority: | P1 |
| Reporter: | Wayne Schneider | Assignee: | Wayne Schneider |
| Resolution: | Done | Votes: | 0 |
| Labels: | ci, sprint34 | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | 1 day, 7 hours, 30 minutes | ||
| Original estimate: | Not Specified | ||
| Issue links: |
|
||||||||||||||||||||||||||||
| Sprint: | |||||||||||||||||||||||||||||
| Comments |
| Comment by Wayne Schneider [ 21/Mar/18 ] |
|
Marc Johnson reported: Symptoms Most of the builds failed with an error similar to: failed: [10.36.1.243] (item=ui-users.settings.permsets) => {"accept": "application/json", "accept_encoding": "identity", "changed": false, "connection": "close", "content": "Error attempting to update permissions metadata: Attempting to add non-existent permissions to user", "content_type": "text/plain", "failed": true, "host": "10.36.1.243:9130", "item": "ui-users.settings.permsets", "msg": "Status code was not [200, 422]: HTTP Error 500: Internal Server Error", "redirected": false, "status": 500, "transfer_encoding": "chunked", "url": "http://10.36.1.243:9130/perms/users/2408ae64-56ad-4177-9024-1e35fe5d895c/permissions", "user_agent": "ansible-httpget", "x_okapi_permissions": "[\"perms.users.item.post\"]", "x_okapi_request_id": "863699/perms", "x_okapi_tenant": "diku", "x_okapi_token": "eyJhbGciOiJIUzUxMiJ9.eyJzdWIiOiJkaWt1X2FkbWluIiwidXNlcl9pZCI6IjFhZDczN2IwLWQ4NDctMTFlNi1iZjI2LWNlYzBjOTMyY2UwMSIsInRlbmFudCI6ImRpa3UifQ.vrW69kPTGFlyAT6YJeddG8fP174wI-UH6mx3eWO70v4-D-Y0pQGTn1VegLoD5m6-caOY8jmMR74uUo_f1qTx3Q", "x_okapi_trace": "POST mod-authtoken-1.4.1-SNAPSHOT.20 http://10.36.1.243:9131/perms/users/2408ae64-56ad-4177-9024-1e35fe5d895c/permissions : 202 3580us, POST mod-permissions-5.1.0-SNAPSHOT.12 http://10.36.1.243:9132/perms/users/2408ae64-56ad-4177-9024-1e35fe5d895c/permissions : 500 8058us", "x_okapi_url": "http://10.36.1.243:9130", "x_okapi_user_id": "1ad737b0-d847-11e6-bf26-cec0c932ce01"}
folio-snapshot failed with a different error: fatal: [10.36.1.22]: FAILED! => {"changed": false, "connection": "close", "content": "POST request for mod-permissions-5.1.0-SNAPSHOT.12 /_/tenantpermissions failed with Internal Server Error: Unable to satisfy dependencies for permission inventory-storage.all", "content_length": "174", "content_type": "text/plain", "failed": true, "msg": "Status code was not [200]: HTTP Error 500: Internal Server Error", "redirected": false, "status": 500, "url": "http://10.36.1.22:9130/_/proxy/tenants/diku/install"}
However this looks to be due to a related cause (I haven’t dug deeply into the code to check this assumption). Initial Cause Investigation The first error is produced here: https://github.com/folio-org/mod-permissions/blob/8e72f8b525457f6d7be39f925e5f240ec2321250/src/main/java/org/folio/rest/impl/PermsAPI.java#L1652 The second error is produced here: https://github.com/folio-org/mod-permissions/blob/8e72f8b525457f6d7be39f925e5f240ec2321250/src/main/java/org/folio/rest/impl/TenantPermsAPI.java#L97 Both of which were added in the same commit, which I think was merged yesterday (https://github.com/folio-org/mod-permissions/commit/8e72f8b525457f6d7be39f925e5f240ec2321250) - interestingly git blame cites the original commit which makes finding when it changed in master harder) as part of https://folio-org.atlassian.net/browse/MODPERMS-29. It looks like we now reject assignments of permissions to users or permission sets where the permission does not exist. Further Investigation I’m starting to track down the permissions involved (and to confirm my understanding of the cause). I had a quick look at trying to add which permission(s) caused the failure, or were involved in the activity. I don’t think I’m familiar enough with the code to make those changes without risking introducing other issues. Kurt, I reckon you might be better placed to potentially do this, if you think it makes sense? Possible Interim Mitigation A tweak to mod-permissions to change the errors to be a 422 response should mean that folio-ansible marks these responses as success in some circumstances (the testing environment but not snapshot I think) which might temporarily mitigate some of the impact. A consequence of that could be to mask the underlying issue of having undefined permissions which are attempted to be being granted. |
| Comment by Wayne Schneider [ 21/Mar/18 ] |
|
Also from Marc Johnson: I thought I’d start with the folio-snapshot build error message as this gave me an immediate hint at where to look (and it is an area I am familiar with): I’ve found a typo in one of the permission names (see https://folio-org.atlassian.net/browse/MODINVSTOR-91) and am pushing a fix for that. Will run the folio-snapshot (and then folio-testing) build to see if that is the only discrepancy. (As an aside, I wrote a python script to help determine the discrepancies, with the help of a quick diff - https://github.com/k-int/mod-inventory-storage/blob/check-permissions/check-permission-discrepancies.py) |
| Comment by Wayne Schneider [ 21/Mar/18 ] |
|
See https://folio-org.atlassian.net/browse/MODPERMS-29?focusedCommentId=76802&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel for an explanation of the effects of the change. |
| Comment by Wayne Schneider [ 21/Mar/18 ] |
|
John Malconian has work on branch folio-1051a of folio-ansible that may take care of the issue of assigning permissions. |
| Comment by Wayne Schneider [ 22/Mar/18 ] |
|
Temporary workaround: pinned the folio-testing and folio-snapshot builds to mod-permissions-5.0.1-SNAPSHOT.11. Work is in progress on refactoring the Ansible build to use the tenant-admin-permissions role to assign admin permissions. Once the blocking issues are closed, that work should be able to be merged into master and we can remove the pinning for mod-permissions. |
| Comment by Wayne Schneider [ 22/Mar/18 ] |
|
With the workarounds in place, I believe the blocked circ issues can be unblocked – Marc Johnson, Jakub Skoczen? |
| Comment by Marc Johnson [ 23/Mar/18 ] |
|
Wayne Schneider Yup, I'll be unblocking and testing them this morning |
| Comment by Mike Taylor [ 23/Mar/18 ] |
|
I see that one of the blockers,
This can't be the right solution, can it?. The module needs notes.domain.requests to make and maintain notes, does it not? |
| Comment by Marc Johnson [ 23/Mar/18 ] |
|
Mike Taylor That is an example of an interesting scenario. (Including Heikki Levanto Kurt Nordstrom Matt Connolly and Jakub Skoczen as people who might have more knowledge / opinion about this) The current changes to mod-permissions means that unknown permissions cannot be used, and as far as I can tell notes.domain.requests is not defined in any module. What might make this especially interesting is the approach within mod-notes to use a wildcard permission mechanism ("permissionsDesired": ["notes.domain.*", "notes.domain.all"]). Which allows different domains of notes to be defined elsewhere to mod-notes. Therefore, the initial question might be (assuming we want this late binding of domains for mod-notes and strong references to permissions within mod-permissions) where could / should those domain permissions be defined? In this specific case, I guess UI requests could define the permission itself? Or to define the supported domains within mod-notes? |
| Comment by Mike Taylor [ 23/Mar/18 ] |
|
Thanks, Marc, that's a nice summary of the problem. I think the best solution is for the server-side module to define the permission for managing notes for that module – and then the client module can refer to that permission, including it in a higher-level one, if it wants to. For example, mod-users would define notes.domain.users, and ui-users would be at liberty to include this in ui-users.allPerms or something. |
| Comment by Heikki Levanto [ 23/Mar/18 ] |
|
Yes, I was thinking along those lines when I designed the notes permissions. |
| Comment by Wayne Schneider [ 23/Mar/18 ] |
|
In this particular case, because ui-requests.all contains the subPermission notes.domain.all, removing notes.domain.requests from the permissionSet should not break anything. That said, ui-requests.all probably shouldn't have notes.domain.all included in it. I note that, AFAICT, no other module makes use of notes.domain.* permissions, and no atomic permissionSets for notes domains have been defined in any module that I could find. I have raised issue
|
| Comment by Wayne Schneider [ 25/Mar/18 ] |
|
Changes merged to folio-ansible and folio-infrastructure. |