[CoE] Security Roles - drift report & config #348
Conversation
There was a problem hiding this comment.
Got me a test vCenter up and running finally, so I was really able to bang on this. Generally, it looks good! But... only for the "happy path" 😂
Once things go sideways here, we see a lot of stack traces, instead of (helpful) error messages.
I think I provided all of the commands that I used to run/discover different unexpected behaviors... let me know if you have any problems reproducing 👍
| return _get_privilege_group_descriptions(authorizationManager) | ||
|
|
||
|
|
||
| def find(role_name=None, service_instance=None, profile=None): |
There was a problem hiding this comment.
suggestion This should either get or actually search. For instance, on my current test instance, vcenter_roles.find returns one called "Resource pool administrator (sample)".
However, vcenter_roles.find administrator doesn't return any results. I have to seek for vcenter_roles.find "Resource pool administrator (sample)" in order to get the role, which is get rather than find, since I need to know the name first.
If it's important to be able to allow exact matches, I'd do something like add exact=False to the arguments, and then add a bit of logic to catch all that.
| if role_name is not None and role_name != role.info.label: | ||
| continue |
There was a problem hiding this comment.
If changing this to actually search, it would be:
if role_name and role_name not in role.info.label:
continue
This would treat an empty string as role_name as the same as None, which is probably expected behavior.
There was a problem hiding this comment.
Also for a case insensitive match: role_name.lower() not in role.info.label.lower() (or .upper())
| if group not in new_privileges_by_groups: | ||
| new_privileges_by_groups[group] = [] | ||
| privileges_in_group = role_config["privileges"][group] | ||
| for priv in group_privileges[group]: |
There was a problem hiding this comment.
suggestion try this with a non-existent group, e.g. salt-call vcenter_roles.save '{"role": "Blerps", "privileges": {"Fnord": ["Create resource pool", "something else", "cool guy"]}}'
You should see this raise a KeyError, which is not so good 😅
|
|
||
| def config(name, config, service_instance=None, profile=None): | ||
| """ | ||
| Get/Set roles configuration based on drift report. |
There was a problem hiding this comment.
suggestion This should read something like Set vCenter role configuration. -- it may be happening due to something on a drift report, but it's not necessary. One can simply use this to enforce their roles configuration.
| .. code-block:: json | ||
| (vim.AuthorizationManager.Role) { |
There was a problem hiding this comment.
suggestion need a blank line and then indention x4 spaces for code blocks.
There was a problem hiding this comment.
(the only reason this didn't cause an error is that docstrings for things in tests aren't built as part of our test runs)
There was a problem hiding this comment.
I rely on docstrings to catch it, I'll pay more attention about this in the tests
done for both tests
| # Can't mock name via constructor: https://docs.python.org/3/library/unittest.mock.html#mock-names-and-the-name-attribute | ||
| mock = Mock(*args, **kwargs) | ||
| mock.name = name | ||
| return mock |
There was a problem hiding this comment.
suggestion (non-blocking) when this change Onemind-Services-LLC@8117573 gets merged in this will want to change. TBH if that PR doesn't get updated by tomorrow afternoon I'll probably go ahead and split that out.
| profile="vcenter", | ||
| ) | ||
| # comparing 2 dict with '==' fails, because of inner lists with different orders, use drift.drift_report | ||
| assert drift.drift_report(ret[0], old_role[0]) == {} |
There was a problem hiding this comment.
suggestion I'd like to see a test in here that just makes sure there is stuff in this drift report. It's entirely possible for an erroneous mock to make this pass all the time.
Maybe below before line 304, assert drift.driftt_report(ret[0], old_role[0]) == expected_drift, something like that.
No description provided.