Lsat.fromMacaroon not handling v2 macaroons correctly, only checks identifier from i and not from both i and i64

[mdedys]

Working with Lsat.fromToken the identifier doesn't get set correctly if it is not using utf8

I noticed when using Lsat.fromChallenge (specifically Line 414) after importing the macaroon the identifier is set based on .i or .i64.

In the code from Lsat.fromToken, it calls Lsat.fromMacaroon which then imports the macaroon only grabs the .i identifier.

Line 307

  const identifier = Macaroon.importMacaroon(macaroon)._exportAsJSONObjectV2().i
    let id: Identifier
    try {
      if (identifier == undefined) {
        throw new Error(
            `macaroon identifier undefined`
        )
      }
      id = Identifier.fromString(identifier)
    } catch (e) {
      throw new Error(
        `Unexpected encoding for macaroon identifier: ${e.message}`
      )
    }

To support both v1 macaroons and v2 macaroons I am proposing the following:

   // identifier.ts

   static fromBase64(str: string): Identifier {
     return new this().fromBase64(str)
   }
   
   // lsat.ts
  static getIdentifierFromMacaroon(macaroon: string): Identifier | null {
    const { i, i64 } = Macaroon.importMacaroon(macaroon)._exportAsJSONObjectV2();
    if (i) {
        return Identifier.fromString(i)
    }
    if (i64) {
      return Identifier.fromBase64(i64)
    }
    return null
  }

  static fromMacaroon(macaroon: string, invoice?: string): Lsat {
    /* existing code */

    let id;
    try {
      id = Lsat.getIdentifierFromMacaroon(macaroon);
      if (!id) {
          throw new Error(`macaroon identifier undefined`);
      }
    } catch (e) {
      throw new Error(
        `Unexpected encoding for macaroon identifier: ${e.message}`
      )
    }
   
    /* existing code */
  } 
  

[bucko13]

Hey @mdedys,

Im so sorry I didn’t see this sooner. Im setup to watch this repo but for some reason never got an alert about your post. I think this seems like a reasonable proposal and I’d be happy to checkout a PR with your proposed changes and corresponding tests if you’re still interested

[mdedys]

Looks like you took care of this here: #15. Thanks @bucko13

[bucko13]

Sweet. Yeah I was pretty sure that was caught. Good news is I’m getting notifications again in case you’ve got anymore in the future!