Users command: option to report on legal hold membership #290
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ |
CHANGELOG.md
Outdated
| @@ -16,6 +16,8 @@ how a consumer would use the library (e.g. adding unit tests, updating documenta | |||
|
|
|||
| - New command `code42 users remove-role` to remove a user role from a single user. | |||
|
|
|||
| - New command 'code42 users list --include-legal-hold-membership' to print the legal hold matter name and ID for any user on legal hold | |||
There was a problem hiding this comment.
I know this has probably already been said, but what is the use-case for this functionality? I had heard most customers have a single matter for legal hold.
Is this flag general enough or this just for a specific use-case? I am also wondering if this is the right place to include this flag... What if I wanted to filter users by matter ID instead? Or what if I want to know which detection list or alert rules each user belong to as well?
There was a problem hiding this comment.
The primary use case that this addresses is "show me all the users on legal hold."
There are customers that have a lot of legal hold matters. Therefore, the legal-hold show <matter ID> command would not meet their needs if they need to see users on all legal holds.
Also, legal-hold show <matter ID> produces the users in column format. This makes sense for a show command, which is meant to be a high-level overview (This was discussed in the PR that added the devices list --include-legal-hold-membership). However, for an admin to work with the data and put it in different formats, the list command provided some more flexibility (at least for device info)
There was a problem hiding this comment.
I'm good with this being in the CLI proper. As it could be useful to any customer using legal hold.
I think I'd also be ok with future flags for including if a user is on a detection list as well. Having the CLI users command be a kind of one-stop reporting tool to get all users info makes sense. Especially for larger customers who might have multiple teams (or automations) managing who gets put on what list/hold.
|
I have read the CLA Document and I hereby sign the CLA |
| .agg(",".join) | ||
| .rename( | ||
| { | ||
| "legalHold.legalHoldUid": "legalHoldUid", |
There was a problem hiding this comment.
It would be nice to be able to pass in user friendly keys for Table format headers for when using DataFrameOutputFormatter, as that was the norm before.
Table format should use label-esque keys while all the other ones should use raw API values.
| assert "Legal Hold #1,Legal Hold #2" in result.output | ||
| assert "123456789,987654321" in result.output | ||
|
|
||
|
|
There was a problem hiding this comment.
Can we get tests for the following two cases:
1.) A user is not on any legal hold but the --include-legal-hold-membership flag was passed in (keys should be there but their values should be None)
2.) A user is on a legal hold but the flag --include-legal-hold-membership was not passed in (keys and values should not be present in the output)
There was a problem hiding this comment.
I added these tests. The code didn't handle cases where 0 custodians were returned. Thanks for suggesting this test case.
I also added a test for cases where the flag--include-legal-hold-membership and no matters were returned.
|
The build failures will go away if you catch your branch up with upstream |
|
when using Table format, the spacing between the end of the Users data in the beginning of the Legal Hold data is really really big... Even when the user is part of many matters, it is still almost twice as big as it needs to be. |
|
I am little curious what the legal hold data is a comma-separated string rather than a list type. For example, when I create a CSV, the license entry is Same with JSON. Would using lists be easier for programmatically handling the output for CSV and JSON outputs? |
|
Re: Build Failures Re: Column spacing for a long list of legal holds: Re: Legal Hold Info as a comma-separated string
However, I'm not sure how to programmatically have that display as a list for some formats, and as a string for the table. If I had to choose, I err on the side of making it a list for all formats, as opposed to the comma-separated string. |
tests/cmds/test_users.py
Outdated
|
|
||
|
|
||
| def custodian_list_generator(): | ||
| yield TEST_CUSTODIANS_RESPONSE |
There was a problem hiding this comment.
These should use the create_mock_response() method:
yield create_mock_response(mocker, data=TEST_CUSTODIANS_RESPONSE)
Anything that is a response from a py42 method should use this method, so all these ones here,
it is being used in some of the other fixtures below as an example!
Please and thank you! This is looking really good.
There was a problem hiding this comment.
@maddie-vargo Work with one of the other members of Literally Skynet on a good time to merge this, thanks for your contribution!
This PR addresses the final component of Issue #176 by adding functionality to the
userscommand to report on legal hold membership. This option will report on the legal hold matter name and ID for any user on legal hold. This work largely mimics the legal hold reporting option in thedevicescommandTesting Procedure
PR Checklist
Did you remember to do the below?