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

401 unauthorized firing 'complete' instead of 'error' or 'denied' #4276

Closed
colinskow opened this issue Sep 5, 2015 · 14 comments

Comments

@colinskow
Copy link
Member

commented Sep 5, 2015

I want my app to send the user back to the login screen when a live replication fails due to a 401 unauthorized response.

Based on your documentation: "In a live replication, only cancelling the replication should trigger this [complete] event." I would expect this to be handled by either 'error' or 'denied'.

With the 'complete' event, I have to iterate through the errors on both 'push' and 'pull' properties to tell if an unauthorized occurred... more complicated than it should be.

@daleharvey

This comment has been minimized.

Copy link
Member

commented Sep 5, 2015

Can you describe the situation a little more? I believe this may be due to the fact that replication can reject writes with a 401 and replication will carry on by design, people may have a validate_doc_update to do per document authorisation which will reject some writes but not others.

You mentioned the denied event, that is supposed to be fired in this situation are you not seeing it? a 'denied' event isnt a completion one and the complete event will still fire after denied does

@colinskow

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2015

As a security feature, when a user logs out I remove them from the _security doc of their database. This means that replication will fail with 401 unauthorized on any other sessions that user has opened (intended behavior). This sets off the 'complete' event and nothing else.

In contrast, a validate_doc_update failure should result in 403 forbidden and replication continues.

I am listening for the 'denied' event but it does not fire in the case of 401 unauthorized. Although I am using PouchDB 4.0.0 because 4.0.1 hasn't been firing events at all.

@daleharvey daleharvey added the bug label Sep 5, 2015
@colinskow

This comment has been minimized.

Copy link
Member Author

commented Sep 10, 2015

So I did a bit of further testing. When PouchDB live replication receives an unauthorized or forbidden error from the process of attempting to find or write a checkpoint (/_local) then replication is terminated and the complete event is fired. This seems like rational behavior because CouchDB doesn't like the credentials. Although based on your documentation, it should be firing the error event instead.

When a normal validate_doc_update fails with unauthorized or forbidden then the denied event correctly fires and replication continues.

@colinskow

This comment has been minimized.

Copy link
Member Author

commented Sep 11, 2015

By the way, here is the source code that I'm working on that sets up 3-way sync and tracks events...
https://github.com/colinskow/ng-pouch-mirror

@daleharvey

This comment has been minimized.

Copy link
Member

commented Sep 26, 2015

@colinskow It seems like you are wanting to get your bugs fixed fairly quickly and fixing them yourself, so thanks :), and leaving these here in case you are looking to take a look at this (or anyone else)

https://github.com/pouchdb/pouchdb/blob/master/tests/integration/test.retry.js#L543 Is a test that overrides the ajax handler so you can customise whatever you want to return, this should hopefully help in generating a test for this condition.

We dont trigger denied on checkpoint failures and seems like 50 / 50 on whether we should, but we definitely should be firing the error event if a replication fails due to a bunch of 401s, and I think thats the case that this bug should focus on. most of that code is handled @ https://github.com/pouchdb/pouchdb/blob/master/lib/replicate/replicate.js#L58

@daleharvey

This comment has been minimized.

Copy link
Member

commented Oct 3, 2015

So I wrote a test for this, however fixing it is tricky, we have some not so perfect state handling code in the replicator so I may need to do a few refactorings before getting to the fix

https://gist.github.com/daleharvey/09d273f5d8502c8aa37a

daleharvey added a commit that referenced this issue Oct 3, 2015
daleharvey added a commit that referenced this issue Oct 4, 2015
@nolanlawson

This comment has been minimized.

Copy link
Member

commented Oct 4, 2015

Leaving this open unless @daleharvey thinks 31cdcf8 solves the original issue.

@daleharvey

This comment has been minimized.

Copy link
Member

commented Oct 4, 2015

Yup this fixes the issue, previously complete was getting fired because we werent tracking the cancelled state properly, test ensures we do now

@daleharvey daleharvey closed this Oct 4, 2015
@srikanth235

This comment has been minimized.

Copy link

commented Apr 22, 2016

I just wanted to check whether this fix has gone into latest i.e. 5.3.2 release. I'm still seeing the same error in 5.3.2.

@nolanlawson nolanlawson reopened this Apr 23, 2016
@nolanlawson

This comment has been minimized.

Copy link
Member

commented Apr 23, 2016

Reopening since apparently 31cdcf8 doesn't solve the issue.

@daleharvey

This comment has been minimized.

Copy link
Member

commented Jun 2, 2016

@srikanth235 do you have any further information on the issue you are seeing? we identified the bug and landed the fix with a passing test so if you are seeing something related broken it would help to know exactly what that was. Thanks

@coderroggie

This comment has been minimized.

Copy link

commented Jun 17, 2016

So, I guess I'm not clear as to what should happen when my session with couchdb expires as it relates to a live sync that is currently happening. Should that only fire the "complete" event or should it also fire an "error" event? I'm not sure that the docs are completely clear there. Or I missed something in the docs...

@colinskow

This comment has been minimized.

Copy link
Member Author

commented Jun 18, 2016

@coderroggie 401/unauthorized should fire the "denied" event.

@daleharvey

This comment has been minimized.

Copy link
Member

commented Jun 19, 2016

Closing as a dupe of #5172

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.