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

Track additional events in the Audit Log #2797

Open
dabeeeenster opened this issue Sep 21, 2023 · 4 comments
Open

Track additional events in the Audit Log #2797

dabeeeenster opened this issue Sep 21, 2023 · 4 comments
Labels
api Issue related to the REST API improvement Improvement to the existing platform tech-debt Technical debt issues
Milestone

Comments

@dabeeeenster
Copy link
Contributor

A user requests, for all events, track the date, time, logged in user (if possible) and the source user’s IP address of the event:

  • a user logged into the account
  • a user logged out of the account
  • a user changes their password
  • a user enabled or disabled 2FA authentication
  • a user tried to log in to the account, but the password was entered incorrectly
  • a user tried to log in to the account, but the 2FA code was entered incorrectly
  • An Administrator created a user new account (account name, email for which the account was created)
  • An Administrator deleted the existing account (account name, email for which the account was created)
  • An Administrator created a new role
  • An Administrator deleted an existing role
  • When access rights were assigned a certain role
  • When a user was assigned to a role
  • When a user was removed from a role
  • When a project was created
  • When a project was deleted
@dabeeeenster dabeeeenster added api Issue related to the REST API tech-debt Technical debt issues improvement Improvement to the existing platform labels Sep 21, 2023
@matthewelwell
Copy link
Contributor

The biggest concern here is that currently, the AuditLog entities are associated with a project and, in most cases, a specific environment (see here). We will need to update the AuditLog model to be associated with only an organisation. Given the size of the AuditLog table, we would need to consider any impact the migration may have (although, since we'd be adding it as nullable, I think it would be fine).

Subsequently, we will need to change to the underlying functionality used when creating an AuditLog record. The method here relies on the entity being associated with an environment or a project. We will need to update this to handle entities that are only associated with an organisation.

We can achieve some of this by ensuring that the UserOrganisation entity extends this model, but we will also need to resolve the complexity of creating audit records in all organisations when a user updates something on their account (e.g. password, 2FA). This could probably be added directly on to the user model I think but slightly breaks the current implementation.

There would also likely be some FE changes required to display these in the UI appropriately and we may also need to adjust the end point used (we recently moved from a generic /api/v1/audit to /api/v1/projects/:id/audit).

@matthewelwell
Copy link
Contributor

matthewelwell commented Sep 27, 2023

From what I can tell, the high level approach here should be:

  1. Add the organisation attribute to the AuditLog record. Perhaps (to avoid having to update all the existing records) we could add it as _organisation = models.ForeignKey(Organisation, null=True) and then add an organisation property to the class which gets it from project.organisation if _organisation is null. This might add some complexity when querying in the ORM but could simplify the migration.
  2. Update the UserOrganisation, UserPermissionGroup (may also need to look at how permissions are assigned here too and add the base class to any through models), Project and UserPermissionGroupMembership classes to inherit from a model generated by core.models.abstract_base_auditable_model_factory and add the relevant methods on the model to generate the AuditLog text. (NB: we'll also need to look at the new classes created as part of @gagantrivedi's work on roles, this logic all lives in the flagsmith-rbac repository)

These 2 steps should get us about 70% of the above logic. The tricky ones to deal with are:

a user logged into the account
a user logged out of the account
a user tried to log in to the account, but the password was entered incorrectly
a user tried to log in to the account, but the 2FA code was entered incorrectly

These actions do not result in a model creation so will need to be handled separately. You can see examples of this already happening by looking at the task functions here.

We will also need to adjust the logic such that we can create multiple audit log records from a single model change. For example, when a user changes their password, this should create an AuditLog record in all organisations that the user belongs to.

@dabeeeenster dabeeeenster added this to the Isotoma milestone Oct 3, 2023
@riceyrice
Copy link

Following some discussion, we agreed the following clarifications (correct me if I'm wrong):

  • Log Create/Update/Delete of user, tracking changes to name/email/password fields
  • Log successful and unsuccessful auth by all mechanisms i.e. password, password+2FA, Google, GitHub, SAML
  • Log any change in assignment between user and permission, whether user-permission, user-group, group-permission, user-role, group-role, role-permission, also log Create, Update, Delete of group and role
  • Categorise logs by new/existing RelatedObjectType values as appropriate - mapping which of these values is "security-related" is out of scope for now

@riceyrice
Copy link

Some decisions from the weekly call 19 Oct 2023:

  • OK with log message being a set of action/field/identifier triples e.g. "updated password: stephen@example.com; updated [insert other field here]: stephen@example.com"
  • Should the use of the soft delete mixin break unique or unique_together constraints (e.g. can't create project with same name as deleted project) try to use conditional constraints to achieve this but check that supported databases can handle this (Postgres, Oracle, MySQL)
  • Hard delete may be required for compliance e.g. to delete or anonymise user
  • Consider adding a data migration to call command to create initial history models so that update logging has previous history to compare with - also make sure update logging does not fail if there is no previous history
  • Given that the current audit log mechanism is based on history models which reflect m2m fields and through models, it is OK to log that e.g. a group changed its users as a log message connected to the group model (which has an m2m field to user), rather than as log message(s) connected to the user models that were added/removed (the reverse relation)
  • It is OK to add a transaction.on_commit guard to ensure that the audit task can "see" the history model that triggered it

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 improvement Improvement to the existing platform tech-debt Technical debt issues
Projects
None yet
Development

No branches or pull requests

3 participants