Skip to content
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

feat: add site audit logs generator #1181

Merged
merged 11 commits into from
Mar 6, 2024

Conversation

dcshzj
Copy link
Contributor

@dcshzj dcshzj commented Mar 4, 2024

Problem

Site audit logs are manually created using some script that we have, which brings a lot of overhead and is not an ideal long-term solution.

Solution

Breaking Changes

  • Yes - this PR contains breaking changes
  • No - this PR is backwards compatible with ALL of the following feature flags in this doc

Features:

  • Allow users to self-service request for their site audit logs.

Tests

  • Fill in the form, ensuring that you are either an Isomer Admin or a collaborator of the repo(s) that you are requesting site audit logs for.
  • Verify that you receive an email with the site audit logs for the repos that you have requested for.
  • Try again with an account that is not an Isomer Admin account and the account does not have collaborator access to the repo(s) that you are requesting site audit logs for.
  • Verify that you still receive an email, but with an error message saying that you are not a collaborator of the repo(s).

Deploy Notes

New environment variables:

  • SITE_AUDIT_LOGS_FORM_KEY : Secret key for the site audit logs form
    • added env var to 1PW + SSM script (fetch_ssm_parameters.sh)

Additional steps

  • Create /efs/audit-logs folder on EFS.

Copy link
Contributor Author

dcshzj commented Mar 4, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @dcshzj and the rest of your teammates on Graphite Graphite

@dcshzj dcshzj marked this pull request as ready for review March 4, 2024 03:22
@dcshzj dcshzj requested a review from a team March 4, 2024 03:22
src/routes/formsg/formsgSiteAuditLogs.ts Outdated Show resolved Hide resolved
src/routes/formsg/formsgSiteAuditLogs.ts Show resolved Hide resolved
src/routes/formsg/formsgSiteAuditLogs.ts Show resolved Hide resolved
src/routes/formsg/formsgSiteAuditLogs.ts Show resolved Hide resolved
src/services/admin/AuditLogsService.ts Outdated Show resolved Hide resolved
ResultAsync.combine(
repoNames.map((repoName) => {
const userSessionData = new UserWithSiteSessionData({
githubId: user.githubId,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if this doesnt exist? ie an admin that uses this form that only has email sites?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be set to undefined, but that is not used anyway since we rely on the accessToken from TokenService.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is confusing to me :(
if it is not needed here, why declare it? doesnt seem like a necessary key too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, it is defined and is needed because of this control flow:

if (this.isGithubProps(props)) {
this.githubId = props.githubId
this.accessToken = props.accessToken
}

Otherwise, it would not be possible for us to set the accessToken for the request.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait this is an issue right? not all users have a github id, wouldnt this then fail for those users?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adjusted in ae8410a.

src/services/admin/AuditLogsService.ts Outdated Show resolved Hide resolved
src/services/admin/AuditLogsService.ts Show resolved Hide resolved
src/types/auditLog.ts Outdated Show resolved Hide resolved
src/services/admin/AuditLogsService.ts Show resolved Hide resolved
@dcshzj dcshzj force-pushed the 03-04-feat_add_site_audit_logs_generator branch from 7c7ea3f to e2e5945 Compare March 5, 2024 02:14
@dcshzj dcshzj changed the base branch from 03-04-feat_mail_support_sending_emails_with_attachments to develop March 5, 2024 02:15
@dcshzj dcshzj requested review from kishore03109 and a team March 5, 2024 02:55
Copy link
Contributor

@kishore03109 kishore03109 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only real blocking is the error messages returned to agency users

@dcshzj dcshzj requested review from kishore03109 and a team March 6, 2024 02:08
Copy link
Contributor

@kishore03109 kishore03109 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only blocking is in verifying that non gh users admin users should be able to use this form to get their site's audit logs

@dcshzj dcshzj requested review from kishore03109 and a team March 6, 2024 08:06
@dcshzj dcshzj merged commit d706677 into develop Mar 6, 2024
13 checks passed
@mergify mergify bot deleted the 03-04-feat_add_site_audit_logs_generator branch March 6, 2024 10:23
@alexanderleegs alexanderleegs mentioned this pull request Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants