continuous changes feed fails if there is an admin account

[gr2m]

I tried to figure out why that is, but had no luck. Here's a test script to reproduce the issue: https://github.com/gr2m/pouchdb-server-continuous-feed-debug

Would appreciate some help with this <3

1. Install pouchdb-server: npm install -g pouchdb-server
2. Start pouchdb-server: pouchdb-server -p 5985 --in-memory
3. open http://localhost:5985/_utils
4. Create database "test"
5. Fix admin party, create admin user, username / password don't matter
6. Sign out
7. Open http://localhost:5985/test/_changes?since=0&feed=continuous&heartbeat=30000
8. Boom, you should see

TypeError: Object [object Promise] has no method 'on'
   at changes (/pouchdb-server-continuous-feed-debug/node_modules/pouchdb-server/node_modules/express-pouchdb/lib/routes/changes.js:43:35)
   at Layer.handle [as handle_request] (/pouchdb-server-continuous-feed-debug/node_modules/pouchdb-server/node_modules/express/lib/router/layer.js:82:5)
   at next (/pouchdb-server-continuous-feed-debug/node_modules/pouchdb-server/node_modules/express/lib/router/route.js:100:13)
   at Route.dispatch (/pouchdb-server-continuous-feed-debug/node_modules/pouchdb-server/node_modules/express/lib/router/route.js:81:3)
   at Layer.handle [as handle_request] (/pouchdb-server-continuous-feed-debug/node_modules/pouchdb-server/node_modules/express/lib/router/layer.js:82:5)
   at /pouchdb-server-continuous-feed-debug/node_modules/pouchdb-server/node_modules/express/lib/router/index.js:235:24
   at param (/pouchdb-server-continuous-feed-debug/node_modules/pouchdb-server/node_modules/express/lib/router/index.js:332:14)
   at param (/pouchdb-server-continuous-feed-debug/node_modules/pouchdb-server/node_modules/express/lib/router/index.js:348:14)
   at Function.proto.process_params (/pouchdb-server-continuous-feed-debug/node_modules/pouchdb-server/node_modules/express/lib/router/index.js:392:3)
   at /pouchdb-server-continuous-feed-debug/node_modules/pouchdb-server/node_modules/express/lib/router/index.js:229:12
   at Function.match_layer (/pouchdb-server-continuous-feed-debug/node_modules/pouchdb-server/node_modules/express/lib/router/index.js:296:3)
   at next (/pouchdb-server-continuous-feed-debug/node_modules/pouchdb-server/node_modules/express/lib/router/index.js:190:10)
   at /pouchdb-server-continuous-feed-debug/node_modules/pouchdb-server/node_modules/express/lib/router/index.js:192:16
   at Function.match_layer (/pouchdb-server-continuous-feed-debug/node_modules/pouchdb-server/node_modules/express/lib/router/index.js:296:3)
   at next (/pouchdb-server-continuous-feed-debug/node_modules/pouchdb-server/node_modules/express/lib/router/index.js:190:10)
   at /pouchdb-server-continuous-feed-debug/node_modules/pouchdb-server/node_modules/express/lib/router/index.js:192:16

[gr2m]

btw the --in-memory doesn't matter, just makes testing simpler.

[gr2m]

I think I found the cause of the issue. pouchdb-security is wrapping promises, so decorations like .on get lost. Here's the function in question (couldn't find the code on GitHub):

function securityWrapper(checkAllowed, original, args) {
  var userCtx = args.options.userCtx || {
    //Admin party!
    name: null,
    roles: ["_admin"]
  };
  if (userCtx.roles.indexOf("_admin") !== -1) {
    return original();
  }
  if (!checkAllowed) {
    return Promise.resolve().then(throw401);
  }
  return filledInSecurity(args)
    .then(function (security) {
      if (!checkAllowed(userCtx, security)) {
        throw401();
      }
    })
    .then(original);
}

As there is no admin party anymore, the userCtx.roles.indexOf becomes []. So the promise returned from original does not get returned, instead the one from filledInSecurity(args) does.

Changing the code to this below prevents exception:

var promise = filledInSecurity(args)
    .then(function (security) {
      if (!checkAllowed(userCtx, security)) {
        throw401();
      }
    })
    .then(original);

  promise.on = function() {
    console.log('check 1,2')
    return promise;
  }

  return promise;

Instead, you'll see check 1,2 in the logs :)

I guess it'd be save to just add an .on method to all promises wrapped by pouchdb-secruity?

[gr2m]

Here is a patch for pouchdb-security that fixes the problem: https://gist.github.com/gr2m/e9d39d92f72a6e580dcd/revisions

Is pouchdb-security security somewhere on GitHub so I can submit a pull request? /cc @marten-de-vries

[nolanlawson]

@gr2m I don't believe the fix is quite right. If the issue is that we are wrapping something that is supposed to be an EventEmitter, then shouldn't all events from the original object be forwarded to the new object? Or it should just extend EventEmitter. We've done this a few times, e.g. here and here.

[marten-de-vries]

@gr2m Duplicate of https://github.com/pouchdb/express-pouchdb/issues/202 . pouchdb-security's code is on Launchpad, not Github, because Python-PouchDB is and the tests are shared. I've been putting this off too long anyway, giving it a look now...

[gr2m]

for the time being, this fork includes the fix, if anyone else runs into this before it gets resolved: https://github.com/gr2m/pouchdb-server/

[nolanlawson]

As far as I can see, the only events that are emitted on the db object itself are 'created' and 'destroyed' as well as the undocumented 'change'/'delete'/'update'/'create' which I would like to remove (see https://github.com/pouchdb/pouchdb/pull/3740). It appears the second group isn't used anywhere in express-pouchdb or pouchdb, but the first group is indeed used by the changes listener.

So it looks like the fix is going to involve not only extending EventEmitter, but also making sure that the 'destroyed' and 'created' events are called correctly. @marten-de-vries you can check out test.events.js for how it should work. I admit it's a little bewildering, which is why I've been trying to trim it down to just two events.

Also note that these events should also be emitted by the db.constructor, whatever its constructor may be. Looks like in this case its constructor is just Promise (see here), which is not going to work...

[gr2m]

The issue here is about the feed object that db.changes() returns. express-pouchdb is using the change, error and complete events from it here: https://github.com/pouchdb/express-pouchdb/blob/master/lib/routes/changes.js

[nolanlawson]

Sorry, confused. :confused: In that case there should be no issue, right? db.changes() returns a totally separate object, which yes, is an event emitter.

[nolanlawson]

Oh, I see. Yeah, we are talking about the Changes object, not the PouchDB object. Gotcha.

[nolanlawson]

In that case we are going to need promise to be an EventEmitter that forwards all events from originalPromise, similar to what we did with sync() wrapping replicate(). I still think your fix @gr2m only solves the surface of the issue; it will never emit any change events to its listeners, which will probably mean that your continuous feed just never sends any updates.

[gr2m]

Yeah I agree there must be better ways to fix it, but it works, I checked it.

[marten-de-vries]

@gr2m's No, as far as I can see your solution is still incomplete:

• Even emitters have more methods than just .on()
• If anyone calls .on() after the promise resolved, it's going to be ignored.

As for a working solution, I think something like making the 'new' promise an event emitter too, then as soon as the 'original' result object is available, links .on() to the .emit() of the new promise object should work. That's not something I want to do for every single method that's wrapped though because of overhead, but it should be possible. Looking into that now.

[gr2m]

Yeah, I just fixed .on(), what else is being used in the context of PouchDB's .changes()? Once promise resolves, it gets set to what the original()s .on() method here: https://github.com/gr2m/pouchdb-security/blob/master/index.js#L74

[gr2m]

I've extended my test script: https://github.com/gr2m/pouchdb-server-continuous-feed-debug/blob/master/index.js

Both docs that get created show up in the log, when my pouchdb-server fork is used

[marten-de-vries]

@gr2m oh, right, I overlooked that. Other methods are documented here: https://nodejs.org/api/events.html#events_class_events_eventemitter . If your patch would include those, I guess it should work. I'm still trying the 'extend EventEmitter' way too, though, it might end up being less code to maintain in the long run.

[gr2m]

I see my patch as a temporary workaround, it works fine because .on() is the only method that is used on db.changes(), the othrs EventEmitter methods are not as far as I know. I think this is good enough as a temporary fix, would be great to have pouchdb-security on GitHub, and then make a pull request for farther discussion. Looking forward to your proper fix for that. Thanks all :)

[nolanlawson]

for (method in require('events').EventEmitter.prototype) {
  promise[method] = originalPromise[method].bind(originalPromise);
}

This looks like it should work. It binds all of: ['setMaxListeners', 'emit', 'addListener', 'on', 'once', 'removeListener', 'removeAllListeners', 'listeners']

[marten-de-vries]

@nolanlawson That doesn't work because originalPromise is only going to be defined after an async security lookup in the database. At that time, the user already wants to have a promise with a .on() method. So I'm going to continue with the 'create a new wrapper EventEmitter' approach.

[gr2m]

@nolanlawson what @marten-de-vries says. That's why I'm queuing the calls to .on until we have the promise from original(), see https://github.com/gr2m/pouchdb-security/blob/master/index.js#L81 and https://github.com/gr2m/pouchdb-security/blob/master/index.js#L70-L74

[marten-de-vries]

I just published a new patch version of pouchdb-security. It seems to have fixed the problem (tested with @gr2m's test setup. Very useful!). In the end I indeed used a new EventEmitter that is linked to the pouchdb one. I also abstracted that away into a new npm module, pouchdb-changeslike-wrapper, to make reuse possible in pouchdb-security-db which needed a similar fix.

Code here: https://bazaar.launchpad.net/~marten-de-vries/python-pouchdb/0.x/view/head:/js/pouchdb-changeslike-wrapper/index.js

[gr2m]

confirmed, looks good, thanks @marten-de-vries

[nolanlawson]

:dancer:

[denis-sokolov]

Works for me! Thank you.