-
Notifications
You must be signed in to change notification settings - Fork 2
issue/48 #59
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
Conversation
|
|
||
| # Build crypto/bytes | ||
| WORKDIR /home/node/oko | ||
| RUN yarn workspaces focus @oko-wallet/bytes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question
| @@ -0,0 +1 @@ | |||
| // TODO: @retto | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| RAISE EXCEPTION 'audit_event is append-only'; | ||
| END; $$ LANGUAGE plpgsql; | ||
|
|
||
| CREATE TRIGGER audit_event_block_u BEFORE UPDATE ON audit_event |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lidarbtc Do we have to define "trigger" event? I'm generally quite against this idea. What's the use? (curious)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My intention is to prevent updates and deletions in the database even if the app is compromised. I anticipate such situations would be limited, so if you think this is excessive, I'll remove it.
| // Create audit context for login route | ||
| const auditContext: AuditContext = { | ||
| db: state.db, | ||
| adminUserId: undefined, // Not authenticated yet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by default, AuditContext require admin's id string and user login route doesn't have admin's id string so I made custom audit context for this route.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is quite an arbitrary string concat. Maybe we can at least define function to create this string concat and name the function with the intent?
Upon reconsideration, the login should probably be adminUserId set to “system”. I will remove this and make it to use default AuditContext.
Pull Request
CONTRIBUTING.mdand followed the guidelines.Summary
Links (Issue References, etc, if there's any)