continuous changes feed fails if there is an admin account about pouchdb-server HOT 24 CLOSED

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

from pouchdb-server.

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?

from pouchdb-server.

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

from pouchdb-server.

@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.

from pouchdb-server.

@gr2m Duplicate of pouchdb/express-pouchdb#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...

from pouchdb-server.

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/

from pouchdb-server.

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 pouchdb/pouchdb#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...

from pouchdb-server.

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

from pouchdb-server.

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

from pouchdb-server.

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

from pouchdb-server.

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.

from pouchdb-server.

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

from pouchdb-server.

@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.

from pouchdb-server.

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

from pouchdb-server.

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

from pouchdb-server.

@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.

from pouchdb-server.

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 :)

from pouchdb-server.

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']

from pouchdb-server.

@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.

from pouchdb-server.

@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

from pouchdb-server.

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

from pouchdb-server.

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

from pouchdb-server.

💃

from pouchdb-server.

Works for me! Thank you.

from pouchdb-server.