New Command - Tenant Audit Log Report. Closes #1739 by arjunumenon · Pull Request #2001 · pnp/cli-microsoft365

arjunumenon · 2020-12-11T15:33:55Z

New Command - 'tenant auditlog report'. Closes #1739

Added new command 'm365 spo tenant auditlog report' command which retrieves Audit report using Office 365 Management API. Closes New command: tenant auditlog report #1739

Linked Issue

Closes #1739

arjunumenon

Hey @waldekmastykarz / @rabwill -
Currently I am kind of in a deadlock situation and my thoughts are clouded in a finding an approach for a case.

I am trying to get a result by using Promises.all for generating the audit log.
Since we cannot execute everything in a single Promises.all (We will have around 200 REST API call), we may have to batch that and have execute Promises.all for each batches pf 10.
It will be great if you can have a quick look and let me know how can I take it forward.
Had been struggling with that for couple of days and it seems my thoughts have become clouded 💭unfortunately.

It will be super helpful if you can have a quick look and let me know the direction on that please. 🤞

arjunumenon · 2020-12-17T10:45:53Z

Hey @waldekmastykarz / @rabwill - I have initiated the PR finally. Since the implementation is complicated, I am expecting it to have large number of review comments. Have tried my best to follow the guidelines.

Thanks for all the guidance so far. Looking forward for the comments 🤞.

waldekmastykarz · 2020-12-18T09:35:51Z

I appreciate your effort! We'll review it shortly 👍

plamber

Hi @arjunumenon,
thank you very much for your valuable work. I tested the command and have some minor suggestions on how to improve the current PR.

Let me know if you have any questions.

Wish you a great 2021.

plamber · 2020-12-31T09:10:14Z

+        if (StartEndDateDifference != 1 || StartdateTodayDifference > 7) {
+          return `startTime and endTime must be less than or equal to 24 hours apart, with the start time prior to end time and start time no more than 7 days in the past`;
+        }


If I understand the API correctly we want to allow persons to search within a day. With this limitation we can't do it.

I cannot perform for example such a search: 2020-12-29T15:00:00 and 2020-12-29T16:00:00.

I think this part should be changed to ((StartEndDateDifference > 0 && StartEndDateDifference <= 1) || StartdateTodayDifference > 7)

That is totally correct @plamber . So silly mistake from my end.. My Bad... 🤦. Thanks for the find.

I had not even thought about the time details, since I was focusing only on dates and their difference. I will modify the code and will push it again..

plamber · 2020-12-31T09:11:06Z

+      if ((args.options.startTime && !(args.options.endTime)) || (!(args.options.startTime) && args.options.endTime)) {
+        return `startTime and endTime must both be specified (or both omitted)`;
+      }


Could we simplify this? If we omit the endtime and provide a starttime, just add 24h to the endtime.

This might be useful if we search back 5 days for example. I do not see any reason of specifying the enddate if I just want to see what happened 5 days ago

That is a brilliant suggestion. I was being a robot developer and was blindly following the specs without thinking from the user's shoes. Your suggestion is amazing and definitely increases the usability for the users sticking on to the recommended specs.

Another question If I may @plamber , how about a case where the user enters, endTime alone? just thinking out wild here. Do you think we can generate startTime automatically which is 24 hours before the entered endTime? Or do you think it is not quite needed and don't see that situation coming into picture?

Not sure about this use case. I am seeing it more with the startTime variable.

Thanks @plamber for the suggestion. Will stick on with your approach in that case. We can always update it in future if similar use case comes up.

Another question, do you think we need to update the specs since it is mentioned that Start time and end time must both be specified (or both omitted) for startTime and endTime parameters.

Sorry for bombarding with questions..

Yes, please update the specs too

Thanks @plamber for the confirmation. Guess I do not have permission to update the specs from my end.

Do you think you will be able to update it from your end? Thanks for all the guidance.

@arjunumenon: just send me the exact wording and I will update the specs

@arjunumenon: just send me the exact wording and I will update the specs

Hello @plamber - Below is command specs which I thought could be given. Please free to update the specs if needed from your end if that makes more sense. I would be totally fine with that. 🙂

Option Description

-c, --contentType Audit content type of logs to be retrieved, should be one of the following: AzureActiveDirectory, Exchange, SharePoint, General, DLP.

-s, --startTime [startTime] Start time of logs to be retrieved. Start time and end time must be less than or equal to 24 hours apart. Start time is mandatory if End time is specified.

-e, --endTime [endTime] End time of logs to be retrieved. Start time and end time must be less than or equal to 24 hours apart. If End time is not specified, command will assume the End time to be 24 hours from the specified Start time.

-o, --output [output] Output type. json,text. Default text

--verbose Runs command with verbose logging

--debug Runs command with debug logging

Updated the spects

plamber · 2020-12-31T09:17:46Z

+    telemetryProps.startTime = args.options.startTime;
+    telemetryProps.endTime = args.options.endTime;


The starttime and endtime value might not be interesting in our telemetry but if these parameters have been used. You can use this syntax to do it

telemetryProps.startTime= typeof args.options.startTime !== 'undefined'; telemetryProps.endTime= typeof args.options.endTime !== 'undefined';

On the other hand it would be interesting to see which type of audit is used. Therefore, I would add the option to collect the value for the contenttype

telemetryProps.contentType = args.options.contentType;

Secret between you and me. (That is a copy-paste mistake. Promise me you won't tell to anyone.✋)

Will definitely add the contentType filter to the telemetry since it will benefit us. Thanks for pointing that out.

plamber · 2020-12-31T09:18:54Z

+```sh
+m365 tenant auditlog report --contentType "Exchange" --startTime "2020-12-13" --endTime "2020-12-14"
+```


Would it be possible to add some examples on how to search within a day (between specific hours)?

Sure. I will add the examples to show the usage of the command for a certain time period, especially time period within a day.

plamber · 2020-12-31T09:19:20Z

+`-s, --startTime [startTime]`
+: Start time of logs to be retrieved. Start time and end time must both be specified (or both omitted) and must be less than or equal to 24 hours apart.
+
+`-e, --endTime [endTime]`
+: End time of logs to be retrieved. Start time and end time must both be specified (or both omitted) and must be less than or equal to 24 hours apart.


Please check my comments regarding the endTime omission if you specify a starttime

What I am thinking is to give our explanation accordingly. Something around these lines -

If you haven't entered the endTime, command will automatically consider the endTime as 24 hours after the startTime and will generate the report accordingly.

As per this question, can I give the explanation for the case where they have not entered startTime as well.?

arjunumenon · 2021-01-11T04:22:52Z

Thanks a lot for the comments. I will have a look into that and will start modifying it..

Totally appreciate your patience in the review since there were quite a large number of changes 🙏..

waldekmastykarz · 2021-01-11T10:34:17Z

Likewise: appreciate your effort to get it in 👏

arjunumenon · 2021-01-12T16:13:32Z

Hello @waldekmastykarz - I have updated the code which will handle the review comments. Along with the changes, I had also identified a flaw in the logic which was - endTime was auto calculated even if the user enters a valid endTime. I have given the fix for the same. Line # 151 - Line # 158.

Sorry that I had missed in the initial implementation.

waldekmastykarz · 2021-01-12T18:33:18Z

Awesome! Thank you for such a quick turnaround 👏

waldekmastykarz · 2021-01-13T18:55:26Z

+      }, err => { return Promise.reject(err) });
+  }
+
+  private getAuditLogReportforSingleContentURL(auditURL: string): Promise<AuditlogReport[]> {


This seems to be still unchanged

waldekmastykarz

Nicely done! We're almost done. Would you mind having one last look at the unresolved comments?

coveralls · 2021-01-18T07:42:45Z

Coverage remained the same at 100.0% when pulling 18c5824 on arjunumenon:1739-new-tenantauditlog into ed5be30 on pnp:master.

arjunumenon · 2021-01-19T04:56:47Z

Nicely done! We're almost done. Would you mind having one last look at the unresolved comments?

Hello @waldekmastykarz - I have made the changes based on your recommendation. Can you have a quick look into that and let me know if anything else needs to be changed from my end.

Thanks a lot for the patiently reviewing all the changes ❤. I am sorry that I had missed some of the points which you had told in the initial review process which made you spend time twice.

PS : I have not changed the status to Ready for Review yet. I have not made the commits clean yet. Once we are good with the code, I will clean the commit history and will make the status as Ready for Review. Kindly let me know if you don't agree with this approach so that I will mark is as Ready for Review right away.

waldekmastykarz · 2021-01-19T18:41:31Z

Appreciate your patience and sticking with us as well. I'll mark it as ready for review. Don't worry about the commits. As long as there are no merge conflicts, we can take care of them ❤️

waldekmastykarz

Very nicely done. I've done a few small alignment changes when merging the PR but other than that 👍

waldekmastykarz · 2021-01-20T14:39:23Z

Merged manually. Thank you! 👏

arjunumenon commented Dec 11, 2020

View reviewed changes

Comment thread src/m365/tenant/commands/auditlog/auditlog-report.ts Outdated

Comment thread src/m365/tenant/commands/auditlog/auditlog-report.ts Outdated

arjunumenon force-pushed the 1739-new-tenantauditlog branch 20 times, most recently from d8bed05 to b810ded Compare December 17, 2020 10:40

arjunumenon marked this pull request as ready for review December 17, 2020 10:43

plamber self-assigned this Dec 21, 2020

plamber added the work in progress label Dec 21, 2020

plamber suggested changes Dec 31, 2020

View reviewed changes

plamber added waiting on response and removed work in progress labels Dec 31, 2020

waldekmastykarz marked this pull request as draft January 2, 2021 18:50

waldekmastykarz marked this pull request as draft January 10, 2021 11:11

arjunumenon force-pushed the 1739-new-tenantauditlog branch 3 times, most recently from 5018b6b to e99ff55 Compare January 12, 2021 16:04

arjunumenon marked this pull request as ready for review January 12, 2021 16:13

waldekmastykarz reviewed Jan 13, 2021

View reviewed changes

waldekmastykarz requested changes Jan 13, 2021

View reviewed changes

waldekmastykarz marked this pull request as draft January 13, 2021 19:07

arjunumenon force-pushed the 1739-new-tenantauditlog branch from e99ff55 to 7f4b142 Compare January 18, 2021 07:38

arjunumenon force-pushed the 1739-new-tenantauditlog branch from 9b36ce2 to 4f5bc3d Compare January 19, 2021 03:30

arjunumenon added 5 commits January 19, 2021 09:02

Add 'tenant auditlog report' command.

200c751

AUM - Updated the else for no Audit log

55dbc6b

AUM - Resolving Promises. Completed test Cases

54b70b6

AUM - AUM - Renamed method with standard naming

5217649

AUM - Error Handling and rejection - Batched Promises

18c5824

arjunumenon force-pushed the 1739-new-tenantauditlog branch from 4f5bc3d to 18c5824 Compare January 19, 2021 03:35

waldekmastykarz marked this pull request as ready for review January 19, 2021 18:41

waldekmastykarz self-assigned this Jan 20, 2021

waldekmastykarz approved these changes Jan 20, 2021

View reviewed changes

waldekmastykarz added the pr-merged label Jan 20, 2021

waldekmastykarz closed this Jan 20, 2021

Centaurioun mentioned this pull request Feb 13, 2023

[Snyk] Upgrade ajv from 5.2.5 to 8.12.0 Centaurioun/cli-microsoft365#356

Merged

Option	Description
-c, --contentType	Audit content type of logs to be retrieved, should be one of the following: AzureActiveDirectory, Exchange, SharePoint, General, DLP.
-s, --startTime [startTime]	Start time of logs to be retrieved. Start time and end time must be less than or equal to 24 hours apart. Start time is mandatory if End time is specified.
-e, --endTime [endTime]	End time of logs to be retrieved. Start time and end time must be less than or equal to 24 hours apart. If End time is not specified, command will assume the End time to be 24 hours from the specified Start time.
-o, --output [output]	Output type. json,text. Default text
--verbose	Runs command with verbose logging
--debug	Runs command with debug logging

		telemetryProps.startTime = args.options.startTime;
		telemetryProps.endTime = args.options.endTime;

Conversation

arjunumenon commented Dec 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!