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: Write critical operations to the audit log #959

Merged
merged 4 commits into from
Apr 16, 2024

Conversation

hweihwang
Copy link
Contributor

@hweihwang hweihwang commented Apr 1, 2024

This PR introduces audit logging for critical operations in the Tables app, addressing issue #956. The goal is to enhance compliance measures by logging critical operations into a dedicated log file.

Changes

  • Introduced AuditLogServiceInterface, which outlines the contract for audit logging within the application. The DefaultAuditLogService is the default implementation that dispatches CriticalActionPerformedEvent to interact with the AuditLog App. This service is auto-resolved by registering it in the Dependency Injection Container Application.php. This design allows for future extensibility, such as integrating different logging methods or services for AuditLog.

  • Developed event dispatchers for corresponding events to trigger audit logs (TableDeletedEvent => WhenRowDeletedAuditLogListener, ViewDeletedEvent => WhenViewDeletedAuditLogListener, etc.). This is a standard event-driven approach that decouples auxiliary logic (logging, mail sending, etc.) from the core business logic, effectively handling side effects. Most of these operations can be executed asynchronously via a queue. Each Listener is encapsulated in its own class adhering to the Single Responsibility Principle. The AuditLogServiceInterface is injected into each listener to further promote decoupling.

Testing

Unit tests have been added for the new service and listeners. These tests can be run by executing into the server container and running composer test.

Potential Improvements

  • Implementing a separate background queue worker for all listeners could offload non-business logic from the main service logic, improving performance, especially when dealing with external API calls or IO intensive operations.

  • Applying Database Transactions: The TableService interacts with multiple tables simultaneously (Table, View, Row, Column). Implementing DB transactions and error handling mechanisms can ensure the integrity and reliability of operations.

Please review and provide your feedback, thank you @juliushaertl!

@AndyScherzinger AndyScherzinger added the 2. developing Work in progress label Apr 1, 2024
Signed-off-by: Hoang Pham <hoangmaths96@gmail.com>
Signed-off-by: Hoang Pham <hoangmaths96@gmail.com>
Signed-off-by: Hoang Pham <hoangmaths96@gmail.com>
Copy link
Member

@juliushaertl juliushaertl left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this and sorry for taking so long to check. This looks very good to me already. The conceptual changes make a lot of sense to have events introduced for those actions and while we might not have a immediate use case for the service contract, this could become quite useful in the future.

I left a few minor comments, maybe you can check those. Looking forward to your updates/comments on that.

/**
* @param (int|string)[][] $sortArray
*
* @psalm-param list<array{columnId: int, mode: 'ASC'|'DESC'}> $sortArray
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem related to the implementation but fine to annotate here, just the static analysis fails due to the incomplete array description as the array gets enriched with a columnType key

Error: lib/Db/LegacyRowMapper.php:184:25: InvalidArrayOffset: Cannot access value on variable $sortRule using offset value of 'columnType', expecting 'columnId' or 'mode' (see https://psalm.dev/115)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@juliushaertl Thank you for spot on! Wondering why the ci/cd checks keeps failing (Not immediately but after few days)


Changed to this: @psalm-param list<array{columnId?: int, columnType?: string, mode?: 'ASC'|'DESC'}> $sortArray

Because when running psalm at my local there a case only 2 of them present (columnId & mode)

Copy link
Member

@juliushaertl juliushaertl Apr 11, 2024

Choose a reason for hiding this comment

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

The CI requires approval for outside contributors, i just approved it again, so results should appear more quickly now.

I'm also unsure why that may not happen for you locally, just checked and I get the same error. The only thing I could imagine is psalm cache which sometimes messes things up. You could try running with composer run psalm -- --no-cache --threads=8 and see if that makes a difference.

lib/Event/ViewDeletedEvent.php Outdated Show resolved Hide resolved
lib/Listener/WhenViewDeletedAuditLogListener.php Outdated Show resolved Hide resolved
Signed-off-by: Hoang Pham <hoangmaths96@gmail.com>
Copy link
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

@juliushaertl juliushaertl merged commit 6c85f7e into nextcloud:main Apr 16, 2024
57 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants