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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: audit additional events #2918

Open
wants to merge 87 commits into
base: main
Choose a base branch
from

Conversation

riceyrice
Copy link

@riceyrice riceyrice commented Nov 2, 2023

Thanks for submitting a PR! Please check the boxes below:

  • I have run pre-commit to check linting
  • I have filled in the "Changes" section below?
  • I have filled in the "How did you test this code" section below?
  • I have used a Conventional Commit title for this Pull Request

Changes

Implements #2797

  • refactors AuditLog to associate with organisation without needing project/environment
  • audits security-related project, user, group, role, permission changes per organisation
  • upgrades and patches django-simple-history to correctly handle indirect m2m signals
  • audits user login success/failure and logout signals
  • refactors auth mechanisms to send login success/failure and logout signals

Note: python manage.py populate_history --auto must be run after migrations in order to create initial history instances, otherwise the first update to any model will log a warning but create no audit log records.

Note: There are corresponding changes in flagsmith-saml to ensure django signals are sent which must be deployed to correctly audit saml login success/failure.

Note: Does not include front-end changes to view per-organisation audit logs. AuditLog views look for organisation directly/indirectly and should be simplified once organisation field is backfilled for existing records.

How did you test this code?

Tested manually by making changes to memberships and permissions in the front end app or django admin or django shell. Audit log may be inspected per-project or per-environment in the front end but otherwise must be checked using REST API for per-organisation feed or django shell.
Unit tests will be added once functionality approved. Existing unit tests may fail as I'm currently unable to run them 馃槥
Login success/failure tested manually in front end using samltest.id and developer GitHib/Google apps.
Unit tests now complete.

Copy link

vercel bot commented Nov 2, 2023

The latest updates on your projects. Learn more about Vercel for Git 鈫楋笌

Name Status Preview Comments Updated (UTC)
docs 鉁 Ready (Inspect) Visit Preview 馃挰 Add feedback Apr 15, 2024 2:22pm
flagsmith-frontend-preview 鉁 Ready (Inspect) Visit Preview 馃挰 Add feedback Apr 15, 2024 2:22pm
flagsmith-frontend-staging 鉁 Ready (Inspect) Visit Preview 馃挰 Add feedback Apr 15, 2024 2:22pm

Copy link
Contributor

github-actions bot commented Nov 2, 2023

Uffizzi Ephemeral Environment deployment-43169

鈽侊笍 https://app.uffizzi.com/github.com/Flagsmith/flagsmith/pull/2918

馃搫 View Application Logs etc.

What is Uffizzi? Learn more!

Copy link
Contributor

@matthewelwell matthewelwell left a comment

Choose a reason for hiding this comment

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

@riceyrice on the whole, this looks great! I've added some (mostly) minor comments, but it seems to achieve what we're looking for and the code looks good.

api/audit/models.py Show resolved Hide resolved
api/audit/signals.py Show resolved Hide resolved
Comment on lines 53 to 54
environment=feature_state._get_environment(),
project=feature_state._get_project(),
Copy link
Contributor

Choose a reason for hiding this comment

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

These methods should be 'public' if we're calling them here. Is there a reason they're 'private'?

Copy link
Author

Choose a reason for hiding this comment

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

They are only private because they were already private - I guess they were intended for use by the public get_environment_and_project (now get_organisations_project_environment).
Previously this code accessed member variables directly, which seemed to be against DRY and introduce the possibility that it could pick different values to those used for audit logs that go via _get_environment.
I guess there are some options here:

  • use get_organisations_project_environment public interface (like create_audit_log_from_historical_record in the same file does)
  • make _get_organisations/project/environment into public interface

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the downside of option 1 here is that we don't need the organisations (I don't think?). My preference in that case would be option 2 but if, for whatever reason, we do want the organisation here too, let's go with option 1.

Copy link
Author

Choose a reason for hiding this comment

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

We don't need organisation so went with option 2

api/audit/tasks.py Show resolved Hide resolved
api/audit/tasks.py Outdated Show resolved Hide resolved
Comment on lines 437 to 443
Base.related_object_type = related_object_type
if audit_create:
Base.get_create_log_message = default_get_create_log_message
if audit_update:
Base.get_update_log_message = default_get_update_log_message
if audit_delete:
Base.get_delete_log_message = default_get_delete_log_message
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the benefit of doing it this way versus defining the defaults on the base class itself?

Copy link
Author

Choose a reason for hiding this comment

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

This dance was to avoid changing the default behaviour of abstract_base_auditable_model_factory as many things already call this expecting _AbstractBaseAuditableModel and its implementations that return None, logging nothing by default.
All the things I've added want to log create/update/delete by default (except project but maybe it should).
I will see if I can do it more elegantly using mixins.

Copy link
Author

@riceyrice riceyrice Nov 7, 2023

Choose a reason for hiding this comment

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

I've replaced this with a mixin implementation - it's a bit verbose but hopefully clearer. tbh if create/update/delete logging could be on/off rather than three toggles, that could be a fair bit simpler. Project audit being create/delete only is the only real reason for it and maybe that's not a real requirement.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I don't think that's an intentional requirement. I think having project update would also be beneficial.

api/core/models.py Outdated Show resolved Hide resolved
api/projects/models.py Outdated Show resolved Hide resolved
api/projects/models.py Outdated Show resolved Hide resolved
api/users/models.py Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Issue related to the REST API front-end Issue related to the React Front End Dashboard
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants