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 crash when getting credentials from keyring #1815

Merged
merged 1 commit into from
Jul 19, 2023

Conversation

matrss
Copy link
Collaborator

@matrss matrss commented Jul 13, 2023

I ran into an issue when trying to connect to mscolab. See:
Screencast from 13.07.2023 14:03:03.webm

This bug was probably introduced in #1731.

This patch catches the exception instead of crashing the app. I am not sure what other side-effects this might have though.

Fixes #1816

@matrss matrss changed the base branch from develop to stable July 14, 2023 07:30
@matrss matrss force-pushed the fix-keyring-error branch from 5e97abd to 3a745fa Compare July 14, 2023 07:43
@matrss
Copy link
Collaborator Author

matrss commented Jul 14, 2023

After actually testing this on stable and not running into the issue I started a bisect and came to the conclusion that 600d769 introduced the issue, probably due to how it is using get_password_from_keyring. I'll re-target onto develop again, since on stable this issue does not happen.

@matrss matrss force-pushed the fix-keyring-error branch from 3a745fa to 37e5fb8 Compare July 14, 2023 08:07
@matrss matrss changed the base branch from stable to develop July 14, 2023 08:08
@matrss
Copy link
Collaborator Author

matrss commented Jul 14, 2023

After further examination it looks like this issue could not have been caught by any test case, since we test against a stub implementation of a keyring backend that does not show this behavior. Maybe this only happens with the gnome keyring I am using, and also works fine on KDE. I cannot test that right now.

Maybe we should just make this function more robust by catching any exception that might occur, since it would be a reasonable default to not provide stored credentials no matter what went wrong trying to get them?

Copy link
Member

@ReimarBauer ReimarBauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

verified that KDE had not run into this. But we know already that it does it differently. Making it more robust for others is a good point!

Thx

@ReimarBauer ReimarBauer merged commit 03750bb into Open-MSS:develop Jul 19, 2023
@matrss matrss deleted the fix-keyring-error branch February 16, 2024 09:51
@matrss matrss mentioned this pull request Jul 5, 2024
4 tasks
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.

msui fails to connect to keyring when trying to connect to a local mscolab server
2 participants