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

Create header files for discussion of possible flow audit api. #620

Merged
merged 1 commit into from May 10, 2017

Conversation

Projects
None yet
5 participants
@mnesbit
Copy link
Contributor

mnesbit commented May 3, 2017

This PR is to allow discussion of the shape of the flow audit api. It is not intended as a full implementation.

@mnesbit mnesbit requested review from mikehearn and josecoll May 3, 2017

@chrisr3 chrisr3 added this to the M12 milestone May 3, 2017

@mnesbit mnesbit force-pushed the mnesbit-audit-interface branch from 7b40501 to 9a0335c May 4, 2017

* The extra data will be recorded to the audit logs when the flow progresses.
* Even if empty the basic details (i.e. label) of the step will be recorded for audit purposes.
*/
open val extraAuditData: Map<String, String> get() = emptyMap()

This comment has been minimized.

@rick-r3

rick-r3 May 4, 2017

Contributor

We currently define steps as objects and this would bind in the audit data with what is effectively a constant (the object). I'm assuming a flow author might want to add in e.g. a trade identifier to the audit data. They wouldn't be able to do that with this API, at least the way we currently use it. I was thinking this would be on the ProgressTracker.nextStep call or ProgressTracker.currentStep as an optional, dynamic parameter.

This comment has been minimized.

@mnesbit

mnesbit May 4, 2017

Author Contributor

I did investigate making the extraAuditData part of the api, but it got ugly really fast. In the end I concluded that if you did need this type of data it is perfectly reasonable to create per instance step items in the flow, rather than predefined objects.

This comment has been minimized.

@josecoll

josecoll May 4, 2017

Contributor

Not sure I understand the logic of this, but again, wouldn't you just associated extraAuditData with a Context object that is passed around and into a flow.

This comment has been minimized.

@mikehearn

mikehearn May 4, 2017

Contributor

Yep, that was always the idea - somewhere along the line things got a bit broken and now it's kind of hard to define your own step classes, but the idea was that steps could be more dynamic than just constants (e.g. to express progress bars), that's why they are objects and not just strings.

* Flows can call this method ensure that the active FlowInitiator is authorised for a particular action.
* This provides fine grained control over application level permissions.
* An audit event is always recorded when this method is used.
* If the permission is not granted for the FlowInitiator a FlowException

This comment has been minimized.

@mikehearn

mikehearn May 4, 2017

Contributor

unfinished sentence: a [FlowException] is thrown.

* @param extraAuditData in the audit log for this permission check these extra key value pairs will be recorded
*/
@Throws(FlowException::class)
fun checkFlowPermission(permissionName: String, extraAuditData: Map<String,String>) = stateMachine.checkFlowPermission(permissionName, extraAuditData)

This comment has been minimized.

@mikehearn

mikehearn May 4, 2017

Contributor

Maybe call out specifically in the kdoc:

  1. Example use cases
  2. How it's different to the StartFlow permission?

It wasn't until I read to the bottom of the PR that I fully understood why this wasn't duplication of the checks the RPC already does. But I'm still trying to come up with a case where you would have permission to start a flow, but then have it die half way through because you didn't have enough permissions to complete it.

This comment has been minimized.

@mnesbit

mnesbit May 4, 2017

Author Contributor

Often one doesn't really know what fine grained permissions are required until you have unpacked the initiating request. Though I agree any sane flow would do this very early on.

* @param comment a general human readable summary of the event.
* @param extraAuditData in the audit log for this permission check these extra key value pairs will be recorded.
*/
fun recordAuditEvent(eventType: String, comment: String, extraAuditData: Map<String,String>) = stateMachine.recordAuditEvent(eventType, comment, extraAuditData)

This comment has been minimized.

@mikehearn

mikehearn May 4, 2017

Contributor

When I see stuff like this, I wonder if we should we thinking about integrating with log4j - seems like people might want logger logs and audit event comments to frequently contain the same thing?

This comment has been minimized.

@mnesbit

mnesbit May 4, 2017

Author Contributor

I fully expect to have the default AuditService sending stuff to logs as well as the DB. In production Audit should not go to logs as it exposes secret data and might be edited to hide one's tracks.

This comment has been minimized.

@josecoll

josecoll May 10, 2017

Contributor

Severity of an Audit Event may be important also.

@@ -14,20 +14,40 @@ import org.slf4j.Logger
import java.util.*

/**
* Wrapper over a general authenticated user principal token. The mechanism of authentication

This comment has been minimized.

@mikehearn

mikehearn May 4, 2017

Contributor

Isn't this the same as java.security.Principal? At least ..... in principle? 😹

* The extra data will be recorded to the audit logs when the flow progresses.
* Even if empty the basic details (i.e. label) of the step will be recorded for audit purposes.
*/
open val extraAuditData: Map<String, String> get() = emptyMap()

This comment has been minimized.

@mikehearn

mikehearn May 4, 2017

Contributor

Yep, that was always the idea - somewhere along the line things got a bit broken and now it's kind of hard to define your own step classes, but the idea was that steps could be more dynamic than just constants (e.g. to express progress bars), that's why they are objects and not just strings.

* Marker interface extension of FlowAuditEvent to capture the initiation of a new flow.
* The flow parameters should be captured to the context data.
*/
interface FlowStartEvent : FlowAuditEvent {

This comment has been minimized.

@mikehearn

mikehearn May 4, 2017

Contributor

You can leave out the { } for empty classes and interfaces in kotlin.

* Empty do nothing AuditService as placeholder.
* TODO Write a full implementation that expands all the audit events to the database.
*/
class DummyAuditService : AuditService, SingletonSerializeAsToken() {

This comment has been minimized.

@mikehearn

mikehearn May 4, 2017

Contributor

Maybe this and the concrete data classes should be inside a .internal. package?

* to trusted internal components via ServiceHubInternal.
*/
interface AuditService {
fun recordSystemAuditEvent(event: SystemAuditEvent)

This comment has been minimized.

@mikehearn

mikehearn May 4, 2017

Contributor

I guess the assumption is we'll add more methods here in future? Otherwise it can as well be a function on servicehub itself.

This comment has been minimized.

@josecoll

josecoll May 10, 2017

Contributor

I would rename this to recordAuditEvent(event: AuditEvent) and then provide subclasses of AuditEvent types with a category such as Business or System.

This comment has been minimized.

@mnesbit

mnesbit May 5, 2017

Author Contributor

The intent is that different audit back-ends can implement this common interface and be plugged into the node. I do expect other functions on this eventually too.

@josecoll
Copy link
Contributor

josecoll left a comment

Code level comments.
General comments to follow.

@@ -143,9 +143,29 @@ abstract class FlowLogic<out T> {
}

/**
* Flows can call this method ensure that the active FlowInitiator is authorised for a particular action.

This comment has been minimized.

@josecoll

josecoll May 4, 2017

Contributor

... method to ensure ...

* Flows can call this method ensure that the active FlowInitiator is authorised for a particular action.
* This provides fine grained control over application level permissions.
* An audit event is always recorded when this method is used.
* If the permission is not granted for the FlowInitiator a FlowException

This comment has been minimized.

@josecoll

josecoll May 4, 2017

Contributor

... a FlowException is thrown.

* @param extraAuditData in the audit log for this permission check these extra key value pairs will be recorded
*/
@Throws(FlowException::class)
fun checkFlowPermission(permissionName: String, extraAuditData: Map<String,String>) = stateMachine.checkFlowPermission(permissionName, extraAuditData)

This comment has been minimized.

@josecoll

josecoll May 4, 2017

Contributor

wouldn't this be better offered as an Annotation?
In general, wouldn't Audit annotations be applied within the platform code?
Not sure I get the extraAuditData piece? (what and where does this come from? should it not be standardised somewhere else at deployment time?)

This comment has been minimized.

@mnesbit

mnesbit May 4, 2017

Author Contributor

I think annotations would be nice for some things, but they can't be dynamic enough for real decisions. So things like checks that a user is in the right account group wouldn't be possible.

* @param comment a general human readable summary of the event.
* @param extraAuditData in the audit log for this permission check these extra key value pairs will be recorded.
*/
fun recordAuditEvent(eventType: String, comment: String, extraAuditData: Map<String,String>) = stateMachine.recordAuditEvent(eventType, comment, extraAuditData)

This comment has been minimized.

@josecoll

josecoll May 4, 2017

Contributor

Should the eventType be an enumeration of standard types?

*/
fun recordAuditEvent(eventType: String, comment: String, extraAuditData: Map<String,String>) = stateMachine.recordAuditEvent(eventType, comment, extraAuditData)

/**
* Override this to provide a [ProgressTracker]. If one is provided and stepped, the framework will do something

This comment has been minimized.

@josecoll

josecoll May 4, 2017

Contributor

Seems odd that this generated a dependency on whether or not to use a ProgressTracker?
What do you mean by: "If one is provided and stepped" ?

/**
* Simple concrete data class implementation of FlowAuditEvent
*/
data class FlowAuditEventImpl(override val timestamp: Instant,

This comment has been minimized.

@josecoll

josecoll May 4, 2017

Contributor

Can this not extend the SystemAuditEventImpl data class?

This comment has been minimized.

@mnesbit

mnesbit May 4, 2017

Author Contributor

Sadly not with data classes.

/**
* Simple concrete data class implementation of FlowStartEvent
*/
data class FlowStartEventImpl(override val timestamp: Instant,

This comment has been minimized.

@josecoll

josecoll May 4, 2017

Contributor

ditto

/**
* Simple concrete data class implementation of FlowProgressAuditEvent
*/
data class FlowProgressAuditEventImpl(override val timestamp: Instant,

This comment has been minimized.

@josecoll

josecoll May 4, 2017

Contributor

ditto

/**
* Simple concrete data class implementation of FlowErrorAuditEvent
*/
data class FlowErrorAuditEventImpl(override val timestamp: Instant,

This comment has been minimized.

@josecoll

josecoll May 4, 2017

Contributor

ditto

/**
* Simple concrete data class implementation of FlowPermissionAuditEvent
*/
data class FlowPermissionAuditEventImpl(override val timestamp: Instant,

This comment has been minimized.

@josecoll

josecoll May 4, 2017

Contributor

ditto

@josecoll

This comment has been minimized.

Copy link
Contributor

josecoll commented May 4, 2017

I wholeheartedly agree we need a proper UserService (and underlying pluggable implementation to connected to LDAP, Kerberos or other similar user authentication and management systems).

Definition of User and Permission sets in Corda is currently file system based (configuration files for RPC users, cert files for node identity) - which is neither scalable nor manageable in a Production environment. It should be possible to leverage an existing Permissioning framework (inclusive of underlying persistence infrastructure and/or management UI's) to manage Users, Roles and Permission sets. For example, Spring Data provides some usable, out-of-the-box support in these area: default set of schemas to manage users, roles and permission sets - scroll to Database Schemas in attached.

@mnesbit mnesbit force-pushed the mnesbit-audit-interface branch 3 times, most recently from 0475ef5 to 23ba083 May 5, 2017

@mikehearn
Copy link
Contributor

mikehearn left a comment

OK, this is looking good now.

@josecoll

This comment has been minimized.

Copy link
Contributor

josecoll commented May 10, 2017

As requested, my summary of suggested changes is as follows:

  • enumerating Audit Event Types
  • adding an Audit Event Severity field
  • decoupling Audit from Progress Tracker
  • adding a context object that flows information around our API's
  • adoption of a Permissioning service (user, roles, permission sets; DB persisted & UI driven)
@josecoll
Copy link
Contributor

josecoll left a comment

Approved on the basis @mikehearn has already approved it and don't want to hold you back.

* @param comment a general human readable summary of the event.
* @param extraAuditData in the audit log for this permission check these extra key value pairs will be recorded.
*/
fun recordAuditEvent(eventType: String, comment: String, extraAuditData: Map<String,String>) = stateMachine.recordAuditEvent(eventType, comment, extraAuditData)

This comment has been minimized.

@josecoll

josecoll May 10, 2017

Contributor

Severity of an Audit Event may be important also.

* to trusted internal components via ServiceHubInternal.
*/
interface AuditService {
fun recordSystemAuditEvent(event: SystemAuditEvent)

This comment has been minimized.

@josecoll

josecoll May 10, 2017

Contributor

I would rename this to recordAuditEvent(event: AuditEvent) and then provide subclasses of AuditEvent types with a category such as Business or System.

@josecoll
Copy link
Contributor

josecoll left a comment

Approved on the basis @mikehearn has already approved it and don't want to hold you back.

Create header files for discussion of possible flow audit api.
Fix compile error

Address PR comments

Change from a general interface to a restricted set of audit event types.

Fixup after rebase

@mnesbit mnesbit force-pushed the mnesbit-audit-interface branch from 23ba083 to 540fd74 May 10, 2017

@mnesbit mnesbit merged commit 8aa341d into master May 10, 2017

1 check passed

Pull Requests (Corda) - merge TeamCity build finished
Details

@mnesbit mnesbit deleted the mnesbit-audit-interface branch May 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.