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

Store proxy password and apikeys in native OS credential store #10044

Merged
merged 23 commits into from
Jul 1, 2023
Merged

Conversation

calixtus
Copy link
Member

@calixtus calixtus commented Jun 28, 2023

follow up to #9652
fixes #9652 (comment)
Fixes https://github.com/JabRef/jabref-issue-melting-pot/issues/106
depends on #10004

grafik

Mandatory checks

  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

@calixtus calixtus added preferences status: depends-on-external A bug or issue that depends on an update of an external library labels Jun 28, 2023
@koppor koppor removed the status: depends-on-external A bug or issue that depends on an update of an external library label Jun 28, 2023
@koppor
Copy link
Member

koppor commented Jun 28, 2023

On Windows 10, the Credential Manager displays it as follows:

image

Logitech uses / for separation:

image

Microsoft adds some key-value stuff, but I like the Logitech one better --> is it possible to replace the | with a /?

image

@calixtus
Copy link
Member Author

No, the pipe char is hardcoded in the java-keyring library.

@calixtus
Copy link
Member Author

Some implementation changes were discussed:
The API keys should always be stored in the keyring an not in the preferences anymore at all.
The proxy config should directly include the option to persist the password instead of an abstract general option to use the key store.

@calixtus calixtus added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jun 29, 2023
@koppor koppor added this to the v5.10 milestone Jul 1, 2023
Co-authored-by: Carl Christian Snethlage <[email protected]>
@koppor koppor merged commit 01c41c1 into main Jul 1, 2023
@koppor koppor deleted the keyring branch July 1, 2023 21:28
@credmond
Copy link
Contributor

Was this tested and confirmed using Ubuntu / GNOME? It doesn't work as Keyring.create() repeatedly returns null. I'd say this is OS specific, but do we understand where and when it won't work? When not possible, password saving probably shouldn't be provided as an option (as it's misleading behaviour). Current behaviour is to swallow exceptions, etc...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preferences status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants