Allow reusing SIWE messages in EIP4361-based condition authentication #529

piotr-roslaniec · 2024-06-12T09:48:53Z

Type of PR:

Feature

Required reviews:

2

What this does:

Allows reusing of existing SIWE messages by providing them as a context parameter at decryption time
Internally, it reuses the :userAddressEIP4361 context parameter - Hence, no changes on the server side required.

Issues fixed/closed:

Fixes Add single sign-on capabilities to EIP4361 support #528

Notes for reviewers:

Context variable handling may still change post https://github.com/nucypher/tdec/issues/179

piotr-roslaniec · 2024-06-13T07:20:49Z

Sending review requests for an early review of this draft

manumonti

This looks good to me, not sure how much more work is needed, but it seems pretty complete.

packages/taco/test/conditions/context.test.ts

piotr-roslaniec · 2024-06-21T12:08:37Z

Asking for review despite some rough edges to confirm the design

derekpierre

Based on the changes in #531 , feels like this PR would look a lot different post-#531. And if merged pre-#531 it would create quite a bit of merge conflicts for #531. Would basing over #531 make that simpler?

We also don't address how to help the user "easily" generates the AuthSignature .
It would be nice to have to a function/API (seemingly an AuthProvider) that helps with generating the AuthSignature format we want for external SIWE? Or is that planned for a follow-up? wdyt?

packages/taco/src/conditions/context/context.ts

piotr-roslaniec · 2024-06-24T07:26:49Z

@derekpierre I think we can rebase this over #531.

I added a public method to the EIP4361 auth provider to expose message-creating logic to end-users.

piotr-roslaniec · 2024-06-24T11:52:46Z

Rebased over #531

derekpierre

One thought for this PR, was that the AuthSignature is the thing that is TACo-specific as opposed to the SIWE message. It could be helpful to have something (utility function or class or otherwise) that generates the AuthSignature type so that users don't need to know the specifics of it eg. generateSIWEAuthSignature(address: string, siweMessage: string, signature: Uint8Array). With something like that, the user can use whatever they want for generating the siweMessage and taco-auth can take those as parameters and simply produce an AuthSignature type for them.

It's not something overly flashy, but is helpful in abstracting away the creation of the AuthSignature a bit.

If they are using single sign-on then presumably they already have a SIWE library that they are using and we would be helping them convert that into a format that we can use i.e. the AuthSignature.

Thoughts?

packages/taco/src/conditions/context/context.ts

derekpierre · 2024-06-24T18:50:03Z

packages/taco-auth/src/providers/eip4361.ts

+    return { signature, address, scheme, typedData: message };
+  }
+
+  public static prepareSiweMessage(address: string, chainId: number, domain: string, uri: string) {


Is this static method related to the TODO on L61?

I might be missing the use case for this method? Just a helper for the user to create a SIWE message to then use for the context variable? If so, it seems pretty restrictive since there are more values that SIWE allows apps to customize.

In terms of our default EIP4361 auth provider behaviour, there is a question about what values to use for domain and uri, and whether the user can customize that, so wondering if this is related?

Just a helper for the user to create a SIWE message to then use for the context variable?

Yes, it's just a helper function to expose a SIWE-message-creating logic to end-users. I think I will also refactor-out the types related to SIWE so that it's more configurable and gives users more freedom (as you've noted).

so wondering if this is related?

Yes, it's related, but I planned on fixing the defaults later. I will attempt to squeeze it into this PR, but if it doesn't make sense I will follow up in the next one.

Yes, it's just a helper function to expose a SIWE-message-creating logic to end-users. I think I will also refactor-out the types related to SIWE so that it's more configurable and gives users more freedom (as you've noted).

In that case, I'm a bit apprehensive about the benefits of us trying to provide extensive SIWE-message-creating logic. Then we just end up either making something more restrictive with limited use (less parameters than the SIWE API) or writing a layer on top of the SIWE library that uses the same API parameterization of the library anyway. Users should just use the siwe library or some other one for generating the SIWE message if they want such a high level of customization of messages.

At the moment, we have two worlds:

Apps that expect TACo to do the SIWE info for them because they don't use SIWE at all (:userAddress)

Apps that currently use SIWE and expect to provide TACo with their existing SIWE info so that it can be reused (:userAddressExternalEIP4361)

In world 1) apps don't currently use SIWE at all, so TACo will create/generate the AuthSignature on their behalf. At most, the app may need to specify the domain / uri if taco-auth can't detect those values from the window object / use some default value to get the right value based on the SIWE spec (i.e. prevent the signing from being flagged as "unsafe"). But these apps probably don't need more customization than that, or else they would probably be in world 2) anyway.

In world 2) the app uses their own tooling for the SIWE message/signature and all they want to do is give that information to TACo (Ceramic example). No customization of SIWE message parameters needed here because the message and signature are already generated by the app separately.

I see your point. I had a slightly different idea of how to go about this, but I see merit in this solution.

I dropped the commit with previous changes and added an ability to set defaults for the EIP4361 provider. I will expose it as a part of #533

I had a slightly different idea of how to go about this, but I see merit in this solution.

Happy to listen to and work through your ideas.

So far I'm happy with this PR as it is, I think we may revisit this once we deprecate EIP712 and start ironing-out the user-facing API

derekpierre · 2024-06-24T18:50:32Z

packages/taco/src/conditions/const.ts

+  // USER_ADDRESS_PARAM_EXTERNAL_EIP4361 is not reserved and can be used as a custom context parameter
+  // USER_ADDRESS_PARAM_EXTERNAL_EIP4361


Why not delete these lines instead of commenting them out?

I thought it was more explicit (explicitly disabled), is it confusing? Maybe an assertion is better.

Maybe an assertion is better.

Interesting. Where would you put the assertion?

Alternatively, maybe keep the comment on L37 and remove L38?

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.

Agreed - the assertion seems too heavy.

piotr-roslaniec · 2024-06-24T19:06:02Z

Thoughts?

I think AuthSignature will become a class rather than an interface, and we will add more utilities and conversion functions as we go. Adding more factory methods, especially for SIWE, makes sense. I will look into it.

derekpierre · 2024-06-26T13:06:26Z

packages/taco/src/conditions/const.ts

+  // USER_ADDRESS_PARAM_EXTERNAL_EIP4361 is not reserved and can be used as a custom context parameter
+  // USER_ADDRESS_PARAM_EXTERNAL_EIP4361


Maybe an assertion is better.

Interesting. Where would you put the assertion?

Alternatively, maybe keep the comment on L37 and remove L38?

derekpierre · 2024-06-26T13:08:26Z

packages/taco/src/conditions/context/context.ts

+      } else {
+        // Not a context parameter, we can skip
      }


Is the empty block for a comment something common in typescript? Perhaps just the comment?

Suggested change

} else {

// Not a context parameter, we can skip

}

}

// else not a context parameter, we can skip

It's no TS-related, just something I sometimes do to be more explicit.

derekpierre · 2024-06-26T13:12:03Z

packages/taco/src/conditions/context/context.ts

+
+      // 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]) {


For this case, should we also check that the customParameters[param] value is an AuthSignature? So not just that it is present in the custom parameters, but its type is correct. Or is that already done somewhere else?

I don't think we validate it against the structure of AuthSignature. We may want to use zod for that. Let's see.

Documented in #535, will address in a follow-up PR

piotr-roslaniec · 2024-07-01T09:41:32Z

I'm going to go ahead and merge this, but we can continue discussing here or in #537 if anything

piotr-roslaniec force-pushed the reuse-siwe branch 2 times, most recently from a472dec to bb84d70 Compare June 12, 2024 10:46

piotr-roslaniec requested review from KPrasch, cygnusv, theref, derekpierre and manumonti June 13, 2024 07:21

piotr-roslaniec mentioned this pull request Jun 13, 2024

[EPIC] Auth #527

Merged

17 tasks

manumonti approved these changes Jun 18, 2024

View reviewed changes

packages/taco/test/conditions/context.test.ts Outdated Show resolved Hide resolved

piotr-roslaniec marked this pull request as ready for review June 21, 2024 12:08

derekpierre reviewed Jun 21, 2024

View reviewed changes

packages/taco/src/conditions/context/context.ts Outdated Show resolved Hide resolved

derekpierre mentioned this pull request Jun 21, 2024

Accept/Process EIP-4361 passthrough context variable used for single sign-on capabilities nucypher/nucypher#3513

Merged

7 tasks

piotr-roslaniec force-pushed the reuse-siwe branch from 839775b to f85bc26 Compare June 24, 2024 07:26

piotr-roslaniec force-pushed the reuse-siwe branch from f85bc26 to ed01612 Compare June 24, 2024 11:52

derekpierre reviewed Jun 24, 2024

View reviewed changes

piotr-roslaniec force-pushed the reuse-siwe branch from ef669fa to 1f19192 Compare June 25, 2024 08:55

piotr-roslaniec mentioned this pull request Jun 25, 2024

Expose auth providers from taco #534

Merged

piotr-roslaniec added 5 commits June 26, 2024 14:54

feat(eip4361): allow for reusing siwe messages

d0a2be6

refactor: don't reuse eip4361 alias

680fe19

chore: document external eip4361 behavior

1bf3960

apply pr suggestions

275f00e

feat(auth): set defaults for eip4361 providers

f77f973

piotr-roslaniec force-pushed the reuse-siwe branch from 1f19192 to f77f973 Compare June 26, 2024 12:54

manumonti approved these changes Jun 26, 2024

View reviewed changes

derekpierre reviewed Jun 26, 2024

View reviewed changes

piotr-roslaniec mentioned this pull request Jun 28, 2024

Validate user-provided AuthSignature #537

Merged

piotr-roslaniec merged commit 4eea8e7 into nucypher:epic-auth Jul 1, 2024
2 checks passed

piotr-roslaniec deleted the reuse-siwe branch July 1, 2024 09:41

derekpierre mentioned this pull request Jul 8, 2024

Add single sign-on capabilities to EIP4361 support #528

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow reusing SIWE messages in EIP4361-based condition authentication #529

Allow reusing SIWE messages in EIP4361-based condition authentication #529

piotr-roslaniec commented Jun 12, 2024 •

edited

Loading

piotr-roslaniec commented Jun 13, 2024

manumonti left a comment

piotr-roslaniec commented Jun 21, 2024

derekpierre left a comment •

edited

Loading

piotr-roslaniec commented Jun 24, 2024

piotr-roslaniec commented Jun 24, 2024

derekpierre left a comment

derekpierre Jun 24, 2024

piotr-roslaniec Jun 24, 2024

derekpierre Jun 24, 2024 •

edited

Loading

piotr-roslaniec Jun 25, 2024

derekpierre Jun 25, 2024 •

edited

Loading

piotr-roslaniec Jun 26, 2024

derekpierre Jun 24, 2024

piotr-roslaniec Jun 24, 2024 •

edited

Loading

derekpierre Jun 26, 2024

piotr-roslaniec Jun 26, 2024

derekpierre Jun 27, 2024

piotr-roslaniec commented Jun 24, 2024

derekpierre Jun 26, 2024

derekpierre Jun 26, 2024

piotr-roslaniec Jun 26, 2024

derekpierre Jun 26, 2024

piotr-roslaniec Jun 26, 2024

piotr-roslaniec Jun 28, 2024

piotr-roslaniec commented Jul 1, 2024

		// USER_ADDRESS_PARAM_EXTERNAL_EIP4361 is not reserved and can be used as a custom context parameter
		// USER_ADDRESS_PARAM_EXTERNAL_EIP4361

Allow reusing SIWE messages in EIP4361-based condition authentication #529

Allow reusing SIWE messages in EIP4361-based condition authentication #529

Conversation

piotr-roslaniec commented Jun 12, 2024 • edited Loading

piotr-roslaniec commented Jun 13, 2024

manumonti left a comment

Choose a reason for hiding this comment

piotr-roslaniec commented Jun 21, 2024

derekpierre left a comment • edited Loading

Choose a reason for hiding this comment

piotr-roslaniec commented Jun 24, 2024

piotr-roslaniec commented Jun 24, 2024

derekpierre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekpierre Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekpierre Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piotr-roslaniec Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piotr-roslaniec commented Jun 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piotr-roslaniec commented Jul 1, 2024

piotr-roslaniec commented Jun 12, 2024 •

edited

Loading

derekpierre left a comment •

edited

Loading

derekpierre Jun 24, 2024 •

edited

Loading

derekpierre Jun 25, 2024 •

edited

Loading

piotr-roslaniec Jun 24, 2024 •

edited

Loading