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

Pusher does not report "Forbidden" errors #756

Closed
pkramerzoll opened this issue Oct 6, 2016 · 5 comments
Closed

Pusher does not report "Forbidden" errors #756

pkramerzoll opened this issue Oct 6, 2016 · 5 comments
Assignees
Labels
Milestone

Comments

@pkramerzoll
Copy link

pkramerzoll commented Oct 6, 2016

When the Sync Gateway's Sync Function rejects a document, such as when a user does not have the appropriate role to modify the document, the recommended practice is to throw a "forbidden" exception to reject the document update.

However, the Pusher class in the CBL library does not treat ""Forbidden" errors received from the server as a failure, nor does it set the last error to something that can be handled by the replication changed event.

if (status.Code != StatusCode.Forbidden)

As a consumer of the replication class, it would be very useful to be able to handle these forbidden exceptions being thrown by the Sync Gateway. Otherwise, there is no indication of any kind of failure.

@borrrden
Copy link
Member

borrrden commented Oct 6, 2016

Our sprint plan is tomorrow, I will tag this for review since it is a cross platform change.

@borrrden borrrden added the bug label Oct 8, 2016
@borrrden borrrden added this to the 1.5 milestone Oct 8, 2016
@djpongh djpongh removed this from the 1.5.0 milestone Nov 17, 2016
@borrrden
Copy link
Member

borrrden commented Nov 18, 2016

Note: final decision is that these errors should cause the document to be skipped

@pkramerzoll
Copy link
Author

Will an exception be bubbled up that can be handled by the push replication for these documents?

@borrrden
Copy link
Member

@snej Should this action trigger a change to LastError of the replicator (or rather, would there be any issue with it) before skipping?

@borrrden borrrden added ready and removed backlog labels Nov 19, 2016
@borrrden borrrden added this to the 1.4.0 milestone Nov 19, 2016
@borrrden borrrden self-assigned this Nov 19, 2016
@borrrden
Copy link
Member

Ok, there wasn't that much to do on this. It turns out it was just incomplete (only handled in _bulk_docs). So I put the work into issue/#756. If you can build and try it out, then I will merge it to master. Otherwise I will confirm some scenarios next week.

@djpongh djpongh added ready and removed in progress labels Dec 13, 2016
@borrrden borrrden mentioned this issue Dec 21, 2016
@borrrden borrrden added review and removed ready labels Dec 21, 2016
borrrden added a commit that referenced this issue Dec 21, 2016
* Make sure that every place a document could be rejected by the endpoint is logged and stored in LastError
* Add in a new test for 403 errors
@borrrden borrrden removed the review label Dec 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants