Store proxy password and apikeys in native OS credential store (JabRe…

…f#10044) Co-authored-by: Carl Christian Snethlage <50491877+calixtus@users.noreply.github.com>
koppor · Jul 1, 2023 · 01c41c1 · 01c41c1
1 parent 214f4e3
commit 01c41c1
Show file tree

Hide file tree

Showing 14 changed files with 259 additions and 78 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -36,6 +36,7 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve
 - We enabled the user to change the name of a field in a custom entry type by double-clicking on it. [#9840](https://github.com/JabRef/jabref/issues/9840)
 - We integrated two mail actions ("As Email" and "To Kindle") under a new "Send" option in the right-click & Tools menus. The Kindle option creates an email targeted to the user's Kindle email, which can be set in preferences under "External programs" [#6186](https://github.com/JabRef/jabref/issues/6186)
 - We added an option to clear recent libraries' history. [#10003](https://github.com/JabRef/jabref/issues/10003)
+- We added an option to encrypt and remember the proxy password. [#8055](https://github.com/JabRef/jabref/issues/8055)[#10044](https://github.com/JabRef/jabref/issues/10044)
 
 ### Changed
 
@@ -72,6 +73,7 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve
 - We improved the error message when no terminal was found [#9607](https://github.com/JabRef/jabref/issues/9607)
 - In the context of the "systematic literature functionality", we changed the name "database" to "catalog" to use a separate term for online catalogs in comparison to SQL databases. [#9951](https://github.com/JabRef/jabref/pull/9951)
 - We now show more fields (including Special Fields) in the dropdown selection for "Save sort order" in the library properties and for "Export sort order" in the preferences. [#10010](https://github.com/JabRef/jabref/issues/10010)
+- We now encrypt and store the custom API keys in the OS native credential store. [#10044](https://github.com/JabRef/jabref/issues/10044)
 
 ### Fixed
 
@@ -83,7 +85,7 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve
 - We fixed an issue where custom field in the custom entry types could not be set to mulitline. [#9609](https://github.com/JabRef/jabref/issues/9609)
 - We fixed an issue where the Office XML exporter did not resolve BibTeX-Strings when exporting entries. [forum#3741](https://discourse.jabref.org/t/exporting-bibtex-constant-strings-to-ms-office-2007-xml/3741)
 - We fixed an issue where the Merge Entries Toolbar configuration was not saved after hitting 'Merge Entries' button. [#9091](https://github.com/JabRef/jabref/issues/9091)
-- We fixed an issue where the password is saved locally if user wants to use proxy with authentication. [#8055](https://github.com/JabRef/jabref/issues/8055)
+- We fixed an issue where the password is stored in clear text if the user wants to use a proxy with authentication. [#8055](https://github.com/JabRef/jabref/issues/8055)
 - JabRef is now more relaxed when parsing field content: In case a field content ended with `\`, the combination `\}` was treated as plain `}`. [#9668](https://github.com/JabRef/jabref/issues/9668)
 - We resolved an issue that cut off the number of group entries when it exceeded four digits. [#8797](https://github.com/JabRef/jabref/issues/8797)
 - We fixed the issue where the size of the global search window was not retained after closing. [#9362](https://github.com/JabRef/jabref/issues/9362)

diff --git a/build.gradle b/build.gradle
@@ -142,6 +142,7 @@ dependencies {
 
     implementation 'io.github.java-diff-utils:java-diff-utils:4.12'
     implementation 'info.debatty:java-string-similarity:2.0.0'
+    implementation 'com.github.javakeyring:java-keyring:1.0.2'
 
     antlr4 'org.antlr:antlr4:4.13.0'
     implementation 'org.antlr:antlr4-runtime:4.13.0'

diff --git a/src/main/java/module-info.java b/src/main/java/module-info.java
@@ -99,6 +99,8 @@
 
     requires com.h2database.mvstore;
 
+    requires java.keyring;
+
     requires org.jooq.jool;
 
     // fulltext search

diff --git a/src/main/java/org/jabref/gui/JabRefGUI.java b/src/main/java/org/jabref/gui/JabRefGUI.java
@@ -4,6 +4,7 @@
 import java.sql.SQLException;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Optional;
 import java.util.stream.Collectors;
 
 import javafx.application.Platform;
@@ -25,6 +26,7 @@
 import org.jabref.logic.shared.exception.InvalidDBMSConnectionPropertiesException;
 import org.jabref.logic.shared.exception.NotASharedDatabaseException;
 import org.jabref.logic.util.WebViewStore;
+import org.jabref.model.strings.StringUtil;
 import org.jabref.model.util.FileUpdateMonitor;
 import org.jabref.preferences.GuiPreferences;
 import org.jabref.preferences.PreferencesService;
@@ -71,13 +73,32 @@ public JabRefGUI(Stage mainStage,
                 preferencesService.getInternalPreferences())
                 .checkForNewVersionDelayed();
 
-        if (preferencesService.getProxyPreferences().shouldUseProxy() && preferencesService.getProxyPreferences().shouldUseAuthentication()) {
-            DialogService dialogService = Injector.instantiateModelOrService(DialogService.class);
-            dialogService.showPasswordDialogAndWait(Localization.lang("Proxy configuration"), Localization.lang("Proxy requires password"), Localization.lang("Password"))
-                .ifPresent(newPassword -> {
-                    preferencesService.getProxyPreferences().setPassword(newPassword);
-                    ProxyRegisterer.register(preferencesService.getProxyPreferences());
-                });
+        setupProxy();
+    }
+
+    private void setupProxy() {
+        if (!preferencesService.getProxyPreferences().shouldUseProxy()
+                || !preferencesService.getProxyPreferences().shouldUseAuthentication()) {
+            return;
+        }
+
+        if (preferencesService.getProxyPreferences().shouldPersistPassword()
+                && StringUtil.isNotBlank(preferencesService.getProxyPreferences().getPassword())) {
+            ProxyRegisterer.register(preferencesService.getProxyPreferences());
+            return;
+        }
+
+        DialogService dialogService = Injector.instantiateModelOrService(DialogService.class);
+        Optional<String> password = dialogService.showPasswordDialogAndWait(
+                Localization.lang("Proxy configuration"),
+                Localization.lang("Proxy requires password"),
+                Localization.lang("Password"));
+
+        if (password.isPresent()) {
+            preferencesService.getProxyPreferences().setPassword(password.get());
+            ProxyRegisterer.register(preferencesService.getProxyPreferences());
+        } else {
+            LOGGER.warn("No proxy password specified");
         }
     }
 

diff --git a/src/main/java/org/jabref/gui/preferences/general/GeneralTab.fxml b/src/main/java/org/jabref/gui/preferences/general/GeneralTab.fxml
@@ -65,7 +65,6 @@
     <CheckBox fx:id="inspectionWarningDuplicate"
               text="%Warn about unresolved duplicates when closing inspection window"/>
     <CheckBox fx:id="confirmDelete" text="%Show confirmation dialog when deleting entries"/>
-
     <CheckBox fx:id="collectTelemetry" text="%Collect and share telemetry data to help improve JabRef"/>
 
     <Label styleClass="sectionHeader" text="%Libraries"/>

diff --git a/src/main/java/org/jabref/gui/preferences/network/NetworkTab.fxml b/src/main/java/org/jabref/gui/preferences/network/NetworkTab.fxml
@@ -50,7 +50,7 @@
         <TextField fx:id="proxyUsername" prefWidth="200.0" GridPane.columnIndex="1" GridPane.rowIndex="4" />
         <Label fx:id="proxyPasswordLabel" text="%Password" GridPane.rowIndex="5" />
         <CustomPasswordField fx:id="proxyPassword" prefWidth="200.0" GridPane.columnIndex="1" GridPane.rowIndex="5" />
-        <Label fx:id="proxyAttentionLabel" styleClass="info-message" text="%Password kept for this session only." GridPane.columnIndex="2" GridPane.rowIndex="5" />
+        <CheckBox fx:id="proxyPersistPassword" text="%Persist password between sessions" GridPane.columnIndex="2" GridPane.rowIndex="5"/>
         <Button fx:id="checkConnectionButton" onAction="#checkConnection" prefWidth="200.0" text="%Check connection" GridPane.columnIndex="1" GridPane.rowIndex="6" />
     </GridPane>
    <Label styleClass="sectionHeader" text="%SSL Configuration" />

diff --git a/src/main/java/org/jabref/gui/preferences/network/NetworkTab.java b/src/main/java/org/jabref/gui/preferences/network/NetworkTab.java
@@ -50,8 +50,8 @@ public class NetworkTab extends AbstractPreferenceTabView<NetworkTabViewModel> i
     @FXML private TextField proxyUsername;
     @FXML private Label proxyPasswordLabel;
     @FXML private CustomPasswordField proxyPassword;
-    @FXML private Label proxyAttentionLabel;
     @FXML private Button checkConnectionButton;
+    @FXML private CheckBox proxyPersistPassword;
 
     @FXML private TableView<CustomCertificateViewModel> customCertificatesTable;
     @FXML private TableColumn<CustomCertificateViewModel, String> certIssuer;
@@ -105,7 +105,8 @@ public void initialize() {
         proxyPasswordLabel.disableProperty().bind(proxyCustomAndAuthentication.not());
         proxyPassword.textProperty().bindBidirectional(viewModel.proxyPasswordProperty());
         proxyPassword.disableProperty().bind(proxyCustomAndAuthentication.not());
-        proxyAttentionLabel.disableProperty().bind(proxyCustomAndAuthentication.not());
+        proxyPersistPassword.selectedProperty().bindBidirectional(viewModel.proxyPersistPasswordProperty());
+        proxyPersistPassword.disableProperty().bind(proxyCustomAndAuthentication.not());
 
         proxyPassword.setRight(IconTheme.JabRefIcons.PASSWORD_REVEALED.getGraphicNode());
         proxyPassword.getRight().addEventFilter(MouseEvent.MOUSE_PRESSED, this::proxyPasswordReveal);

diff --git a/src/main/java/org/jabref/gui/preferences/network/NetworkTabViewModel.java b/src/main/java/org/jabref/gui/preferences/network/NetworkTabViewModel.java
@@ -4,7 +4,6 @@
 import java.nio.file.Path;
 import java.util.Collections;
 import java.util.List;
-import java.util.Objects;
 import java.util.Optional;
 import java.util.concurrent.atomic.AtomicBoolean;
 
@@ -43,12 +42,8 @@
 import de.saxsys.mvvmfx.utils.validation.ValidationStatus;
 import de.saxsys.mvvmfx.utils.validation.Validator;
 import kong.unirest.UnirestException;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 
 public class NetworkTabViewModel implements PreferenceTabViewModel {
-    private static final Logger LOGGER = LoggerFactory.getLogger(NetworkTabViewModel.class);
-
     private final BooleanProperty remoteServerProperty = new SimpleBooleanProperty();
     private final StringProperty remotePortProperty = new SimpleStringProperty("");
     private final BooleanProperty proxyUseProperty = new SimpleBooleanProperty();
@@ -57,6 +52,7 @@ public class NetworkTabViewModel implements PreferenceTabViewModel {
     private final BooleanProperty proxyUseAuthenticationProperty = new SimpleBooleanProperty();
     private final StringProperty proxyUsernameProperty = new SimpleStringProperty("");
     private final StringProperty proxyPasswordProperty = new SimpleStringProperty("");
+    private final BooleanProperty proxyPersistPasswordProperty = new SimpleBooleanProperty();
     private final ListProperty<CustomCertificateViewModel> customCertificateListProperty = new SimpleListProperty<>(FXCollections.observableArrayList());
 
     private final Validator remotePortValidator;
@@ -92,7 +88,8 @@ public NetworkTabViewModel(DialogService dialogService, PreferencesService prefe
                 proxyPreferences.getPort(),
                 proxyPreferences.shouldUseAuthentication(),
                 proxyPreferences.getUsername(),
-                proxyPreferences.getPassword());
+                proxyPreferences.getPassword(),
+                proxyPreferences.shouldPersistPassword());
 
         remotePortValidator = new FunctionBasedValidator<>(
                 remotePortProperty,
@@ -140,6 +137,7 @@ public NetworkTabViewModel(DialogService dialogService, PreferencesService prefe
                         Localization.lang("Network"),
                         Localization.lang("Proxy configuration"),
                         Localization.lang("Please specify a password"))));
+
         this.trustStoreManager = new TrustStoreManager(Path.of(sslPreferences.getTruststorePath()));
     }
 
@@ -159,6 +157,7 @@ private void setProxyValues() {
         proxyUseAuthenticationProperty.setValue(proxyPreferences.shouldUseAuthentication());
         proxyUsernameProperty.setValue(proxyPreferences.getUsername());
         proxyPasswordProperty.setValue(proxyPreferences.getPassword());
+        proxyPersistPasswordProperty.setValue(proxyPreferences.shouldPersistPassword());
     }
 
     private void setSSLValues() {
@@ -181,24 +180,6 @@ private void setSSLValues() {
 
     @Override
     public void storeSettings() {
-        storeRemoteSettings();
-        storeProxySettings(new ProxyPreferences(
-                proxyUseProperty.getValue(),
-                proxyHostnameProperty.getValue().trim(),
-                proxyPortProperty.getValue().trim(),
-                proxyUseAuthenticationProperty.getValue(),
-                proxyUsernameProperty.getValue().trim(),
-                proxyPasswordProperty.getValue()
-        ));
-        storeSSLSettings();
-    }
-
-    private void storeRemoteSettings() {
-        RemotePreferences newRemotePreferences = new RemotePreferences(
-                remotePreferences.getPort(),
-                remoteServerProperty.getValue()
-        );
-
         getPortAsInt(remotePortProperty.getValue()).ifPresent(newPort -> {
             if (remotePreferences.isDifferentPort(newPort)) {
                 remotePreferences.setPort(newPort);
@@ -212,25 +193,16 @@ private void storeRemoteSettings() {
             remotePreferences.setUseRemoteServer(false);
             Globals.REMOTE_LISTENER.stop();
         }
-    }
-
-    private void storeProxySettings(ProxyPreferences newProxyPreferences) {
-        if (Objects.equals(newProxyPreferences, proxyPreferences)) {
-            // nothing changed; thus, nothing to store
-            return;
-        }
 
-        ProxyRegisterer.register(newProxyPreferences);
+        proxyPreferences.setUseProxy(proxyUseProperty.getValue());
+        proxyPreferences.setHostname(proxyHostnameProperty.getValue().trim());
+        proxyPreferences.setPort(proxyPortProperty.getValue().trim());
+        proxyPreferences.setUseAuthentication(proxyUseAuthenticationProperty.getValue());
+        proxyPreferences.setUsername(proxyUsernameProperty.getValue().trim());
+        proxyPreferences.setPersistPassword(proxyPersistPasswordProperty.getValue()); // Set before the password to actually persist
+        proxyPreferences.setPassword(proxyPasswordProperty.getValue());
+        ProxyRegisterer.register(proxyPreferences);
 
-        proxyPreferences.setUseProxy(newProxyPreferences.shouldUseProxy());
-        proxyPreferences.setHostname(newProxyPreferences.getHostname());
-        proxyPreferences.setPort(newProxyPreferences.getPort());
-        proxyPreferences.setUseAuthentication(newProxyPreferences.shouldUseAuthentication());
-        proxyPreferences.setUsername(newProxyPreferences.getUsername());
-        proxyPreferences.setPassword(newProxyPreferences.getPassword());
-    }
-
-    public void storeSSLSettings() {
         trustStoreManager.flush();
     }
 
@@ -299,15 +271,14 @@ public void checkConnection() {
 
         final String testUrl = "http://jabref.org";
 
-        // Workaround for testing, since the URLDownload uses stored proxy settings, see
-        // preferences.storeProxyPreferences(...) below.
-        storeProxySettings(new ProxyPreferences(
+        ProxyRegisterer.register(new ProxyPreferences(
                 proxyUseProperty.getValue(),
                 proxyHostnameProperty.getValue().trim(),
                 proxyPortProperty.getValue().trim(),
                 proxyUseAuthenticationProperty.getValue(),
                 proxyUsernameProperty.getValue().trim(),
-                proxyPasswordProperty.getValue()
+                proxyPasswordProperty.getValue(),
+                proxyPersistPasswordProperty.getValue()
         ));
 
         URLDownload urlDownload;
@@ -324,7 +295,7 @@ public void checkConnection() {
             dialogService.showErrorDialogAndWait(dialogTitle, connectionFailedText);
         }
 
-        storeProxySettings(backupProxyPreferences);
+        ProxyRegisterer.register(backupProxyPreferences);
     }
 
     @Override
@@ -368,6 +339,10 @@ public StringProperty proxyPasswordProperty() {
         return proxyPasswordProperty;
     }
 
+    public BooleanProperty proxyPersistPasswordProperty() {
+        return proxyPersistPasswordProperty;
+    }
+
     public ListProperty<CustomCertificateViewModel> customCertificateListProperty() {
         return customCertificateListProperty;
     }

diff --git a/src/main/java/org/jabref/logic/net/ProxyPreferences.java b/src/main/java/org/jabref/logic/net/ProxyPreferences.java
@@ -15,22 +15,25 @@ public class ProxyPreferences {
     private final BooleanProperty useAuthentication;
     private final StringProperty username;
     private final StringProperty password;
+    private final BooleanProperty persistPassword;
 
     public ProxyPreferences(Boolean useProxy,
                             String hostname,
                             String port,
                             Boolean useAuthentication,
                             String username,
-                            String password) {
+                            String password,
+                            boolean persistPassword) {
         this.useProxy = new SimpleBooleanProperty(useProxy);
         this.hostname = new SimpleStringProperty(hostname);
         this.port = new SimpleStringProperty(port);
         this.useAuthentication = new SimpleBooleanProperty(useAuthentication);
         this.username = new SimpleStringProperty(username);
         this.password = new SimpleStringProperty(password);
+        this.persistPassword = new SimpleBooleanProperty(persistPassword);
     }
 
-    public final Boolean shouldUseProxy() {
+    public final boolean shouldUseProxy() {
         return useProxy.getValue();
     }
 
@@ -66,8 +69,8 @@ public void setPort(String port) {
         this.port.set(port);
     }
 
-    public final Boolean shouldUseAuthentication() {
-        return useAuthentication.getValue();
+    public final boolean shouldUseAuthentication() {
+        return useAuthentication.get();
     }
 
     public BooleanProperty useAuthenticationProperty() {
@@ -102,6 +105,18 @@ public void setPassword(String password) {
         this.password.set(password);
     }
 
+    public boolean shouldPersistPassword() {
+        return persistPassword.get();
+    }
+
+    public BooleanProperty persistPasswordProperty() {
+        return persistPassword;
+    }
+
+    public void setPersistPassword(boolean persistPassword) {
+        this.persistPassword.set(persistPassword);
+    }
+
     @Override
     public boolean equals(Object o) {
         if (this == o) {
@@ -118,7 +133,8 @@ public boolean equals(Object o) {
                 && Objects.equals(port.getValue(), other.port.getValue())
                 && Objects.equals(useAuthentication.getValue(), other.useAuthentication.getValue())
                 && Objects.equals(username.getValue(), other.username.getValue())
-                && Objects.equals(password.getValue(), other.password.getValue());
+                && Objects.equals(password.getValue(), other.password.getValue())
+                && Objects.equals(persistPassword.getValue(), other.persistPassword.getValue());
     }
 
     @Override
@@ -129,6 +145,7 @@ public int hashCode() {
                 port.getValue(),
                 useAuthentication.getValue(),
                 username.getValue(),
-                password.getValue());
+                password.getValue(),
+                persistPassword.getValue());
     }
 }