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.

Sign up for GitHub

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

Jump to bottom

feat: include file id in audit logs #44871

Merged
merged 1 commit into from
Apr 30, 2024
Merged

feat: include file id in audit logs #44871

merged 1 commit into from
Apr 30, 2024

Conversation

yemkareems
Copy link
Contributor

@yemkareems yemkareems commented Apr 17, 2024 •
edited by come-nc
Loading

Summary

  • Few logs are changed after this implementation. Find attached is the file of logs before and after this implementation
    nc.logs.txt

Checklist

@yemkareems yemkareems added enhancement 3. to review Waiting for reviews labels Apr 17, 2024
@yemkareems yemkareems added this to the Nextcloud 30 milestone Apr 17, 2024
@yemkareems yemkareems self-assigned this Apr 17, 2024
artonge
artonge reviewed Apr 17, 2024
View reviewed changes
Copy link
Contributor

@artonge artonge left a comment

Choose a reason for hiding this comment

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

Looks good overall

apps/admin_audit/lib/Actions/Files.php Outdated Show resolved Hide resolved
apps/admin_audit/lib/Actions/Files.php Outdated Show resolved Hide resolved
apps/admin_audit/lib/AppInfo/Application.php Outdated Show resolved Hide resolved
@szaimen
Copy link
Contributor

szaimen commented Apr 18, 2024

Sorry, don't feel confident enough to review this

@szaimen szaimen removed their request for review April 18, 2024 08:27
come-nc
come-nc reviewed Apr 22, 2024
View reviewed changes
apps/admin_audit/lib/AppInfo/Application.php Outdated Show resolved Hide resolved
apps/admin_audit/lib/Actions/Files.php Outdated
Comment on lines 197 to 220
} catch (InvalidPathException|NotFoundException $e) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should still log something, maybe even on error level, so that this is not missed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@come-nc logging the exception message as error in log file. I tried to delete a file that is already deleted from oc_filecache and also removed from data/admin/files/log.txt. I get a message in the UI saying "Delete file" action failed but i did not find it getting logged as a error message

@come-nc
Copy link
Contributor

come-nc commented Apr 22, 2024

Please also fix commit names, should not all start with feat:, see https://www.conventionalcommits.org/en/v1.0.0/
Would be good to use the opportunity to also squash them into something like 2 commits only, one for event refactor and one for adding id to logs.

@come-nc come-nc changed the title Feature/include file id in audit logs feat: include file id in audit logs Apr 22, 2024
come-nc
come-nc reviewed Apr 29, 2024
View reviewed changes
apps/admin_audit/lib/Actions/Action.php Outdated
Comment on lines 90 to 97

/**
* Log a error message with a log level of error
* @param string $text
*/
public function error(string $text): void {
$this->logger->error($text, ['app' => 'admin_audit']);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this method, it makes thing unnecessiraly convoluted.
Inject the loggen in the child class if possible, use \OCP\Server::get if not.

@yemkareems yemkareems force-pushed the feature/include-file-id-in-audit-logs branch 3 times, most recently from 6ed0835 to 977bf8e Compare April 29, 2024 09:53
@yemkareems yemkareems force-pushed the feature/include-file-id-in-audit-logs branch 2 times, most recently from bc3e830 to 710f11d Compare April 29, 2024 10:10
come-nc
come-nc reviewed Apr 29, 2024
View reviewed changes
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

Please fix the commit description:
Copie d'écran_20240429_132314

@yemkareems yemkareems force-pushed the feature/include-file-id-in-audit-logs branch from 710f11d to 7b0b54f Compare April 29, 2024 12:04
@yemkareems @come-nc
feat: include file id in audit logs
4d22880 
Signed-off-by: yemkareems <yemkareems@gmail.com>
@come-nc come-nc force-pushed the feature/include-file-id-in-audit-logs branch from 7b0b54f to 4d22880 Compare April 30, 2024 09:31
@yemkareems yemkareems merged commit 66aad43 into master Apr 30, 2024
157 checks passed
@yemkareems yemkareems deleted the feature/include-file-id-in-audit-logs branch April 30, 2024 11:59
@blizzz blizzz mentioned this pull request Jul 24, 2024
Merged
@nickvergessen nickvergessen mentioned this pull request Aug 15, 2024
Merged
4 tasks
@juliusknorr juliusknorr mentioned this pull request Aug 28, 2024
Merged
@come-nc come-nc mentioned this pull request Oct 3, 2024
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@come-nc come-nc come-nc approved these changes

@artonge artonge artonge approved these changes

@ArtificialOwl ArtificialOwl Awaiting requested review from ArtificialOwl

Assignees

@yemkareems yemkareems

Labels
3. to review Waiting for reviews enhancement feature: admin audit
Projects
None yet
Milestone
Nextcloud 30
Development

Successfully merging this pull request may close these issues.
4 participants
@yemkareems @szaimen @come-nc @artonge