Feature/add whitelisting on certificate preset#91
Feature/add whitelisting on certificate preset#91alexbourret wants to merge 16 commits intomasterfrom
Conversation
MayeulRousselet
left a comment
MayeulRousselet
left a comment
There was a problem hiding this comment.
As for now, it is impossible to have only a white list on Library and not on Lists.
I'll advice to apply white list if and only a white list is not empty.
My use case:
I want only a white list on libraries, not on List.
With this settings, I can't read/write to any list.
There was a problem hiding this comment.
Seems to work fine from a quick test - but a few changes to make I think
At least one label on the UI is wrong
I'm not sure about the naming as "whitelist" is considered problematic.
Also confusion of libraries and folders maybe?
Suggestions in review.
Another thing - having write but not read is a bit weird - perhaps rather thana multiselect we can have just a choice of either/or?
I'll do another test after the changes are made
| "subParams": [ | ||
| { | ||
| "name": "whitelist_name", | ||
| "label": "Library name", |
There was a problem hiding this comment.
Is this a typo? Should it say list name?
There was a problem hiding this comment.
(will be taken care of automatically if naming changed)
| }, | ||
| { | ||
| "name": "libraries_whitelist", | ||
| "label": "Whitelisted libraries", |
There was a problem hiding this comment.
From a naming POV this a bit confusing. Also "whitelist" is itself no longer considered the best term for this on "Inclusion and Diversity" grounds - that is the standard practice today and considered a risk for Enterprise & B2B software.
Moving away from it also means we can also avoid using the word "list" where it could be a bit confusing.
For activation param I suggest:
"name": "activate_allowlists",
"label": "Allowlists",
So, re
"name": "libraries_whitelist",
This param seems to be about folders, not libraries. The example here:
"description": "/sites/YourSite/Shared Documents/your folder path"
Is pointing to a folder, not a document library. "Shared Documents" is a document library, anything under it is not. Perhaps it covers both, but calling it Library is probably confusing, folder is better even if referring to both I think.
So here I suggest:
"name": "allowed_folders",
"label": "Allowed folders",
Re
"name": "whitelist_name",
"label": "Library name",
As mentioned above this seems to be a folder path not a name, so I suggest:
"name": "allowed_folder_path",
"label": "Folder path",
"description": "/sites/YourSite/Shared Documents/your folder path"
For the rights subparam:
"name": "allowed_folder_rights",
"label": "Access rights",
For the lists param, I'd suggest for naming the param as a whole:
"name": "allowed_lists",
"label": "Allowed lists"
For name subparam:
There's a mention of libraries here, but I think it is an accident?
I suggest
"name": "allowed_list_name",
"label": "List name",
description": "List name from the list's SharePoint URL"
(this is sure the name -so not display name or ID? As a GUID ID also exists for the list, we should be clear we are not talking about that)
For rights subparam:
"name": "allowed_list_rights",
"label": "Access rights",
| self.app_certificate = { | ||
| 'libraries_whitelist': [ | ||
| { | ||
| '$$hashKey': 'object:540', |
There was a problem hiding this comment.
We can remove these probably? Tests pass without them
'$$hashKey': 'object:540',
I think they're an artifact of angular JS or something.
| if not self.activate_white_list: | ||
| return True | ||
| if isinstance(path_to_test, list): | ||
| for path_size in range(len(path_to_test) + 1, 0, -1): |
There was a problem hiding this comment.
✍️ Maybe put a comment just to say - match with path or subpath (anything underneath considered allowed). I was feeling dense and didn't get it straight away
also ⛏️
We don't need the +1 here:
len(path_to_test) + 1,
It's confusing
| def can_write_list(self, list_name): | ||
| return self.can_do("write", self.lists_whitelist, list_name.lower()) | ||
|
|
||
| def can_do(self, required_right, rights, path_to_test): |
There was a problem hiding this comment.
⛏️ path_to_test isn't always a path, sometimes it is a list? Might be worth making that clearer somehow?
Maybe just make it item_to_test - and say folder paths will be list type, sharepoint lists will be strings
I mean it is obvious once you know but it was confusing at first glance...
|
|
||
| def assert_can_read_path(self, path): | ||
| if not self.can_read_path(path): | ||
| raise Exception("This preset does not have read access to '{}'".format(path)) |
There was a problem hiding this comment.
❓ I wonder, should we make it clear that this is the preset config rather than the rights it has on sharepoint? I guess we shouldn't even though it might be easier from a troubleshooting POV?
Hang on, how do we want this to work? With this strategy: "apply white list if and only a white list is not empty", applied separately to folders and the lists, this would not be possible. If we want the flexibility just to restrict folders but not lists (or vice versa), it's better to have two toggles/checkboxes for the two allow-lists (which I think is what Alex has done now), and NOT assume all are allowed because a whitelist is empty, as we may indeed want to (for example) exclude ALL lists while allowing a single folder. |
3 participants
No description provided.