-
Notifications
You must be signed in to change notification settings - Fork 7
Feature/add whitelisting on certificate preset #91
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 10 commits
aaeb3f5
5977da8
31c2f94
2bd63f1
1d36311
4fd49e5
3cb97f0
45fa2c3
71842d6
669207e
a5c089c
a4a82db
9e5bf1f
b6c8a4f
299e0f4
7735e6c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -63,6 +63,74 @@ | |
| "type": "PASSWORD", | ||
| "description": "If required by private key", | ||
| "mandatory": false | ||
| }, | ||
| { | ||
| "name": "activate_whitelist", | ||
| "label": "Whitelists", | ||
| "type": "BOOLEAN", | ||
| "description": "Not advised: access rights should be handled at Azure app level" | ||
| }, | ||
| { | ||
| "name": "libraries_whitelist", | ||
| "label": "Whitelisted libraries", | ||
| "type": "OBJECT_LIST", | ||
| "description": "", | ||
| "visibilityCondition": "model.activate_whitelist === true", | ||
| "subParams": [ | ||
| { | ||
| "name": "whitelist_name", | ||
| "label": "Library name", | ||
| "type": "STRING", | ||
| "description": "/sites/YourSite/Shared Documents/your folder path" | ||
| }, | ||
| { | ||
| "name": "whitelist_rights", | ||
| "label": "Access rights", | ||
| "type": "MULTISELECT", | ||
| "description": "", | ||
| "selectChoices": [ | ||
| { | ||
| "value": "read", | ||
| "label": "Read" | ||
| }, | ||
| { | ||
| "value": "write", | ||
| "label": "Write" | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| }, | ||
| { | ||
| "name": "lists_whitelist", | ||
| "label": "Whitelisted lists", | ||
| "type": "OBJECT_LIST", | ||
| "description": "", | ||
| "visibilityCondition": "model.activate_whitelist === true", | ||
| "subParams": [ | ||
| { | ||
| "name": "whitelist_name", | ||
| "label": "Library name", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this a typo? Should it say list name?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (will be taken care of automatically if naming changed) |
||
| "type": "STRING", | ||
| "description": "List ID from the list's SharePoint URL" | ||
| }, | ||
| { | ||
| "name": "whitelist_rights", | ||
| "label": "Access rights", | ||
| "type": "MULTISELECT", | ||
| "description": "", | ||
| "selectChoices": [ | ||
| { | ||
| "value": "read", | ||
| "label": "Read" | ||
| }, | ||
| { | ||
| "value": "write", | ||
| "label": "Write" | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| from safe_logger import SafeLogger | ||
|
|
||
| logger = SafeLogger("sharepoint-online plugin") | ||
|
|
||
|
|
||
| class WhiteList(): | ||
| def __init__(self, config=None): | ||
| self.config = config or {} | ||
| self.activate_white_list = self.config.get("activate_whitelist", False) | ||
| self.libraries_whitelist = {} | ||
| self.lists_whitelist = {} | ||
| libraries_whitelist = self.config.get("libraries_whitelist", []) | ||
| if self.activate_white_list: | ||
| for library in libraries_whitelist: | ||
| library_path = library.get("whitelist_name", "").strip("/").lower() | ||
| library_rights = library.get("whitelist_rights", []) | ||
| self.libraries_whitelist[library_path] = library_rights | ||
| lists_whitelist = self.config.get("lists_whitelist", []) | ||
| for list_item in lists_whitelist: | ||
| list_name = list_item.get("whitelist_name", "").lower() | ||
| list_rights = list_item.get("whitelist_rights", []) | ||
| self.lists_whitelist[list_name] = list_rights | ||
| logger.info("Whitelisting with libraries:{} and lists:{}".format(self.libraries_whitelist, self.lists_whitelist)) | ||
|
|
||
| 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)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❓ 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? |
||
|
|
||
| def assert_can_write_path(self, path): | ||
| if not self.can_write_path(path): | ||
| raise Exception("This preset does not have write access to '{}'".format(path)) | ||
|
|
||
| def assert_can_read_list(self, list_name): | ||
| if not self.can_read_list(list_name): | ||
| raise Exception("This preset does not have read access to the list '{}'".format(list_name)) | ||
|
|
||
| def assert_can_write_list(self, list_name): | ||
| if not self.can_write_list(list_name): | ||
| raise Exception("This preset does not have write access to the list '{}'".format(list_name)) | ||
|
|
||
| def can_read_path(self, path): | ||
| return self.can_do("read", self.libraries_whitelist, path.strip("/").lower().split("/")) | ||
|
|
||
| def can_write_path(self, path): | ||
| return self.can_do("write", self.libraries_whitelist, path.strip("/").lower().split("/")) | ||
|
|
||
| def can_read_list(self, list_name): | ||
| return self.can_do("read", self.lists_whitelist, list_name.lower()) | ||
|
|
||
| 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): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ⛏️ path_to_test isn't always a path, sometimes it is a list? Might be worth making that clearer somehow? |
||
| 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): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✍️ 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 ⛏️ |
||
| tokens_in_path = path_to_test[0:path_size] | ||
| path_chunk_to_test = "/".join(tokens_in_path) | ||
| right_for_path = rights.get(path_chunk_to_test, []) | ||
| if required_right in right_for_path: | ||
| return True | ||
| return False | ||
| else: | ||
| right_for_path = rights.get(path_to_test, []) | ||
| return required_right in right_for_path | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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",