Skip to content

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

Merged
koppor merged 23 commits into
mainfrom
keyring
Jul 1, 2023
Merged

Store proxy password and apikeys in native OS credential store#10044
koppor merged 23 commits into
mainfrom
keyring

Conversation

@calixtus

@calixtus calixtus commented Jun 28, 2023

Copy link
Copy Markdown
Member

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 component: 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

koppor commented Jun 28, 2023

Copy link
Copy Markdown
Member

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
Copy Markdown
Member Author

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

@calixtus

Copy link
Copy Markdown
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 <50491877+calixtus@users.noreply.github.com>
Comment thread src/main/java/org/jabref/gui/JabRefGUI.java Outdated
@koppor koppor merged commit 01c41c1 into main Jul 1, 2023
@koppor koppor deleted the keyring branch July 1, 2023 21:28
@credmond

Copy link
Copy Markdown
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

component: 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