Skip to content
This repository has been archived by the owner on Jun 30, 2021. It is now read-only.

Complete Activity Log System #560

Merged
merged 28 commits into from Dec 14, 2018
Merged

Complete Activity Log System #560

merged 28 commits into from Dec 14, 2018

Conversation

T-Dnzt
Copy link

@T-Dnzt T-Dnzt commented Nov 27, 2018

Issue/Task Number: 463
closes #463

Overview

This PR completes the Activity Log system (formerly known as Audit system) added for users to the whole system to keep track of all data changes (insert, update, delete). The logic has now been split into its own sub application, the ActivityLogger. Due to the need of its usage in both the EWalletDB and the EWalletConfig (to keep track of changes to the settings), the move to a new sub application was required.

To reduce the amount of duplicated code between sub applications, a new sub app, named Utils, has also been introduced and can be used by any other sub app. It contains modules that don't deal with business logic and can be reused throughout the application.

This is a big PR. Changes had to be made to every single sub application due to the nature of the feature. Luckily, the commits have been split in a way that reviewing the changes shouldn't be too annoying, since each sub app gets its own commit (yeah, I rewrote the history and went down from 25+ meaningless commits with stuff moving around all the time to those 10 digestible (yet big) commits.

Changes & Implementation Details

  • The main change is the introduction of the ActivityLog system, which tracks changes to records and who is responsible for them. Currently, this system does not involve reversing to previous states but it could be implemented easily on top of the existing feature.

  • Changes have been made to tests throughout the application to add the new mandatory parameter, originator. Any record supported by the system (and defined in the configuration) can be passed to the originator field. %ActivityLogger.System{} and %EWalletDB.Seeder{} can also be used in tests and seeds, respectively.

  • I originally wanted to keep the ActivityLog sub app completely separated with its own GenServer receiving log requests and storing them asynchronously, without increasing the duration of the requests. Sadly, due to the complexity of having asynchronous features spanning multiple sub-applications, I had to give up for now. Doing it for the EWalletConfig sub-app was a requirement due to the state that needs to be kept (the list of sub-apps that need their configs to be refreshed), but here we don't have any state. Therefore, simply calling the insert synchronously is "good enough" (yet unsatisfying). I'd like us to revisit this in the future.

Note that adding one GenServer running in the ActivityLogger sub-application is easy - the problems rise when running the tests. Due to some weird ExUnit stuff, it doesn't seem possible to reuse the GenServer started as part of another sub-application, so it becomes necessary to boot a new GenServer for each test. That's what we did for the configuration system and it was alright, because it could be done in the setup step by initiating a new GenServer, getting its PID, initiating the configuration with that specific PID and doing any update by passing that PID every time.

Doing the same thing for the Activity Log system would require being able to pass that PID for every single call: controllers, gates, etc. We would have to send the PID of the Activity Log server to use to the controllers which would send it all the way down to the schemas which finally call the ActivityLogger functions through their Repos.

If anyone has any suggestions, I'm all ear.

Usage

The system is meant to be as easy to use as possible. Here are the steps to add it to a new schemas.

  1. Make sure the schema has a log type, defined in the application.ex of the sub-app:
    ActivityLogger.configure(%{
      EWalletDB.Seeder => "seeder",
      EWalletDB.User => "user",
      EWalletDB.Invite => "invite",
      EWalletDB.Key => "key",
      EWalletDB.ForgetPasswordRequest => "forget_password_request",
      EWalletDB.UpdateEmailRequest => "update_email_request",
      EWalletDB.AccountUser => "account_user",
      EWalletDB.Transaction => "transaction",
      EWalletDB.Mint => "mint",
      EWalletDB.TransactionRequest => "transaction_request",
      EWalletDB.TransactionConsumption => "transaction_consumption",
      EWalletDB.Account => "account",
      EWalletDB.Category => "category",
      EWalletDB.ExchangePair => "exchange_pair",
      EWalletDB.Wallet => "wallet",
      EWalletDB.Membership => "membership",
      EWalletDB.AuthToken => "auth_token",
      EWalletDB.APIKey => "api_key",
      EWalletDB.Token => "token",
      EWalletDB.Role => "role"
    })
  1. Use the ActivityLogger.ActivityRepo in your Repo
defmodule EWalletDB.Repo do
  use Ecto.Repo, otp_app: :ewallet_db
  use ActivityLogger.ActivityRepo, repo: EWalletDB.Repo
  
  # ...
end
  1. Use ActivityLogger.ActivityLogging in your schema
defmodule MySchema do
  use ActivityLogger.ActivityLogging
  # ...
end
  1. Add activity_logging() in the schema definition
  schema "account" do
    external_id(prefix: "acc_")
 
    # ...

    timestamps()
    activity_logging()
  end
  1. Use cast_and_validate_required_for_activity_log in your changesets
  defp changeset(%MySchema{} = schema, attrs) do
    account
    |> cast_and_validate_required_for_activity_log(
      attrs, # attributes 
      [:name, :description], # fields to cast
      [:name] # fields that are required, optional
      [:encrypted_stuff] # fields that are encrypted, optional
    )

This function will check for the originator.

  1. Use the new Repo functions

insert: Repo.insert_record_with_activity_log(changeset)
update: Repo.update_record_with_activity_log(changeset)
delete: Repo.delete_record_with_activity_log(changeset)

Finished reading the wall of text above

  • O
  • Sirn
  • Med
  • Jar

Review

  1. Utils sub app
  • O
  • Sirn
  • Med
  • Jar
  1. ActivityLogger sub app
  • O
  • Sirn
  • Med
  • Jar
  1. ActivityLogger in EWalletDB
  • O
  • Sirn
  • Med
  • Jar
  1. Remove uneeded originator from EWalletDB
  • O
  • Sirn
  • Med
  • Jar
  1. ActivityLogger in EWallet
  • O
  • Sirn
  • Med
  • Jar
  1. ActivityLogger in EWallet API
  • O
  • Sirn
  • Med
  • Jar
  1. ActivityLogger in Admin API
  • O
  • Sirn
  • Med
  • Jar
  1. ActivityLogger in EWalletConfig
  • O
  • Sirn
  • Med
  • Jar
  1. ActivityLogger in EWalletDB Seeds
  • O
  • Sirn
  • Med
  • Jar

To Do

  • Handle encrypted fields properly in every schema

An endpoint to retrieve those logs will be added in another PR.

@ghost ghost assigned T-Dnzt Nov 27, 2018
@ghost ghost added the s2/wip 🚧 label Nov 27, 2018
@T-Dnzt T-Dnzt changed the title 463 complete audit system Complete Audit/Logging System Nov 27, 2018
apps/ewallet/lib/ewallet/gates/transaction_request_gate.ex Outdated Show resolved Hide resolved
apps/ewallet/lib/ewallet/web/originator.ex Outdated Show resolved Hide resolved
apps/ewallet_db/lib/ewallet_db/account_user.ex Outdated Show resolved Hide resolved
apps/ewallet_db/lib/ewallet_db/audit.ex Outdated Show resolved Hide resolved
apps/ewallet_db/lib/ewallet_db/soft_delete.ex Show resolved Hide resolved
apps/ewallet_db/lib/ewallet_db/transaction_request.ex Outdated Show resolved Hide resolved
apps/ewallet_db/lib/ewallet_db/transaction_request.ex Outdated Show resolved Hide resolved
apps/ewallet_db/lib/ewallet_db/user.ex Outdated Show resolved Hide resolved
apps/ewallet_db/lib/ewallet_db/user.ex Outdated Show resolved Hide resolved
@T-Dnzt T-Dnzt changed the title Complete Audit/Logging System Complete Activity Log System Dec 3, 2018
@T-Dnzt T-Dnzt added this to the v1.1 milestone Dec 3, 2018

config :activity_logger, ActivityLogger.Repo,
adapter: Ecto.Adapters.Postgres,
url: {:system, "DATABASE_URL", "postgres://localhost/ewallet_dev"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this use different database from ewallet? 🤔

{:ok, config}
end

defp secret_key(:prod), do: decode_env("EWALLET_SECRET_KEY")
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this use eWallet's secret key? 🤔

@T-Dnzt T-Dnzt mentioned this pull request Dec 6, 2018
@mederic-p
Copy link
Contributor

Added log test in controller, please check this commit
Fixed comments in PR, please check this commit

@@ -0,0 +1,41 @@
defmodule ActivityLogger.Repo.Migrations.RenameIdPrefixFromAdtToLogInActivityLog do
Copy link
Contributor

Choose a reason for hiding this comment

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

Was thinking if you'd do this migration. 👍

assert log.target_encrypted_changes == %{
"encrypted_metadata" => %{"something" => "secret"}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

:mindblown:

)
end

test "generates an activity log when minting" do
Copy link
Contributor

Choose a reason for hiding this comment

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

:mindblown:

@unnawut
Copy link
Contributor

unnawut commented Dec 14, 2018

Can we add some tests to ActivityLogger.ActivityLogTest?

  • ActivityLog.insert/3 (it's being tested indirectly through TestDocument.insert/1 and TestUser.insert/1 right now but I think it's nice to have direct tests on the function)
  • Add a test that ActivityLog.insert/3 removes changes[:prevent_saving] from fields and encrypted fields.

@T-Dnzt T-Dnzt merged commit 8927b23 into master Dec 14, 2018
@ghost ghost removed the s3/review 👀 label Dec 14, 2018
@T-Dnzt T-Dnzt deleted the 463-complete-audit-system branch December 14, 2018 11:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Complete the Audit system
5 participants