Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Updating a _users doc with same password does not invalidate sessions for that user #268

Closed
gr2m opened this issue Nov 5, 2015 · 12 comments
Closed

Updating a _users doc with same password does not invalidate sessions for that user #268

gr2m opened this issue Nov 5, 2015 · 12 comments

Comments

@gr2m
Copy link
Contributor

gr2m commented Nov 5, 2015

I'm actually not sure if this is an express-pouchdb or a CouchDB bug, I'll file a bug with CouchDB, too.

To reproduce

  1. Create a users doc
  2. In a different browser, sign in with the new account
  3. Update the users doc with "password": "<current password here>"
  4. Reload the other browser, the user is still signed in in PouchDB server, but signed out when tested in CouchDB
@rnewson
Copy link

rnewson commented Nov 5, 2015

The salt value is included in the session cookie (and verified when querying), so that's by design. We want session cookies derived from the old password to be invalidated

@gr2m
Copy link
Contributor Author

gr2m commented Nov 5, 2015

@rnewson thanks a lot Robert! Could you point us to where it's implemented in CouchDB?

I think we need the same implementation here: https://github.com/pouchdb/express-pouchdb/blob/master/lib/routes/session.js#L18-L37

I don't know where the validation of the AuthSession cookie is currently happening in express-pouchdb. I guess we would need to adjust the implementation there as well. Can you point us to the right place for that @marten-de-vries

@marten-de-vries
Copy link
Member

It's happening here: https://github.com/pouchdb/express-pouchdb/blob/master/lib/routes/authentication.js#L91

Other relevant code is in https://github.com/pouchdb/pouchdb-auth/blob/master/index.js ; The good news is that the test suite for that module has been completely ported, making the chance of breaking something incidentally quite a bit smaller.

@nolanlawson
Copy link
Member

Just talked with @gr2m; seems the best fix for this would be to remove the session database entirely and move to a model more like CouchDB's; where the cookie hash is determined by a combination of the password hash and salt, and the salt changes every time you update the user document's password, which automatically invalidates the cookie, so there's no need for a separate session database.

@gr2m
Copy link
Contributor Author

gr2m commented Nov 21, 2015

@rnewson could you point us to where CouchDB is implementing the calculation of AuthSession?

@gr2m
Copy link
Contributor Author

gr2m commented Nov 21, 2015

Is it this one here: https://github.com/apache/couchdb-couch/blob/master/src/couch_httpd_auth.erl#L266-L271

@nolanlawson nolanlawson mentioned this issue Dec 12, 2015
Closed
@marten-de-vries
Copy link
Member

Sorry, only now catching up with this. It took me some reading through the CouchDB code to actually grasp what's going on, but I think their approach should be fine for pouchdb-auth too. It would require some small API changes (so be version 2.0), mostly in that the API will provide the initial session id ('cookie'), not the consumer. I'm planning to try to make a proof of concept tomorrow.

@gr2m
Copy link
Contributor Author

gr2m commented Dec 23, 2015

maybe this will help: https://github.com/hoodiehq/couchdb-calculate-session-id

@marten-de-vries
Copy link
Member

Thanks @gr2m, that certainly helped. Would you accept a PR that uses a bit more lightweight crypto implementation than browserify-crypto in the browser?

I have a first version of the 'new' pouchdb-auth that again passes most of the old test suite. It needs more testing first, but I hope to open a PR sometime soon.

@gr2m
Copy link
Contributor Author

gr2m commented Dec 25, 2015

nice!

Note that we landed a fix in https://github.com/hoodiehq/couchdb-calculate-session-id yesterday:
hoodiehq/couchdb-calculate-session-id#7

PR away!

I’d also like to change order of arguments, which would be a breaking change: hoodiehq/couchdb-calculate-session-id#6 – any thoughts on this?

marten-de-vries added a commit that referenced this issue Dec 25, 2015
@marten-de-vries
WIP - (#268) - pouchdb-auth update
ca5936c 
Switches from a session db to a cookie based session system, making user
document changes invalidate the session.
marten-de-vries added a commit to pouchdb/pouchdb-auth that referenced this issue Dec 25, 2015
@marten-de-vries
WIP - (pouchdb/express-pouchdb#268) - 2.0 (session db -> id switch)
532c5cd 
Instead of a session database, the plugin now uses session ids, much like
CouchDB. To take this into account, minor API changes have been made.
@marten-de-vries
Copy link
Member

Ok, so a first version is online. See #272 for the PR, and pouchdb/pouchdb-auth#4 for the real implementation changes. If anyone wants to look it over, I'd love the feedback. Even if it's just checking if pouchdb-auth's new API makes sense before releasing 2.0 (README.md has been updated). No hurry though, I want to sleep a bit on this myself before pushing. Also, performance still needs to be checked. It might have degraded significantly and that's bad as it's part of PouchDB's test suite.

marten-de-vries added a commit to pouchdb/pouchdb-auth that referenced this issue Dec 28, 2015
@marten-de-vries
WIP - (pouchdb/express-pouchdb#268) - 2.0 (session db -> id switch)
0917037 
Instead of a session database, the plugin now uses session ids, much like
CouchDB. To take this into account, minor API changes have been made.
marten-de-vries added a commit to pouchdb/pouchdb-auth that referenced this issue Dec 28, 2015
@marten-de-vries
WIP - (pouchdb/express-pouchdb#268) - 2.0 (session db -> id switch)
b2a6d87 
Instead of a session database, the plugin now uses session ids, much like
CouchDB. To take this into account, minor API changes have been made.
marten-de-vries added a commit to pouchdb/pouchdb-auth that referenced this issue Jan 14, 2016
@marten-de-vries
(pouchdb/express-pouchdb#268) - 2.0 (session db -> id switch)
47c2312 
Instead of a session database, the plugin now uses session ids, much like
CouchDB. To take this into account, minor API changes have been made.
marten-de-vries added a commit that referenced this issue Jan 15, 2016
@marten-de-vries
(#268) - pouchdb-auth update
d605920 
Switches from a session db to a cookie based session system, making user
document changes invalidate the session.
marten-de-vries added a commit that referenced this issue Jan 25, 2016
@marten-de-vries
(#268) - pouchdb-auth update
9f5a9a9 
Switches from a session db to a cookie based session system, making user
document changes invalidate the session.
nolanlawson pushed a commit that referenced this issue Jan 30, 2016
@marten-de-vries @nolanlawson
(#268) - pouchdb-auth update
29a3651 
Switches from a session db to a cookie based session system, making user
document changes invalidate the session.
nolanlawson pushed a commit that referenced this issue Feb 13, 2016
@marten-de-vries @nolanlawson
(#268) - pouchdb-auth update
ef495aa 
Switches from a session db to a cookie based session system, making user
document changes invalidate the session.
@nolanlawson
Copy link
Member

fixed by ef495aa, thanks @marten-de-vries !

gr2m pushed a commit to pouchdb/pouchdb-server that referenced this issue Jun 28, 2017
@marten-de-vries @gr2m
(pouchdb/express-pouchdb#268) - 2.0 (session db -> id switch)
a1984f0 
Instead of a session database, the plugin now uses session ids, much like
CouchDB. To take this into account, minor API changes have been made.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@gr2m @rnewson @nolanlawson @marten-de-vries