-
Notifications
You must be signed in to change notification settings - Fork 22
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
Expose auth providers from taco
#534
Changes from 1 commit
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 |
---|---|---|
|
@@ -18,14 +18,33 @@ const ERR_MISSING_SIWE_PARAMETERS = 'Missing default SIWE parameters'; | |
|
||
export class EIP4361AuthProvider { | ||
private readonly storage: LocalStorage; | ||
private readonly providerParams: EIP4361AuthProviderParams; | ||
|
||
constructor( | ||
// TODO: We only need the provider to fetch the chainId, consider removing it | ||
private readonly provider: ethers.providers.Provider, | ||
private readonly signer: ethers.Signer, | ||
private readonly providerParams?: EIP4361AuthProviderParams, | ||
providerParams?: EIP4361AuthProviderParams, | ||
) { | ||
this.storage = new LocalStorage(); | ||
if (providerParams) { | ||
this.providerParams = providerParams; | ||
} else { | ||
this.providerParams = this.getDefaultParameters(); | ||
} | ||
} | ||
|
||
private getDefaultParameters() { | ||
if (typeof window !== 'undefined') { | ||
// If we are in a browser environment, we can get the domain and uri from the window object | ||
const maybeOrigin = window?.location?.origin; | ||
return { | ||
domain: maybeOrigin.split('//')[1].split('.')[0], | ||
uri: maybeOrigin, | ||
}; | ||
} | ||
// If not, we have no choice but to throw an error | ||
throw new Error(ERR_MISSING_SIWE_PARAMETERS); | ||
piotr-roslaniec marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
public async getOrCreateAuthSignature(): Promise<AuthSignature> { | ||
|
@@ -46,7 +65,7 @@ export class EIP4361AuthProvider { | |
|
||
private async createSIWEAuthMessage(): Promise<AuthSignature> { | ||
const address = await this.signer.getAddress(); | ||
const { domain, uri } = this.getParametersOrDefault(); | ||
const { domain, uri } = this.providerParams; | ||
const version = '1'; | ||
const nonce = generateNonce(); | ||
const chainId = (await this.provider.getNetwork()).chainId; | ||
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. Can we do 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. Not every I think we may want to explore requiring that every 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. Ah ok. |
||
|
@@ -64,26 +83,4 @@ export class EIP4361AuthProvider { | |
const signature = await this.signer.signMessage(message); | ||
return { signature, address, scheme, typedData: message }; | ||
} | ||
|
||
// TODO: Create a facility to set these parameters or expose them to the user | ||
private getParametersOrDefault(): { | ||
domain: string; | ||
uri: string; | ||
} { | ||
// If we are in a browser environment, we can get the domain and uri from the window object | ||
if (typeof window !== 'undefined') { | ||
const maybeOrigin = window?.location?.origin; | ||
return { | ||
domain: maybeOrigin.split('//')[1].split('.')[0], | ||
uri: maybeOrigin, | ||
}; | ||
} | ||
if (this.providerParams) { | ||
return { | ||
domain: this.providerParams.domain, | ||
uri: this.providerParams.uri, | ||
}; | ||
} | ||
throw new Error(ERR_MISSING_SIWE_PARAMETERS); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,7 +13,7 @@ import { | |
GlobalAllowListAgent, | ||
toBytes, | ||
} from '@nucypher/shared'; | ||
import { makeAuthProviders } from '@nucypher/taco-auth'; | ||
import { AuthProviders, makeAuthProviders } from '@nucypher/taco-auth'; | ||
import { ethers } from 'ethers'; | ||
import { keccak256 } from 'ethers/lib/utils'; | ||
|
||
|
@@ -143,6 +143,25 @@ export const decrypt = async ( | |
porterUri?: string, | ||
signer?: ethers.Signer, | ||
customParameters?: Record<string, CustomContextParam>, | ||
): Promise<Uint8Array> => { | ||
const authProviders = makeAuthProviders(provider, signer); | ||
return decryptWithAuthProviders( | ||
provider, | ||
domain, | ||
messageKit, | ||
authProviders, | ||
porterUri, | ||
customParameters, | ||
); | ||
}; | ||
|
||
export const decryptWithAuthProviders = async ( | ||
piotr-roslaniec marked this conversation as resolved.
Show resolved
Hide resolved
|
||
provider: ethers.providers.Provider, | ||
domain: Domain, | ||
messageKit: ThresholdMessageKit, | ||
authProviders?: AuthProviders, | ||
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. Interested to see what this parameter looks like when EIP712 eventually gets removed. 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. If we don't foresee new providers near-term, it could be just |
||
porterUri?: string, | ||
customParameters?: Record<string, CustomContextParam>, | ||
): Promise<Uint8Array> => { | ||
if (!porterUri) { | ||
porterUri = getPorterUri(domain); | ||
|
@@ -154,8 +173,6 @@ export const decrypt = async ( | |
messageKit.acp.publicKey, | ||
); | ||
const ritual = await DkgClient.getActiveRitual(provider, domain, ritualId); | ||
// TODO: Temporary helper method to keep the external taco.ts decrypt function simple | ||
const authProviders = makeAuthProviders(provider, signer); | ||
return retrieveAndDecrypt( | ||
provider, | ||
domain, | ||
|
@@ -188,7 +205,7 @@ export const isAuthorized = async ( | |
domain: Domain, | ||
messageKit: ThresholdMessageKit, | ||
ritualId: number, | ||
) => | ||
): Promise<boolean> => | ||
DkgCoordinatorAgent.isEncryptionAuthorized( | ||
provider, | ||
domain, | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Judging by this code here - https://1.x.wagmi.sh/examples/sign-in-with-ethereum#step-3-sign--verify-message, assuming
window.location
is available, can we just use thehost
property for thedomain
value instead of the parsing?Also, out of curiosity is
document
an option in addition towindow
? Noticed this here - https://github.com/spruceid/siwe-notepad/blob/main/src/providers.ts#L132. Not sure what environmentdocument
is available ... 🤔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.
Yes,
document
andwindow
are globally available objects provided by the browser environment.I updated this snippet.