Identityserver4.admin: Add audit tables to track IdSrv configuration data changes

[cerginio]

During administration clients, scopes, users, roles may be important to have control on it's history.

One of the cheap way to get changes history could be SQL server Track Data Changes feature.

1. Add feature for SQL server
   https://docs.microsoft.com/en-us/sql/relational-databases/track-changes/enable-and-disable-change-data-capture-sql-server?view=sql-server-2017
2. Add views to read tracking changes tables and build around them AuditDbContext.

Second options is to realize it on application level by using EF handlers and changetracker.
https://blog.tonysneed.com/2017/10/01/trackable-entities-for-ef-core/

[ruant]

I would like to see it not bound to MSSQL, that way people can choose what SQL technology they want to run themselves.

[skoruba]

I agree with @cerginio - audit log is very important. I'll check some details about the implementation that mentioned @ruant - it will be nice to use the implementation independent of the database engine.

[skoruba]

@cerginio - Any suggestions about that?

[cerginio]

I would chose application level logging based on EF handlers. Defenetely we must store in each row for each history table username from bearer token. I would use change tracker state and save row before change into history table. That tables should be defined in separate audit db context.
But how to achieve that with clean code is a big question, taking into account that we reuse ids4 entity model and do not have control on it.

[xmichaelx]

I would suggest throwing event for every action we want to be part of the audit log and allow for registering listeners in the DI for those events.

public async Task<int> RemoveClientAsync(ClientDto client)
{
            var clientEntity = client.ToEntity();
            var result = await _clientRepository.RemoveClientAsync(clientEntity);
            this.auditLogger.Log(client, result);  // audit logger injected
            return result ;
}

Audit log sounds like a long list of events which we would ideally like to store in database designed for doing so like EventStore. In simplest scenario we could simply dump all events there without providing UI in AdminUI for reading them. Then for auditing we could use built-in EventStore.UI to query events either live or from past.

[Robban1980]

I think an important part here is not also not bind this functionality to a specific storage type, like EventStore.
Raise an audit event and if subscribed to, the implementer can decide what they want to do with it. Be it storing it in MSSQL, PostgreSQL, EventStore etc.

[RobCodemonkey]

One of the compliance requirements for this kind of implementation would be immutability - I presume that would be handled on the implementer end rather than by the functionality handling audit events.

This might require guidelines for setup on the user side.

[skoruba]

Currently I am working on this stuff here:
https://github.com/skoruba/IdentityServer4.Admin/tree/feature/audit-logging
I have used the services for logging based on IdentityServer4.

[pbros]

Will this audit logging extend to logging user logins?

It would be useful to have an audit log of successful (and failed) login history for each user. If presented to the user, he could notice suspicious activity

[cerginio]

I played a bit with simple audit implementation in db agnostic way:
https://dejanstojanovic.net/aspnet/2018/november/tracking-data-changes-with-entity-framework-core/
https://www.meziantou.net/2017/08/14/entity-framework-core-history-audit-table

And it descover overhead in DAL implementation.
RemoveClientRelationsAsync(Client client) in
https://github.com/skoruba/IdentityServer4.Admin/blob/8ea5449965c8254d28c36c82ab45a6a77eafa663/src/Skoruba.IdentityServer4.Admin.BusinessLogic/Repositories/ClientRepository.cs

If you update single propery in Client - let's say Description from "A" to "B",
in audit logs you will see like all 9 navigation properties was removed and inserted!
and all client properties was modified. I tried to manually calculate diff and separate from the noise, but it killing my brain. it not worth of it.

@skoruba please consider how to implement an update in a more smart and lean way.
Above audit obfuscation such approach to update the whole client tree for single change kills performance.

I will try to suggest PR with audit this week.

[cerginio]

298

PR suggested

1. Please take changes
2. add migration for Audits table
3. update db
4. play with UI
5. choose random client
6. update description from "A" to "B"
7. SELECT * FROM [dbo].[ClientConfigAudit]

ER: 13 changes in audit log
1 modify
6 delete
6 inserts

[cerginio]

expected:
1 modify

[skoruba]

Done in #391

[skoruba]

I am closing this issue, it was released in master branch.