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

Make the services connection errors consistent #5406

Open
strycore opened this issue Apr 7, 2024 · 7 comments
Open

Make the services connection errors consistent #5406

strycore opened this issue Apr 7, 2024 · 7 comments

Comments

@strycore
Copy link
Member

strycore commented Apr 7, 2024

The way our integrations deal with being logged out / expired token / invalid credentials is a complete mess. No 2 services behave the same and I seem to remember only one or 2 happen to do the right thing.

Here's a bunch of stuff we're doing wrong

  • Lutris considers the user logged in to a service when the Service connect dialog is opened. There is no check to whether the user is actually logged in.
  • Lutris displays cryptic error messages the user does not care about and doesn't want to see. They just want to log in.
  • The buttons on the sidebar are closer to quantum mechanics than proper UX. You never know if you're logged or not until you have clicked the button.
  • If you have clicked the button and your token is no longer valid, well good luck with what happens next. Lutris will probably force you to log out and log back in. I think there's one integration that is a bit smarter than the others and manages to show a login dialog without being all dramatic.

Anyway, this area of lutris has been chaotic, ugly, nondeterministic and offensive. We all share the blame for letting that happen.

danieljohnson2 added a commit that referenced this issue May 4, 2024
… log you out and back in; this will ask for new credentials and then reload anyway.

Should help with #5406
danieljohnson2 added a commit that referenced this issue May 4, 2024
…in with WebConnectDialog; is_authenticated() is then false when this file is present, so even though we have cookies at this time, Lutris won't think we're already authenticated.

This means if you just close out of the WebConnectDialog, you are not considered to be logged in anyway- this extra file remains in that case.

Should help with #5406
@danieljohnson2
Copy link
Contributor

I see room for improvement here, so I've made the 'Refresh' button log you back in if you are not found to be connected, which will ask you for a password, then reload anyway. Saves you the click you were almost certainly going to make.

I've also added an interlock so if the login process is abandoned, even with cookies remaining, we don't consider that a successful login anymore. You've got to reach login_callback() or it won't count.

But I feel like there's more wanted here, and it's not clear to me. Do we want to preemptively check each source at some point to detect if you have been disconnected? I don't think we want to just poll all the sources all the time.

@strycore
Copy link
Member Author

strycore commented May 4, 2024

No polling needed, I would just like services to stop showing "token expired" errors and just show the login window instead.

danieljohnson2 added a commit that referenced this issue May 19, 2024
… logs back in; that's the handling we do on reload, and it should be appropriate for failures at other times.

Should help with #5406
@danieljohnson2
Copy link
Contributor

Hmm. I don't have the ability to really test this very well, but I think we can get closer here.

I've changed Lutris to just use one AuthenticationError class, and to have a backstop that will log out and log in again if such an error hits the backstop. That should help.

There are services that never raise this error. Origin for instance. So maybe we aren't done here.

@strycore
Copy link
Member Author

I don't have the ability to really test

Well that should make you life easier. If you can't test then you can't change.

Origin for instance

Origin can be removed.

@danieljohnson2
Copy link
Contributor

I tested this by sabotaging it- deleting credential files while Lutris was running. It's not exactly the same as being expired by the service, but I managed to run the offending code path.

I can just revert it all if you feel that's not good enough. I dunno how you're going to test this, but I'm sure you'll figure something out.

@strycore
Copy link
Member Author

I can test because I have accounts on all services we propose but yes, it is annoying to reproduce.

@danieljohnson2
Copy link
Contributor

I'll take that as "revert it all and leave it to you."

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

No branches or pull requests

2 participants