Allow passing auth via header or cookie #3725
Conversation
|
Its working now. This means we need to do another change on all api endpoints. So far I only converted a couple to make the review easier, others can be done in separate PRs. Edit: Im also setting a cache-control header in the session middleware now, depending if the user is authenticated or not. Its so easy to handle this way :) |
|
What do you think about removing the existing |
I know historically I have been very pro about leaving the auth in the API requests: because then we can strongly type which API requests require auth, and which ones are optional. This was necessary especially when we were using websockets too, bc then both types could handle auth in the same way. But I suppose I'm open to getting rid of it, and only using cookie-based auth. Its not as easy for app devs, because they have no idea which routes require auth, but I also realize that using cookie / header auth is the standard, and it will simplify the code a lot. So if others are fine with getting rid of it, I spose I am too. |
|
Im not sure why a client would have to know if auth is required or not. Either the user is logged in, then you send the token. Or he is not, then you dont send it. Thats how most apis work afaik. In any case this auth via header/cookie will be available from the next major version (0.19). My question is if 0.19 should also support the current auth method via parameter. Then we would have to keep that logic around for backwards compatibility until 0.20. Or we make a hard break with 0.19 and switch entirely to header/cookie based auth. |
|
Hard break IMO, otherwise backwards-compatibility hacks will keep piling on top of each other and make the codebase completely unmaintainable. Plus in a lot of cases (like removing a column, or changing a data structure), there isn't a clean way to do backwards compatibility anyway. Since there are a ton of client apps, we should give plenty of advance notice, once we finalize all the breaking changes. |
|
I kinda disagree. Most clients/apps need to be able to handle multiple versions of lemmy, because even well-maintained lemmy instances may lag behind weeks of new versions. If there's a hard break in one version, then the client would need to add logic to determine the server version and based on that send different types of requests. This is a ton of work, and the apps will just be broken while that's being implemented, and afterwards broken for old instances. If there's backwards compatibility for maybe ~a month of updates, then the client can simply switch to the new system only after a reasonable amount of instances have upgraded and not need to maintain server version checks. Or from a different perspective: breaking all third-party apps for "no good reason" would be bad for lemmys reputation. |
Lemmy should either start supporting two API versions with different endpoints concurrently (proper deprecation), or it will break API on update. Either way, a proper and well-maintained client will have to "add logic to determine the server version and based on that send different types of requests", where version is just a Lemmy release, or an unauthorized non-versioned API call to get a list of supported API versions and endpoints.
Luckily, Lemmy apps are still in very active development. And the best and feasible way to do this is a longer RC period, where an announcement of upcoming breaking changes is made, then a small-but-not-too-small instance opts to be the first to bite the bullet and start running an early RC version (with prior announcement), providing real-world test bed for developers and users alike, and letting early app bug reports to start flowing in ;) After that, a large-but-not-too-large instance shall elect to be the first to upgrade to the last RC (which would usually be just a release), to make sure app developers are taking notice and adding support for v0.19 while still keeping v0.18 instances working. This should last 10-14 days, and if no major bugs are found, v0.19 proper can be released. That way, the "broken while that's being implemented" should only affect a small number of users. And the "broken for old instances" shouldn't happen with any well-maintained app.
But clients SHOULD "maintain server version checks", and this will actually be a good opportunity to force them!
Indeed, and the earlier clients are forced to learn how to handle such breakages, the better. The worst thing to do would be to keep backward-compatibility, without providing a separate new API version and endpoint (which I'm assuming is not on the cards). Such backwards-compatibility will only serve to give extra incentive for app developers to procrastinate adding support for the new auth method, prioritizing all the visible features they and their users crave. And by the time Lemmy v0.20 is out, maybe their apps will be more complete, and their energy and excitement for developing for Lemmy has already waned. |
| }; | ||
| res | ||
| .headers_mut() | ||
| .insert(CACHE_CONTROL, HeaderValue::from_static(cache_value)); |
There was a problem hiding this comment.
Actually the cache-control wont work correctly as long as we support params like GetComments.auth. Problem is that the middleware is not aware of them at all, so it would send cache-control: public header. This means that private api responses can be cached and served to other users.
So if we want cache-control, we need to get rid of those params. I think its a good enough reason for a breaking change.
|
In this case clients can be compatible with both the old and new version at the same time, without any version checks. They only need to send the existing auth params (eg
Good point, we should definitely make a post some weeks before the major release to explain what needs to be changed by client devs. Including explanation how to stay compatible with old and new versions at the same time. |
|
That's correct for things like this, but think about others like @phiresky 's fix for adding proper timezones to every date column, or especially, for something like a removed DB column that affects a view coming back in the API. There isn't always going to be a clean way to add a hack to codebase to support both types, even if we were to serve them at separate endpoint headings (IE As long as we give a lot of ample testing time, and have ds9.lemmy.ml or voyager.lemmy.ml for app devs to test on after finalizing breaking changes, then at most app down-time would be 1 or two days while servers and apps push their updates. IMO until the lemmy API is consider finalized, and as long as we are still in rapid development, we should be okay with adding breaking changes. People shouldn't have enterprise-level expectations for community-developed software like lemmy that hasn't even reached a |
|
Failing toml format for some reason, run |
|
Was because of #3851 |
There was a problem hiding this comment.
I'd prefer the breaking change, IE removing auth from the structs, and simplifying this code a lot. But I'll go with the majority decision among @Nutomic , @SleeplessOne1917 , and @phiresky .
If we do decide to keep the auth, then at least add a few TODOs in the new code, warning that this is only to support the auth in structs.
|
@dessalines I think you misunderstood me, Im also arguing to get rid of the current auth params on api structs because its the only way we can send proper cache-control headers. However that change needs to be made in a later pr once lemmy-js-client is updated to send auth as header/cookie, otherwise the api tests wont pass. |
|
When I get back, I can regenerate the types for lemmy-js-client, make sure cookie auth is working, and I can make a new lemmy-js-client RC. |
|
@Nutomic I'm back, you can remove all the I'll then regenerate lemmy-js-client, get the API tests working, and then I can start working on lemmy-ui using cookie auth instead. |
|
I would prefer to merge this as is, and then make a separate PR to remove auth fields. No need to do everything at once. |
|
Very well, that's up to you. |
Only take auth via header or cookie. This requires a new version of lemmy-js-client for api tests to pass.
* Remove explicit auth params (ref #3725) Only take auth via header or cookie. This requires a new version of lemmy-js-client for api tests to pass. * rework api_crud * remove remaining auth params, move logic to session middleware * fmt, fix test * update js client * remove auth param from api tests * Pass auth as header * add ! * url vars, setHeader * cleanup * fmt * update * Updating for new lemmy-js-client. --------- Co-authored-by: Dessalines <tyhou13@gmx.com> Co-authored-by: Dessalines <dessalines@users.noreply.github.com>
Passing auth tokens using cookie or header is much easier as clients dont have to pass auth separately to the params of each api call. It also avoids problems with auth tokens being sent as part of the url which can get logged (#3499). Additionally, this way we can set proper cache-control headers with a middleware that checks if the user is logged in based on auth cookie/header.