search

zenhack / powerbox-http-proxy

Apache License 2.0
4 stars 3 forks source link

Might be too aggressive on deleting existing tokens #7

Closed ocdtrekkie closed 3 years ago

ocdtrekkie commented 3 years ago

Since the functionality to re-request has gone in, I've noticed I get a lot more Powerbox requests for feeds that I am confident I already granted permissions to. It's plausible that some of them have changed their http to https or vice versa, but it seems to happen more than that alone would justify.

My guess is that perhaps sometimes the proxy is deleting tokens that were still valid, and having to re-request them, but I don't see a clear pattern yet. I'd say more "personal blogs" than major websites end up doing this, so it may have to do with reliability or response time of the sites being connected to.

zenhack commented 3 years ago

Ack, thanks for the report.

The logic is definitely a bit pessimistic; it will try to fetch a new token basically whenever it gets a 500 response. The way the code is currently structured makes it hard to tell the difference between a genuine 500 error and the bridge responding with 500 because it couldn't restore the token.

We could try to refactor this to talk to the Sandstorm API and call restore() itself, and only try fetching a new token if that fails (otherwise assuming it's just a real 500 error).

Alternatively, it might be nice to make changes to sandstorm-http-bridge such that it's easy to tell the difference between these two scenarios, maybe by adding a header (one that's not on websession's whitelist) to the response in the case of an error during restore or such.

ocdtrekkie commented 3 years ago

It isn't, as far as I can tell, massively problematic, but I figured I should report it.

I guess the big question is, when it's not open on the UI, and just doing Cron jobs or mobile access, will it drop the existing token? Or will it only drop the token when it's possible for it to request a new one at the same time? Because of the possibility it can miss a single request, it probably shouldn't stop trying to use the existing token unless it's possible to ask the user for a new one.

zenhack commented 3 years ago

It should only delete the old token if fetching a new one succeeds, so if you're not around to approve it it should continue as before.

ocdtrekkie commented 3 years ago

So, I am still getting the impression on a failure, it drops the old token entirely. I had an Internet outage yesterday, and basically every feed stop working until I went in and debug-feed'd to force it to give me fresh Powerbox requests for nearly every feed.

zenhack commented 3 years ago

Looking again, you're right -- the logic in there does delete the token before trying to fetch a new one. I'll fix it soonish.

zenhack commented 3 years ago

@ocdtrekkie, I pushed a fix, and also pushed a new ttrss spk that includes it. Did some basic testing to make sure it doesn't break any existing functionality, but didn't go to the trouble of orchestrating a 500 while the browser is closed... but I think it's right.

ocdtrekkie commented 3 years ago

I'll try it out. There's a few feeds I find pretty unreliable that tend to trigger the issue regularly. If those stop I'll know you fixed it.

Thanks!

ocdtrekkie commented 3 years ago

It seems to be working quite well over the past day, and I've had no errant Powerbox requests. (I assume most of them come from single missed queries during maintenance windows or similar during the night.)