Prevent perpetual use of expired auth signature; remove superfluous API functions

derekpierre commented 3 weeks ago

Type of PR:

[ ] Bugfix
[ ] Feature
[ ] Documentation
[x] Other

Required reviews:

[ ] 1
[ ] 2
[x] 3

What this does:

High-level idea of the changes introduced in this PR. List relevant API changes (if any), as well as related PRs and issues.

Based over #560 (somewhat of a follow-up PR).

EIP4361AuthProvider caches the auth signature it first generates perpetually (unless storage is explicitly cleared eg. localStorage.clear() in apps). However, this auth signature can become expired (> 2h old), and so the provider should detect that on cache retrieval and regenerate the auth signature if it becomes expired.
Add test for SingleSignOnEIP4361AuthProvider
Clean up tests so that signer is obtained from provider, instead of creating separate fakeProvider() and fakeSigner(); this way they are better linked for tests. This only applies to tests - in real life it is possible to have a signer with no provider, but this was not the case for tests prior to this change anyway.
Remove no longer applicable/superfluous API calls from being exposed:
- isAuthorized(...) - calls into the Coordinator contract to call this function, but we are moving away from this where calls should be made directly to the AccessController. However the accessController only checks for isEncryptionAuthorized and doesn't provide the ability to check if an encrypted address is authorized - which is what is implemented here. Therefore, it only applies to the GlobalAllowList contracts, but it's unclear if we need that ability in taco-web
- registerEncrypters(...) - Was never publicly exposed and it is unclear if we need this ability in taco-web. #324 alluded to it, but not sure if we still care. Let me know if we do (cc @arjunhassard ).
If we decide down the road we need them, we can add them back.

Issues fixed/closed:

Fixes #...

Closes https://github.com/nucypher/taco-web/issues/324

Why it's needed:

Explain how this PR fits in the greater context of the NuCypher Network. E.g., if this PR address a nucypher/productdev issue, let reviewers know!

Notes for reviewers:

What should reviewers focus on? Is there a particular commit/function/section of your PR that requires more attention from reviewers?

netlify[bot] commented 3 weeks ago

Deploy Preview for taco-nft-demo canceled.

Name	Link
Latest commit	236f2963e68ff116acf55a2c30e0bce67ebe4e0d
Latest deploy log	https://app.netlify.com/sites/taco-nft-demo/deploys/66c4e6ce70156c0009928c63

netlify[bot] commented 3 weeks ago

Deploy Preview for taco-demo canceled.

Name	Link
Latest commit	236f2963e68ff116acf55a2c30e0bce67ebe4e0d
Latest deploy log	https://app.netlify.com/sites/taco-demo/deploys/66c4e6ceb8611e0008572d8f

codecov-commenter commented 3 weeks ago

Codecov Report

Attention: Patch coverage is 84.61538% with 6 lines in your changes missing coverage. Please review.

Project coverage is 88.55%. Comparing base (e8f9098) to head (236f296). Report is 177 commits behind head on main.

Files	Patch %	Lines
...ackages/taco-auth/src/providers/eip4361/eip4361.ts	80.00%	3 Missing and 1 partial :warning:
packages/taco-auth/src/storage.ts	85.71%	2 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #567 +/- ## =========================================== + Coverage 23.12% 88.55% +65.42% =========================================== Files 62 59 -3 Lines 10175 6098 -4077 Branches 260 300 +40 =========================================== + Hits 2353 5400 +3047 + Misses 7763 660 -7103 + Partials 59 38 -21 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

derekpierre commented 2 weeks ago

Overall looks good 👍🏻 While I do find this acceptable (and very convenient for implementers) it's interesting to observe the lines between application and library logic blurring a bit here.

Because we opted to cache the message ourselves to prevent users having to sign every time for proof of wallet ownership, it forces us to manage the cache appropriately - this PR does the latter. Definitely open to ideas here if you have any.

nucypher / taco-web