permission model cleanup + util tests #29657
Conversation
| def test_can_manage_releases(self): | ||
| self.permissions = Permissions(manage_releases=False) # manage_releases is True by default | ||
| self.assertFalse(can_manage_releases(self.web_user, self.domain, "app_id")) | ||
| self.permissions = Permissions(manage_releases=False, manage_releases_list=["app_id"]) |
There was a problem hiding this comment.
Seems redundant and/or misleading to specify manage_releases=False when passing manage_releases_list=[...]. Would it be out of scope to change that in this PR?
There was a problem hiding this comment.
It's not quite redundant since a value of True indicates that permission is granted to all items (regardless of what's in the list). If we change this to ignore the boolean if there are items in the list we will also have to ensure that the list is cleared / set to null. I did debate this in the design doc (https://docs.google.com/document/d/1wVxmo-12dPIMwei2rn-6X9SL3GU8_KJLnY0VeVWlXdA/edit?disco=AAAAMTin2uQ) and decided to keep the boolean rather than solely relying on the the list since it is more explicit.
I guess ideally the boolean would be named manage_all_releases
There was a problem hiding this comment.
Another option would be to have the default be a non-list constant like manage_releases=ALL_APPS. The manage_releases argument would either accept that constant or a list (similar to how many arguments accept None or some other value), with an empty list meaning the same as manage_releases=False. This could be supported by a docstring.
The problem with the existing implementation is that it allows ambiguous arguments (manage_releases=True, manage_releases_list=["app_id"]), and since manage_releases=True is the default, passing only manage_releases_list without manage_releases is ambiguous.
There was a problem hiding this comment.
@millerdev since this is the actual couch doc I'm not going to make the change but once the migration is complete we can change how the python API works. One thing I'd like to clarify now though is whether you think this change should be made to the SQL model: https://github.com/dimagi/commcare-hq/pull/29688/files#diff-a4894a64fa9396fb6c911d9c9b7804a48ea00d4359d179cc49f84490312aca78R78-R93
Personally it don't think it should since in the DB we are restricted by types so we'd need to interpret NULL as 'allow all' which I think is ambiguous.
There was a problem hiding this comment.
I was referring to the Python API only, not the way it is stored in the database. I do think it makes sense to have a separate boolean column for allow_all in conjunction with a check constraint of not (allow_all and allowed_items).
| column values | check result |
|---|---|
allow_all = True; allowed_items = [] |
TRUE |
allow_all = False; allowed_items = [] |
TRUE |
allow_all = True; allowed_items = ["stuff"] |
FALSE (not allowed) |
allow_all = False; allowed_items = ["stuff"] |
TRUE |
Summary
Removing some unused code + adding some tests
Safety Assurance
Automated test coverage
Some existing tests + some new ones.
Safety story
remove ability to set report permission via 'view_report'
I have reviewed every call or reference to this function as well as the code that saves / updates roles. I've also tested making changes via the UI.
simplify view_web_app
Purely a stylistic change
remove Permission.set
Function is unused (as is evident via audit of all usages of
.set(). Function added in beaf755. Last usage removed in c37bad5remove redundant permission check
Removing a redundant check. Function is covered by unit tests.
test for can_manage_releases
Addition of tests cases
remove unused arg
Function only used in 2 places and neither make use of the
is_archivedargument.remove irrelevant comments
Comments only.
remove ability to 'or' permissions
This was only used when there were multiple sets of permissions per user. It is no longer used.
Rollback instructions