search

bobrippling / podsync

4 stars 1 forks source link

Feature: Dual Authorize #2

Closed luctius closed 1 year ago

luctius commented 1 year ago

Allows for both authorizing via cookies and via user:pass login on each of the end points.

I saw 2 options of implementing this; either via filters, or (probably) via normal methods in the 'then' closure. The latter should allow for the endpoints to keep their full path as one line.

I chose for the former however; since in my mind it makes for cleaner code.

Any comments?

luctius commented 1 year ago

Note that this should also fix #1 , as Kast seems to work without problem.

Also note that Cargo.lock has not been updated; it still contains a version 1.2 for podsync.

luctius commented 1 year ago

I'll take a better look at the return values later this week.

luctius commented 1 year ago

I encountered a situation where at first antenna pod would work without a problem, and with the new version of this PR it did not. podsyn was located behind an reverse proxy, and I traced it back to the removal of the cookie check at login_authorize.

Do you see any disadvantages of adding the cookie to that check? If not I would suggest adding it back.

luctius commented 1 year ago

Also, while Kast works, Antenna-Pod and Kasts do not share new podcast subscriptions.

Do you think this is related to the 'sync-devices' endpoint?

bobrippling commented 1 year ago

I encountered a situation where at first antenna pod would work without a problem, and with the new version of this PR it did not. podsyn was located behind an reverse proxy, and I traced it back to the removal of the cookie check at login_authorize.

Do you see any disadvantages of adding the cookie to that check? If not I would suggest adding it back.

Interesting, the only place AntennaPod sends up an auth header is for /login, and there might optionally be a cookie (which we ignore with the removal of the cookie check), so that would be expected to pass, but perhaps something in the auth interceptor is adding it to another endpoint. In which case yeah, let's add it back in.

Also, while Kast works, Antenna-Pod and Kasts do not share new podcast subscriptions.

Do you think this is related to the 'sync-devices' endpoint?

Very likely - while the episode endpoints are device-agnostic, the subscriptions endpoints aren't. I can take a look at the sync-devices endpoint a little later in the week if you don't get there first, but if that doesn't look like it'll work I've an idea or two about faking our way round it anyway.

Actually, for a quick check - do you see any hits on the sync-devices endpoint in your podsync logs at the moment?

luctius commented 1 year ago

Actually, for a quick check - do you see any hits on the sync-devices endpoint in your podsync logs at the moment?

Yes, I get a hit, right after login, when connecting with Kasts.

bobrippling commented 1 year ago

Ok - I'd like to merge this and then we can look at sync-devices separately, if you're happy to resolve the conflict?

luctius commented 1 year ago

Done

bobrippling commented 1 year ago

Thanks! Out of interest, do you have any steps to reproduce the login_authorize situation where it needed a cookie as well as the basic auth? I had a go at replicating it but didn't seem to flip the right switches.

bobrippling commented 1 year ago

Turns out kasts ignores errors on the sync-devices endpoint, which is why we won't have seen anything on the UI