Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Allow login for limited federation instances #764

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kescherCode
Copy link

Addresses #637

@kescherCode kescherCode force-pushed the fix/limited-federation-login branch 2 times, most recently from 49b6fa2 to 77950ec Compare January 3, 2024 10:32
@kescherCode

This comment was marked as resolved.

@kescherCode kescherCode marked this pull request as ready for review January 3, 2024 11:44
@grishka
Copy link
Member

grishka commented Jan 3, 2024

So you're finding an existing session for that instance, and then making a request under that session? That won't address the most popular case when you don't have any sessions on that instance to begin with.

@kescherCode
Copy link
Author

kescherCode commented Jan 3, 2024

Check https://cts.kescher.at/about to see why this isn't possible.

You can't see any info about a limited federation instance while not logged in (which is what I am assuming you mean with "the most popular case"). Since there will be an error anyway, "about the instance" can only ever be viewed while logged in to begin with.

@kescherCode
Copy link
Author

(previous force push was merely an indentation fix)

@grishka
Copy link
Member

grishka commented Jan 3, 2024

Still, how do you get that first session going? How common is it to have multiple accounts on the same server?

@kescherCode
Copy link
Author

The first session, you get going by logging in. That's what the first commit in this PR fixes. The second commit are fixes that only apply after logging in.

It doesn't matter how common multiple accounts on the same instance are, does it? I certainly do have that use-case, however.

If you're wondering why I'm just picking any account with a given domain: That's because picking the actual account it would apply to is pretty hard. If you want to, I could force the current account ID to be passed into all methods requiring it.

@grishka
Copy link
Member

grishka commented Jan 3, 2024

Sorry, I didn't notice the part where it shows a confirmation alert.

Also fetch instance info again if it's a placeholder instance (which is the case for newly added limited federation instances).

Fixes a so-far unreported issue for instances that require auth on /custom_emojis (the emoji list would remain empty).
@kescherCode
Copy link
Author

Any updates on merging this? The app literally crashes without this PR, so declaring an intent on eventually merging this (or not, you know, that's fine) would be neat.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants