Skip to content

Remove explicit auth params#3946

Merged
dessalines merged 20 commits intomainfrom
remove-explicit-auth
Sep 21, 2023
Merged

Remove explicit auth params#3946
dessalines merged 20 commits intomainfrom
remove-explicit-auth

Conversation

@Nutomic
Copy link
Copy Markdown
Member

@Nutomic Nutomic commented Sep 6, 2023

Followup for #3725. Only take auth via header or cookie, not as query param. This requires a new version of lemmy-js-client for api tests to pass.

@Nutomic
Remove explicit auth params (ref #3725)
36c3b0f 
Only take auth via header or cookie. This requires a new version
of lemmy-js-client for api tests to pass.
@Nutomic
rework api_crud
0611f2c
@dessalines
Copy link
Copy Markdown
Member

dessalines commented Sep 6, 2023

I'll start doing a lemmy-js-client RC for this rn.

@dessalines
Copy link
Copy Markdown
Member

dessalines commented Sep 6, 2023

Okay that should be deployed soon, 0.19.0-rc.8

@Nutomic Nutomic force-pushed the remove-explicit-auth branch from e6cc584 to 6699829 Compare September 7, 2023 13:39
@Nutomic Nutomic marked this pull request as ready for review September 7, 2023 13:39
@Nutomic
Copy link
Copy Markdown
Member Author

Nutomic commented Sep 7, 2023

How to pass the auth to lemmy-js-client now? I think it should be held in LemmyHttp, but dont see anything in your PR.

@phiresky
Copy link
Copy Markdown
Collaborator

phiresky commented Sep 11, 2023

If I understand correctly and this is the part of the auth change that breaks backwards compat I'm still of the opinion this should only be done one version later (0.20) since there's not really a reason to do it now and that will mean apps won't have to implement version checking gymnastics and instantly break with 0.19.

@Nutomic
Copy link
Copy Markdown
Member Author

Nutomic commented Sep 12, 2023

Version checking is not necessary, clients can send both the old auth param (in query/post param) as well as the auth cookie/header. Then it will work with both Lemmy 0.18 and 0.19.

@dessalines
Copy link
Copy Markdown
Member

dessalines commented Sep 12, 2023

How to pass the auth to lemmy-js-client now? I think it should be held in LemmyHttp, but dont see anything in your PR.

You can pass in headers, so you'd pass in the Cookie as a header.

@biosfood biosfood mentioned this pull request Sep 14, 2023
Merged
@Nutomic
Copy link
Copy Markdown
Member Author

Nutomic commented Sep 18, 2023

Okay thats working, check the last commit. Its a bit awkward having to hardcode the Lemmy URL in many different places, would be good if LemmyHttp.baseUrl would be publicly readable. And would be good if auth header could be changed without having to reconstruct LemmyHttp.

@dessalines
Copy link
Copy Markdown
Member

dessalines commented Sep 18, 2023 •
edited
Loading

EDIT: okay I added a client.setHeaders function. lemmy-js-client: 0.19.0-rc.9

dessalines
dessalines approved these changes Sep 18, 2023
View reviewed changes
Comment thread api_tests/src/community.spec.ts Outdated
Comment thread api_tests/src/shared.ts Outdated
Comment thread crates/routes/src/images.rs
@Nutomic
Copy link
Copy Markdown
Member Author

Nutomic commented Sep 19, 2023

With lemmy-js-client: 0.19.0-rc.9 Im getting lots of errors about auth property missing from form types.

@dessalines
Copy link
Copy Markdown
Member

dessalines commented Sep 19, 2023

0.19.0-rc.10 should be done in a minute.

@Nutomic
Copy link
Copy Markdown
Member Author

Nutomic commented Sep 19, 2023

Alright its passing locally.

@Nutomic
Copy link
Copy Markdown
Member Author

Nutomic commented Sep 20, 2023

This needs another lemmy-js-client after #3869 is merged and I fixed conflicts. Pretty annoying to do this manually all the time. How about we move lemmy-js-client into this repo so it stays in sync with the Rust code automatically?

@dessalines
Copy link
Copy Markdown
Member

dessalines commented Sep 20, 2023 •
edited
Loading

#3970

Okay 0.19.0-rc.11 is deployed.

@Nutomic
Copy link
Copy Markdown
Member Author

Nutomic commented Sep 20, 2023

Still not working, BlockInstance form needs auth removed, and LemmyHttp.blockInstance() is missing.

@dessalines
Copy link
Copy Markdown
Member

dessalines commented Sep 20, 2023

Oops, forgot to add that.

@dessalines
Copy link
Copy Markdown
Member

dessalines commented Sep 20, 2023

Pushed up. Sorry for these.

@Nutomic
Copy link
Copy Markdown
Member Author

Nutomic commented Sep 20, 2023

Great, api tests are passing now so its good to merge.

@dessalines dessalines enabled auto-merge (squash) September 20, 2023 15:20
@SleeplessOne1917
Copy link
Copy Markdown
Contributor

SleeplessOne1917 commented Sep 20, 2023

@Nutomic @dessalines once this gets merged I can start working on the UI side of blocking instances.

@dessalines dessalines merged commit f858d8c into main Sep 21, 2023
@alexandrupero
Copy link
Copy Markdown
Contributor

alexandrupero commented Sep 22, 2023

Does the latest lemmy-ui (main branch) support this change?

@SleeplessOne1917
Copy link
Copy Markdown
Contributor

SleeplessOne1917 commented Sep 22, 2023

@alexandrupero not yet. It will soon though.

@Nutomic Nutomic mentioned this pull request Sep 28, 2023
Closed
4 tasks
@mormaer mormaer mentioned this pull request Sep 30, 2023
Closed
@NatoBoram NatoBoram mentioned this pull request Dec 9, 2023
Closed
@Nothing4You Nothing4You deleted the remove-explicit-auth branch September 11, 2025 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dessalines dessalines dessalines approved these changes

@phiresky phiresky Awaiting requested review from phiresky phiresky is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Nutomic @dessalines @phiresky @SleeplessOne1917 @alexandrupero