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

Audit log #1042

Merged
merged 8 commits into from May 6, 2024
Merged

Audit log #1042

merged 8 commits into from May 6, 2024

Conversation

nrozanov
Copy link
Contributor

@nrozanov nrozanov commented Apr 24, 2024

Description

Added AuditLog data model
Started adding new records to it on user creation / deletion / activating etc.
New "Audit Log" buttons on account and user page that lead to audit log page

Look & Feel

New endpoints:

  • GET /accounts/auditlog/<account_id>
  • GET /users/auditlog/<user_id>

How to test

Run flexmeasures db upgrade
Create new user / delete existing one / activate or deactivate some user
Check there are records on audit log page for this user

Further Improvements

Fix bug with user not being able to view himself unless he is an admin.

Related Items


  • I agree to contribute to the project under Apache 2 License.
  • To the best of my knowledge, the proposed patch is not based on code under GPL or other license that is incompatible with FlexMeasures

Signed-off-by: Nikolai Rozanov <nickolay.rozanov@gmail.com>
@nrozanov nrozanov requested a review from nhoening April 24, 2024 20:33
Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

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

Very elegant work, thanks!
Giving you first feedback, not on all remaining questions, but on issues I found by first review / iteration.

A thought about the table-level access rule implementation:
This approach should be described in auth.policy where we document mostly our auth approach (I give some text snippets I believe would work in the following).

We should make it semi-official that "we don't have a general solution for table-level auth (as we haven't needed a general implementation so far), but there is a nice custom approach to it."

"A class method on the model can be added which returns an AuthModelMixin. That would have an __acl__() function with your rules, which the auth policy will then go on and use. The permission_required_for_context decorator can make sure this AuthModelMixin object is used by the policy via ctx_loader. It can even pass in the context if that is helpful for your logic.

See the AuditLog model class for an example, where we required authorization logic which governs if a subset of a table (e.g. all audit logs that relate to an account) are availabe to the current user."

flexmeasures/ui/templates/crud/account_audit_log.html Outdated Show resolved Hide resolved
flexmeasures/ui/templates/crud/account_audit_log.html Outdated Show resolved Hide resolved
flexmeasures/data/models/user.py Outdated Show resolved Hide resolved
flexmeasures/data/services/users.py Show resolved Hide resolved
flexmeasures/data/services/users.py Show resolved Hide resolved
flexmeasures/data/models/user.py Outdated Show resolved Hide resolved
flexmeasures/data/models/user.py Outdated Show resolved Hide resolved
flexmeasures/api/v3_0/users.py Show resolved Hide resolved
flexmeasures/data/models/user.py Outdated Show resolved Hide resolved
flexmeasures/data/models/user.py Outdated Show resolved Hide resolved
Signed-off-by: Nikolai Rozanov <nickolay.rozanov@gmail.com>
Signed-off-by: Nikolai Rozanov <nickolay.rozanov@gmail.com>
Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

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

I like it a lot. Getting close I believe!

flexmeasures/api/v3_0/accounts.py Outdated Show resolved Hide resolved
flexmeasures/api/v3_0/users.py Show resolved Hide resolved
flexmeasures/cli/data_delete.py Outdated Show resolved Hide resolved
flexmeasures/data/models/audit_log.py Outdated Show resolved Hide resolved
flexmeasures/data/services/users.py Outdated Show resolved Hide resolved
flexmeasures/ui/templates/crud/account_audit_log.html Outdated Show resolved Hide resolved
flexmeasures/ui/templates/crud/user_audit_log.html Outdated Show resolved Hide resolved
flexmeasures/ui/templates/crud/user_audit_log.html Outdated Show resolved Hide resolved
flexmeasures/api/v3_0/users.py Outdated Show resolved Hide resolved
flexmeasures/data/services/users.py Outdated Show resolved Hide resolved
Signed-off-by: Nikolai Rozanov <nickolay.rozanov@gmail.com>
Signed-off-by: Nikolai Rozanov <nickolay.rozanov@gmail.com>
Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

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

Looks really good to me, aside from three small things:

  • one thing I find weird about naturalized datetime. Not a deal breaker
  • the missing changelog entry, please add
  • can you link the active username to a mailto link to their email?

flexmeasures/utils/time_utils.py Show resolved Hide resolved
@nhoening nhoening changed the title Audit log WIP Audit log Apr 30, 2024
Signed-off-by: Nikolai Rozanov <nickolay.rozanov@gmail.com>
Signed-off-by: Nikolai Rozanov <nickolay.rozanov@gmail.com>
Signed-off-by: Nikolai Rozanov <nickolay.rozanov@gmail.com>
@nhoening nhoening merged commit 7e4ca14 into main May 6, 2024
9 checks passed
@nhoening nhoening deleted the audit-log branch May 6, 2024 14:04
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.

None yet

2 participants