-
Notifications
You must be signed in to change notification settings - Fork 2
OKO-575 #250
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
| * Defines which API names are allowed for each operation type. | ||
| * Each operation type has a specific set of APIs that can be called. | ||
| */ | ||
| export const ALLOWED_APIS: Record<OperationType, string[]> = { |
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 not a generic Record of the type described as such. This is a very specific object instance.
|
|
||
| await pool.query( | ||
| `INSERT INTO "commit_reveal_sessions" | ||
| (session_id, operation_type, client_ephemeral_pubkey, id_token_hash, state, expires_at) |
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.
Just so you know, you are assuming the whitespace in the left side since you used backtick operators
| hasCommitRevealApiBeenCalled, | ||
| } from "@oko-wallet/oko-pg-interface/commit_reveal"; | ||
|
|
||
| import { ErrorCodeMap } from "@oko-wallet/oko-api-error-codes"; |
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.
external deps
| app.use("/keyshare/v2", keyshareV2Router); | ||
|
|
||
| const commitRevealRouter = makeCommitRevealRouter(); | ||
| app.use("/commit-reveal/v2", commitRevealRouter); |
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.
Not anymore valid
|
@eldenpark Ready for review. I’ve addressed the comments |
|
|
||
| // Verify signature: message = node_pubkey + session_id + auth_type + id_token + operation_type + api_name | ||
| const nodePubkeyHex = state.server_keypair.publicKey.toHex(); | ||
| const message = `${nodePubkeyHex}${cr_session_id}${authType}${idToken}${session.operation_type}${apiName}`; |
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 should become a function. I'll modify.
| | "register_ed25519"; | ||
|
|
||
| export const ALLOWED_APIS = { | ||
| export const ALLOWED_APIS: Record<OperationType, ApiName[]> = { |
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.
Record<OperationType, ApiName[]> implies any of the matches between K and V, such that
sign_in => register" is a valid interface. This is not the case. I'll modify.
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.
Record<OperationType, ApiName[]> implies any of the matches between K and V, such that sign_in => register" is a valid interface. This is not the case. I'll modify.
I applied the same changes to the oko api middleware as well.
|
@chihunmanse Why does "commit-reveal" operation need to happen between user <> oko? |
Because the oko server also uses the user’s OAuth token for sign-up and sign-in, it can theoretically front-run just like a ks node. |
| } | ||
|
|
||
| // message = node_pubkey + session_id + auth_type + id_token + operation_type + api_name | ||
| function makeSigMessage({ |
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.
@chihunmanse Please review the change I made here. We may want to use this function in other places as well.
| const state = params.state ?? "COMMITTED"; | ||
|
|
||
| await pool.query( | ||
| `INSERT INTO "commit_reveal_sessions" (session_id, operation_type, client_ephemeral_pubkey, id_token_hash, state, expires_at) VALUES ($1, $2, $3, $4, $5, $6)`, |
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.
Too long and I'd like to set a coding practice where we write 4 items in a line. I'll make a change
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.
Well, never mind. It's a test case and I'll leave it as it is!
Pull Request
CONTRIBUTING.mdand followed the guidelines.Summary
Links (Issue References, etc, if there's any)