-
Notifications
You must be signed in to change notification settings - Fork 23
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
Allow reusing SIWE messages in EIP4361-based condition authentication #529
Changes from all commits
d0a2be6
680fe19
1bf3960
275f00e
f77f973
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,16 @@ | ||
import {ethers} from "ethers"; | ||
|
||
import { EIP4361AuthProvider } from './providers/eip4361'; | ||
import { EIP4361AuthProvider, EIP4361AuthProviderParams } from './providers/eip4361'; | ||
import { EIP712AuthProvider } from './providers/eip712'; | ||
import { AuthProviders, EIP4361_AUTH_METHOD, EIP712_AUTH_METHOD } from './types'; | ||
|
||
export const makeAuthProviders = (provider: ethers.providers.Provider, signer?: ethers.Signer): AuthProviders => { | ||
export const makeAuthProviders = ( | ||
provider: ethers.providers.Provider, | ||
signer?: ethers.Signer, | ||
siweDefaultParams?: EIP4361AuthProviderParams | ||
): AuthProviders => { | ||
return { | ||
[EIP712_AUTH_METHOD]: signer ? new EIP712AuthProvider(provider, signer) : undefined, | ||
[EIP4361_AUTH_METHOD]: signer ? new EIP4361AuthProvider(provider, signer) : undefined | ||
[EIP4361_AUTH_METHOD]: signer ? new EIP4361AuthProvider(provider, signer, siweDefaultParams) : undefined | ||
} as AuthProviders; | ||
}; |
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -6,22 +6,24 @@ import { CoreConditions, CoreContext } from '../../types'; | |||||||||||
import { CompoundConditionType } from '../compound-condition'; | ||||||||||||
import { Condition, ConditionProps } from '../condition'; | ||||||||||||
import { ConditionExpression } from '../condition-expr'; | ||||||||||||
import { CONTEXT_PARAM_PREFIX, CONTEXT_PARAM_REGEXP, RESERVED_CONTEXT_PARAMS } from '../const'; | ||||||||||||
import { CONTEXT_PARAM_PREFIX, CONTEXT_PARAM_REGEXP, RESERVED_CONTEXT_PARAMS, USER_ADDRESS_PARAMS } from '../const'; | ||||||||||||
|
||||||||||||
|
||||||||||||
export type CustomContextParam = string | number | boolean; | ||||||||||||
export type CustomContextParam = string | number | boolean | AuthSignature; | ||||||||||||
export type ContextParam = CustomContextParam | AuthSignature; | ||||||||||||
|
||||||||||||
const ERR_RESERVED_PARAM = (key: string) => | ||||||||||||
`Cannot use reserved parameter name ${key} as custom parameter`; | ||||||||||||
const ERR_INVALID_CUSTOM_PARAM = (key: string) => | ||||||||||||
`Custom parameter ${key} must start with ${CONTEXT_PARAM_PREFIX}`; | ||||||||||||
const ERR_AUTH_PROVIDER_REQUIRED = (key: string) => | ||||||||||||
`Authentication provider required to satisfy ${key} context variable in condition`; | ||||||||||||
`No matching authentication provider to satisfy ${key} context variable in condition`; | ||||||||||||
const ERR_MISSING_CONTEXT_PARAMS = (params: string[]) => | ||||||||||||
`Missing custom context parameter(s): ${params.join(', ')}`; | ||||||||||||
const ERR_UNKNOWN_CONTEXT_PARAMS = (params: string[]) => | ||||||||||||
`Unknown custom context parameter(s): ${params.join(', ')}`; | ||||||||||||
const ERR_NO_AUTH_PROVIDER_FOR_PARAM = (param: string) => | ||||||||||||
`No custom parameter for requested context parameter: ${param}`; | ||||||||||||
|
||||||||||||
export class ConditionContext { | ||||||||||||
public requestedParameters: Set<string>; | ||||||||||||
|
@@ -86,16 +88,24 @@ export class ConditionContext { | |||||||||||
} | ||||||||||||
|
||||||||||||
private validateAuthProviders(requestedParameters: Set<string>): void { | ||||||||||||
requestedParameters | ||||||||||||
.forEach(param => { | ||||||||||||
const maybeAuthMethod = AUTH_METHOD_FOR_PARAM[param]; | ||||||||||||
if (!maybeAuthMethod) { | ||||||||||||
return; | ||||||||||||
} | ||||||||||||
if (!this.authProviders[maybeAuthMethod]) { | ||||||||||||
throw new Error(ERR_AUTH_PROVIDER_REQUIRED(param)); | ||||||||||||
} | ||||||||||||
}); | ||||||||||||
for (const param of requestedParameters) { | ||||||||||||
// If it's not a user address parameter, we can skip | ||||||||||||
if (!USER_ADDRESS_PARAMS.includes(param)) { | ||||||||||||
continue; | ||||||||||||
} | ||||||||||||
|
||||||||||||
// If it's a user address parameter, we need to check if we have an auth provider | ||||||||||||
const authMethod = AUTH_METHOD_FOR_PARAM[param]; | ||||||||||||
if (!authMethod && !this.customParameters[param]) { | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For this case, should we also check that the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we validate it against the structure of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Documented in #535, will address in a follow-up PR |
||||||||||||
// If we don't have an auth method, and we don't have a custom parameter, we have a problem | ||||||||||||
throw new Error(ERR_NO_AUTH_PROVIDER_FOR_PARAM(param)); | ||||||||||||
} | ||||||||||||
|
||||||||||||
// If we have an auth method, but we don't have an auth provider, we have a problem | ||||||||||||
if (authMethod && !this.authProviders[authMethod]) { | ||||||||||||
throw new Error(ERR_AUTH_PROVIDER_REQUIRED(param)); | ||||||||||||
} | ||||||||||||
} | ||||||||||||
} | ||||||||||||
|
||||||||||||
private async fillAuthContextParameters(requestedParameters: Set<string>): Promise<Record<string, ContextParam>> { | ||||||||||||
|
@@ -119,12 +129,20 @@ export class ConditionContext { | |||||||||||
// First, we want to find all the parameters we need to add | ||||||||||||
const requestedParameters = new Set<string>(); | ||||||||||||
|
||||||||||||
// Search conditions for parameters | ||||||||||||
// Check return value test | ||||||||||||
if (condition.returnValueTest) { | ||||||||||||
const rvt = condition.returnValueTest.value; | ||||||||||||
if (ConditionContext.isContextParameter(rvt)) { | ||||||||||||
// Return value test can be a single parameter or an array of parameters | ||||||||||||
if (Array.isArray(rvt)) { | ||||||||||||
rvt.forEach((value) => { | ||||||||||||
if (ConditionContext.isContextParameter(value)) { | ||||||||||||
requestedParameters.add(value); | ||||||||||||
} | ||||||||||||
}); | ||||||||||||
} else if (ConditionContext.isContextParameter(rvt)) { | ||||||||||||
requestedParameters.add(rvt); | ||||||||||||
} else { | ||||||||||||
// Not a context parameter, we can skip | ||||||||||||
} | ||||||||||||
Comment on lines
+144
to
146
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the empty block for a comment something common in typescript? Perhaps just the comment?
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's no TS-related, just something I sometimes do to be more explicit. |
||||||||||||
} | ||||||||||||
|
||||||||||||
|
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.
Why not delete these lines instead of commenting them out?
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.
I thought it was more explicit (explicitly disabled), is it confusing? Maybe an assertion is better.
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.
Interesting. Where would you put the assertion?
Alternatively, maybe keep the comment on L37 and remove L38?
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.
An assertion could be a statement in this file that gets executed when the file is imported. That's a bit ham-fisted, I need to think about this. In the meantime, I will remove L38.
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.
Agreed - the assertion seems too heavy.