Feat/add authenticate method pkp base #447

joshLong145 · 2024-04-29T15:49:38Z

Description

Adds an authenticate method which will use the authContext if provided to reassign the controllerSessionSigs property on PKPBase which allows for authentication operations to be explicitly invoked. This is helpful when we require to first check the authentication status before allowing other operations to occur. Without this addition it would not be possible to get the controllerSessionSigs before calling a higher level function on PKPEthers for example.
A new validation method has been added validateAuthenticate which will check on the object internally to validate that authentication operations did not fail and we are able to continue with future operations on the network.

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

An Additional check has been added to test-pkp-sign-with-session-sigs-session-cache.mjs
to assert that session has been defined before calling signMessage

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

Ansonhkg · 2024-04-29T16:40:44Z

packages/pkp-base/src/lib/pkp-base.ts

+ (await this.authContext?.client?.getSessionSigs(
+ this.authContext.getSessionSigsProps
+ )) || this.controllerSessionSigs;
+


can we have an ultimate check to see if controllerSessionSigs exists?

if(!controllerSessionSigs){ throw new Error('bruh.. no session sigs'); }

Updated the pr with a new validation method validateAuthenticate() which will throw if controllerSessionSigs is not defined after authenticate is called.

MaximusHaximus

Questions in-line

MaximusHaximus · 2024-04-29T17:15:12Z

packages/pkp-base/src/lib/pkp-base.ts

+ const controllerSessionSigs =
+ (await this.authContext?.client?.getSessionSigs(
+ this.authContext.getSessionSigsProps
+ )) || this.controllerSessionSigs;


This strikes me as a bit odd -- why would we keep the existing value for this.controllerSessionSigs here rather than throwing an error if our authenticate logic returns a falsy value, or set this.controllerSessionSigs to the falsy value that was returned?

I was also on the fence about this but wanted a fallback if a previous authenticate call was successful. But probably should just let it throw if not defined after getSessionSigs is called.

MaximusHaximus · 2024-04-29T17:18:45Z

packages/pkp-base/src/lib/pkp-base.ts

 }
 }

+ public async authenticate(): Promise<void> {


Would be good to have JSDoc on this if it's part of our public API, especially if we're going to leave the existing controllerSessionSigs intact if our call to getSessionSigs() returns a falsy value, explaining why that is

added doc header

MaximusHaximus · 2024-04-29T17:21:41Z

packages/pkp-base/src/lib/pkp-base.ts

 this.throwError('pkpPubKey (aka. uncompressPubKey) is required');
 }

- this.validateAuthContext();


Can we really get rid of this call and assume everything is correct? It seems like we would still want to call it, after calling authenticate? And if we're going to do that, wouldn't we want to leave the logic inside validateAuthContext() intact rather than removing the check for this.controllerSessionSigs?

The problem I ran into with the validation was the now both authContext and controllerSessionSigs may be defined on the object since we will assign controllerSessionSigs in scope of authenticate which will fail validation since now both will be defined.

Update: I have added a new validateAuthenticate() method to check on the internal properties of the object after getSessionSigs is called on the client which will perform the needed checks similar to validateAuthContext but is specific to the checks needed after we generate a new session instance.

Ansonhkg · 2024-04-29T17:49:25Z

packages/pkp-base/src/lib/pkp-base.ts

 }
 }

+ public async authenticate(): Promise<void> {


copy/paste this if you need:

/** * Authenticates the user - generate sessionSigs from litNodeClient(authContext.client) is provided. Otherwise, use * controllerSessionSigs. * * @returns A Promise that resolves to void. * @throws An error if `controllerSessionSigs` is not found. */

Ansonhkg · 2024-04-30T13:38:16Z

packages/pkp-base/src/lib/pkp-base.ts

+
+ if (providedAuthentications !== 2) {
+ // log which authentications has the user provided
+ if (this.controllerAuthSig) {


Note for myself (@Ansonhkg): remove this for 0.1 release https://linear.app/litprotocol/issue/LIT-3032/[auth-unification]-remove-all-v0-stuff-before-v1-release

MaximusHaximus

Added some questions in-line -- I'm still not sure about what the golden path is for using this API and I think there are some issues w/ error handling. Happy to huddle if you'd like to work it out sync :)

MaximusHaximus · 2024-05-01T16:28:17Z

packages/pkp-base/src/lib/pkp-base.ts

+ if (this.authContext) {
+ // It must have a valid client and getSessionSigsProps
+ if (
+ !(this.authContext.client instanceof LitNodeClientNodeJs) ||


instanceof is less than ideal way to check for validity -- we should avoid it whenever possible, especially given that we're leveraging typescript with interfaces. Consider that if someone passes us something other than a LitNodeClientNodeJs instance, but that thing still conforms to the LitClientSessionManager interface, which the type of this.authMethod expects to see as the client property, then this check will inaccurately throw an error even though it might be a usable implementation.

Also, if we disentangle LitNodeClient from LitNodeClientNodeJs in the future, or define a LitClientSessionManager object that isn't a full fledged LitNodeClientNodeJs instance, then this will be problematic since it might be an instance of multiple implementations of the client.

I think it's safest (and quite reasonable from a usability perspective) just to ducktype check that it appears to be what we expect, using e.g. this.authContext.client && this.authContext.client.getSessionSigsProps

MaximusHaximus · 2024-05-01T16:33:56Z

packages/pkp-base/src/lib/pkp-base.ts

+ this.controllerSessionSigs,
+ ].filter(Boolean).length;
+
+ if (providedAuthentications !== 2) {


I'm confused -- why is 'any 2' the magic numberof authentications that passes this if check and avoids the throwError() call on L282?

Is it really ok if any combination of two of these is passed in? Why is any 2 OK? A comment here explaining why it's ok if any 2 of 3 are defined, but if only 1 or if all 3 are defined it's a critical error

MaximusHaximus · 2024-05-01T16:39:47Z

packages/pkp-base/src/lib/pkp-base.ts

+ if (this.authContext) {
+ const controllerSessionSigs =
+ await this.authContext?.client?.getSessionSigs(
+ this.authContext.getSessionSigsProps


We try to use this.getSessionSigsProps before we actually validate that it exists inside of validateAuthenticate() (because we don't call it until after we call getSessionSigs() on the client) -- and that means that validateAuthenticate() will never throw an error in this case even if this.getSessionSigsProps is undefined, because calling getSessionSigs() with undefined will cause that function to throw a cannot get property 'sessionKey' of undefined

MaximusHaximus · 2024-05-01T16:45:55Z

packages/pkp-base/src/lib/pkp-base.ts

 }
 }

+ private validateAuthenticate() {


If we're only going to call it in one place and it's a private method, we may as well just in-line it where it needs to be until and if we need it in multiple places. Incidentally, this relates to my other comment below that we should be validating this.authContext.client and this.getSessionSigsProps before trying to use those properties, since if this wasn't a separate method you could easily validate those 2 things first and the authentications last.

MaximusHaximus · 2024-05-01T16:48:03Z

packages/pkp-base/src/lib/pkp-base.ts

+ * @throws An error if `controllerSessionSigs` is not found after session generation has resolved.
+ */
+ public async authenticate(): Promise<void> {
+ // If we dont have an authContext instance then we log that this is being depricated soon in favor of implicit session loading


Since this is a brand new method, and the only reason I can see that a consumer would ever call it is to run this.authContext.client.getSessionSigs(), can we just make it assume that the authContext functionality must be set, and must work rather than have it fall-through if it's not defined or the client isn't defined?

What is the use case for someone calling this new method without having those things defined, but being OK with the existing this.controllerSessionSigs value still remaining on the object, but only if there are exactly two out of 3 of this.controllerAuthSig, this.authContext and this.controllerSessionSigs defined, even if one of those isn't this.controllerSessionSigs?

joshLong145 · 2024-05-01T19:10:54Z

Closing as after some investigation with @MaximusHaximus It will be lowering the performance of the overall PKP-* implementations. Will move forward with the Alchemy upgrade with the current implementations using controllerSessionSigs

Josh Long added 3 commits April 29, 2024 11:32

feat: Add 'authenticate' method to pkp-base

27fcfd1

add test for authenticate

0592329

ref: fix validation for new authenticate impl

926b92b

joshLong145 requested a review from Ansonhkg as a code owner April 29, 2024 15:49

joshLong145 requested a review from MaximusHaximus April 29, 2024 16:19

Ansonhkg reviewed Apr 29, 2024

View reviewed changes

MaximusHaximus requested changes Apr 29, 2024

View reviewed changes

Ansonhkg reviewed Apr 29, 2024

View reviewed changes

Josh Long added 2 commits April 29, 2024 16:17

ref: use the same client instance in test case

64df73e

ref: review comments

4b65b4e

joshLong145 requested review from Ansonhkg and MaximusHaximus April 29, 2024 20:23

Ansonhkg approved these changes Apr 30, 2024

View reviewed changes

MaximusHaximus requested changes May 1, 2024

View reviewed changes

joshLong145 closed this May 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Feat/add authenticate method pkp base #447

Feat/add authenticate method pkp base #447

Uh oh!

joshLong145 commented Apr 29, 2024 •

edited

Loading

Ansonhkg Apr 29, 2024

joshLong145 Apr 29, 2024

MaximusHaximus left a comment

MaximusHaximus Apr 29, 2024

joshLong145 Apr 29, 2024

MaximusHaximus Apr 29, 2024

joshLong145 Apr 29, 2024

MaximusHaximus Apr 29, 2024

joshLong145 Apr 29, 2024 •

edited

Loading

joshLong145 Apr 29, 2024

Ansonhkg Apr 29, 2024

Ansonhkg Apr 30, 2024

MaximusHaximus left a comment

MaximusHaximus May 1, 2024

MaximusHaximus May 1, 2024

MaximusHaximus May 1, 2024

MaximusHaximus May 1, 2024

MaximusHaximus May 1, 2024

joshLong145 commented May 1, 2024

Labels

4 participants

Feat/add authenticate method pkp base #447

Feat/add authenticate method pkp base #447

Uh oh!

Conversation

joshLong145 commented Apr 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

How Has This Been Tested?

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaximusHaximus left a 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

Choose a reason for hiding this comment

joshLong145 Apr 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

MaximusHaximus left a 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

Choose a reason for hiding this comment

joshLong145 commented May 1, 2024

Labels

4 participants

joshLong145 commented Apr 29, 2024 •

edited

Loading

joshLong145 Apr 29, 2024 •

edited

Loading