Drop cookie if we get a 401 after authenticating using one. #280
Conversation
colinnewell
force-pushed
the
cookie-sync-error
branch
from
1d67e19
to
83fbaf6
Compare
That way we shouldn't end up with a situation where the cookie is set but invalid for too long. In theory a user can retry on 401. We avoid doing this as part of the RoundTripper interface because that would require storing the body. And we'd need to do that in anticipation of a failure, which is too much of a burden just in case we might fail.
colinnewell
force-pushed
the
cookie-sync-error
branch
from
83fbaf6
to
2708281
Compare
chttp/cookieauth.go
Outdated
| if res != nil && res.StatusCode == http.StatusUnauthorized { | ||
| if a.Cookie() != nil { | ||
| a.client.Jar = nil | ||
| a.setCookieJar() |
There was a problem hiding this comment.
Nuking the entire cookie jar feels a bit heavy-handed. On the other hand, I don't see any obvoius way to delete a specific cookie. 🤔 Perhaps setting it to a value that's already expired would work.
In general, it probably never matters, but I suppose it's possible that with the use of proxy servers or URL redirection, there might be some other cookies stored in there, too, in some cases...
There was a problem hiding this comment.
I agree, I can experiment with deleting the single cookie as it probably is possible as you suggested.
There was a problem hiding this comment.
This should be dealt with now.
|
I have added a test, and I've made the cookie dropping less brutal. Ideally I'd like to test that the less brutal cookie dropping is effective (in not dropping other cookies), but in principle this is good enough to review. |
Note that this exposes an interesting property of the auth mechanism. On the first request after auth, if extra cookies were sent, they won't be sent on that request. They will only be applied to following requests. This is due to the fact that the request has been 'baked' before the auth request/response and we only copy over the auth cookie onto that request. This test captures that (and has a comment to note that behaviour). That's existing behaviour, and at this point I figure it's just worth noting.
|
Okay, added the extra test. In doing so I realised an interesting property of the way the auth is done. Any other cookies sent on the log in response won't get sent to the server until the following request (to the one that triggers the first auth). This is existing behaviour, and I'm not sure anything needs to be done, it just made me scratch my head as I wrote the tests. I added a comment in the test to explain that. |
|
It occurred to me today that it's probably not correct to clear a cookie when we receive a 401, becuase a 401 may happen due to permission restrictions unrelated to an invalid cookie. I think this means that the only reasonable way to address this issue by exposing the flexiblity for the consumer of kivik to clear a cookie if they want to. What do you think? |
|
I guess that's possible. Perhaps it would be possible to make it optional? In my situation, there is nothing else that could affect the situation, so adding on the extra call to reset the auth seems like a faff. So ideally you'd having a mechanism for resetting the cookie, but also an optional (not sure whether to say on or off by default) automatic clearing of the cookie due to 401. Assuming your library automatically cleared the cookie on 401, and the 401 wasn't because the auth cookie was invalid, would that be a bad thing? In that situation, it would trigger an extra call to /_session that would probably also 401 the next time you try to make a call. Note that at this level I'm talking about something very specific to cookie or 'token' based auth where the credentials and the token being used are distinct things. We're currently talking about the token becoming invalid, as opposed to the credentials becoming invalid. |
|
@flimzy what can we do to unblock this? While I'm generally okay with the idea of exposing more control over the authentication, I'm struggling to believe that removing the cookie in this situation is a bad thing. At worst we remove a valid session cookie, and trigger an extra log in attempt the next time a connection attempt is made assuming the source of the 401 has been dealt with. |
|
Thank you for the reminder. I'm not exactly comfortable clearing the 401 unconditionally, since I can imagine this could be very detrimental to some types of applications, and considering that this may well be a bug in CouchDB that needs to be addressed. I'd rather have an opt-in (or at minimum opt-out) work-around. To accomodate that, and hopefully unblock you for now, I've put a fork of the CookieAuth mechanism in my I would consider applying it to this main library if we come up with a reasonable opt in/out mechanism. I'd also like to see a bit more debugging into the actual cause, and maybe we can raise an issue with CouchDB and get their feedback. But I don't know if you have the time or inclination to do that debugging. How do you feel about this? |
|
Making this opt in sounds like a reasonable compromise to me. I can take a look at how to do that at some point soon hopefully. I'm not convinced this is a bug in couchdb, more simply something that can happen. I'm struggling to think of how couchdb would guard against this. It's indicated that the credential doesn't work, there isn't a whole lot more it can do. |
|
I think I've come around on this. I think I was conflating 401 and 403 in my reasoning. I partly blame the fact that I haven't really been able to give this the proper attention it deserves since you've brought it up. A 401 should always mean a bad cookie, as you've said, so it should be safe to reset it. |
That way we shouldn't end up with a situation where the cookie is set
but invalid for long.
This does feel like it might be an abuse of the http.RoundTripper
interface?
This is the start of looking at how to solve go-kivik/kivik#539.
This needs tests and testing. I'm making it visible now to allow discussion.