Comments (24)
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.
from pouchdb-server.
confirmed, looks good, thanks @marten-de-vries
from pouchdb-server.
💃
from pouchdb-server.
Works for me! Thank you.
from pouchdb-server.
Related Issues (20)
- High vulnerabilities in pouchdb-fauxton used by pouchdb-server
- End of life for pouchdb-server ? HOT 3
- getting error You may need an appropriate loader to handle this file type in electron.js HOT 1
- Case-insensitive regex queries
- pouchdb-wrappers has no tests
- Introducing Express-PouchDB failed when the pouchdb-promise was introduced into the library
- Fauxton emulation does not show number of deleted documents
- pouchdb-server runs out of memory and gets killed by oom killer HOT 2
- purge not properly supported
- Provide TypeScript definitions
- Fixed express-pouchdb's replication not handling all forms of basic authentication
- Config couch_httpd_auth.timeout doesn't work as expected and throws unhelpful error
- 'DB open requires options argument with "dynamo" key' - when trying to use --level-backend level-dynamodb
- Please add support for expiring items! HOT 4
- Documentation for ETag header value
- Invalid String Length
- Can express-pouchdb run in Node's cluster mode?
- Is this repo dead? HOT 8
- allDocs() may return incorrect total_rows HOT 2
- Where is pouchdb-server storing data? Even custom data dir arg is not respected
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from pouchdb-server.