Improve error handling for window.ipfs

[lidel]

Question from IRC:

will the UX of if (err.message === 'User denied access to ipfs.files.add') { // Fallback be able to be improved?

For a better context, we currently suggest this in docs/window.ipfs.md:

if (window.ipfs) {  
  try {
    await ipfs.add(Buffer.from('=^.^='))
   } catch (err) {
     if (err.message === 'User denied access to ipfs.files.add') {
       // Fallback
     } else {
       throw err
     }
   }

   const data = await ipfs.cat('QmS5RzFV9v4fucVtjATPxoxABgEmNhhduykzUbQeGyyS3N')
   console.log(data.toString()) // =^.^=
} else {
  // Fallback
}

Initial ideas:

• Perhaps instead of strring we could return a generic error + the name of denied API in separate field? It would make it easier to handle programmaticallly.
• We could expose "generic error message" as an additional attribute of window.ipfs object, so that dapps do not need to hardcode any string.

[JonKrone]

I also noticed this dev difficulty when reading the window.ipfs docs, I agree we should provide some common Error type or utils to detect them.

const accessErrorSymbol = Symbol.for('IPFS Access Error') // or smth

const isAccessError = error => accessErrorSymbol in error

function AccessError(message, permission) {
  const error = new Error(message)
  error[accessErrorSymbol] = true
  error.permission = permission
  return error
}

// companion usage:
if (access popup is closed)
  throw new AccessError('Failed to obtain access response to ${permission} at ${scope}', permission)

// user-land:
try {
 const { CID } = await ipfs.files.add(buf)
 await ipfs.pubsub.publish(friendPool, CID)
} catch (err) {
  if (isAccessError(err) && err.permission.match(/pubsub/)) {
    return // non-critical error, continue user flow
  } else {
   // app-specific handling
  }
}

I think it's good practice for libraries to use Symbols to prevent naming collisions with consumers but we could alternatively do something else.

Because of the security concerns with revealing the availability of window.ipfs in #451, I think the User disabled access to IPFS error will no longer exist, right? We would only reveal window.ipfs if they have that permission turned on?

[lidel]

Hm.. how about something more explicit:

function IpfsApiAccessError(message, permission, scope) {
    this.name = 'IpfsApiAccessError';
    this.message = message;
    this.permission = permission;
    this.scope = scope; // for completeness
    this.stack = (new Error()).stack;
}
IpfsApiAccessError.prototype = new Error; // makes 'instanceof Error' work as well

It makes error-type-check less elaborate (instanceof instead of custom util):

} catch (err) {
  if (err instanceof IpfsApiAccessError && err.permission.match(/pubsub/)) {

Are there any red flags to this approach?

I think the User disabled access to IPFS error will no longer exist, right? We would only reveal window.ipfs if they have that permission turned on?

Yes, that is the plan, but we should tackle #451 in separate PR :)

[JonKrone]

Yep, I'm totally on board. Symbols don't provide much value over this approach and it's much simpler. How about we take it a step further and:

class IpfsApiAccessError extends Error {
  constructor (message, permission, scope) {
    super(message)
    this.permission = permission;
    this.scope = scope; // for completeness
  }
}

Extending Error gives us built-in name, stack, and message!

I'm happy to implement whatever we conclude here.

[lidel]

Sounds good, give it a try in a PR :)

[alanshaw]

Hey! Yes, we can do better with detecting this flavor of error. I'm sorry I didn't see this issue until after @JonKrone had submitted a PR.

There's a few things at play here:

An Error object is not something you can JSON.stringify (meaningfully) or something that the structured clone algorithm will clone. This means that passing errors over postMessage requires some manual serialization/deserialization.

FYI an ACL error is created "server side" so needs to be serialized to be passed back to the caller.

postmsg-rpc (the library behind ipfs-postmsg-proxy) handles this serialization here.

It is influenced by boom when it comes to errors and allows you to attach information to your error for consumption by the client by attaching it to a property output.payload.

So if on the server you were to create an error:

const err = new Error('User denied access to ipfs.files.add')
err.output = { payload: { isIpfsProxyAclError: true, scope, permission } }

i.e. here

Then on the client side you should be able to:

try {
  ipfs.files.add(/* ... */)
} catch (err) {
  if (err.isIpfsProxyAclError) {
    // Error was an IPFS Proxy ACL error
    console.log('Failed to access', err.scope, err.permission)
  }
}

For the sake of simplicity, I'd leave it as that (i.e. not implement a custom subclass).

[lidel]

err.isIpfsProxyAclError was shipped with latest beta (v2.2.2.9120).
Thanks @JonKrone !