Fixes concurrency issues. Refactoring and simplifications to improve readability

georgweiss · georgweiss · commit ac8c88b37c06 · 2025-09-24T11:12:19.000+02:00
diff --git a/app/credentials-management/src/main/java/org/phoebus/applications/credentialsmanagement/CredentialsManagementController.java b/app/credentials-management/src/main/java/org/phoebus/applications/credentialsmanagement/CredentialsManagementController.java
diff --git a/app/credentials-management/src/main/java/org/phoebus/applications/credentialsmanagement/Messages.java b/app/credentials-management/src/main/java/org/phoebus/applications/credentialsmanagement/Messages.java
@@ -31,6 +31,7 @@ public class Messages {
     public static String SecureStoreErrorBody;
     public static String Title;
     public static String ServiceConnectionFailure;
+    public static String UnknownError;
     public static String UserNotAuthenticated;
 
     static
diff --git a/app/credentials-management/src/main/resources/org/phoebus/applications/credentialsmanagement/messages.properties b/app/credentials-management/src/main/resources/org/phoebus/applications/credentialsmanagement/messages.properties
@@ -32,5 +32,6 @@ SecureStoreErrorTitle=Application Launch Failed
 SecureStoreErrorBody=Failed to instantiate the secure store
 ServiceConnectionFailure=Unable to connect to service
 Title=Credentials Management
+UnkownError=Unknown error
 Username=Username
 UserNotAuthenticated=User not authenticated
diff --git a/app/logbook/olog/client-es/src/main/java/org/phoebus/applications/logbook/authentication/OlogServiceAuthenticationProvider.java b/app/logbook/olog/client-es/src/main/java/org/phoebus/applications/logbook/authentication/OlogServiceAuthenticationProvider.java
@@ -19,6 +19,7 @@
 package org.phoebus.applications.logbook.authentication;
 
 import org.phoebus.olog.es.api.OlogHttpClient;
+import org.phoebus.security.authorization.AuthenticationStatus;
 import org.phoebus.security.authorization.ServiceAuthenticationException;
 import org.phoebus.security.authorization.ServiceAuthenticationProvider;
 import org.phoebus.security.tokens.AuthenticationScope;
@@ -31,22 +32,29 @@ public class OlogServiceAuthenticationProvider implements ServiceAuthenticationP
 
     private final AuthenticationScope ologAuthenticationScope;
 
-    public OlogServiceAuthenticationProvider(){
+    private final Logger logger = Logger.getLogger(OlogServiceAuthenticationProvider.class.getName());
+
+    public OlogServiceAuthenticationProvider() {
         ologAuthenticationScope = new OlogAuthenticationScope();
     }
 
     @Override
-    public void authenticate(String username, String password) throws ConnectException{
+    public AuthenticationStatus authenticate(String username, String password) {
         try {
             OlogHttpClient.builder().build().authenticate(username, password);
-        }
-        catch(ConnectException e){
-            throw e;
+            return AuthenticationStatus.AUTHENTICATED;
+        } catch (ConnectException e) {
+            logger.log(Level.WARNING, "Unable to connect to logbook service");
+            return AuthenticationStatus.SERVICE_OFFLINE;
+        } catch (ServiceAuthenticationException e){
+            logger.log(Level.WARNING, "User " + username + " not authenticated");
+            return AuthenticationStatus.BAD_CREDENTIALS;
         }
         catch (Exception e) {
-            Logger.getLogger(OlogServiceAuthenticationProvider.class.getName())
-                    .log(Level.WARNING, "Failed to authenticate user " + username + " with logbook service");
-            throw new ServiceAuthenticationException("Failed to authenticate user " + username + " with logbook service");
+            // NOTE!!! Exception message and/or stack trace could contain request URL and consequently
+            // user's password, so do not log or propagate it.
+            logger.log(Level.WARNING, "Failed to authenticate user " + username + " with logbook service, reason unknown");
+            return AuthenticationStatus.UNKNOWN_ERROR;
         }
     }
 
diff --git a/app/logbook/olog/client-es/src/main/java/org/phoebus/olog/es/api/OlogHttpClient.java b/app/logbook/olog/client-es/src/main/java/org/phoebus/olog/es/api/OlogHttpClient.java
@@ -26,6 +26,7 @@
 import org.phoebus.olog.es.api.model.OlogObjectMappers;
 import org.phoebus.olog.es.api.model.OlogSearchResult;
 import org.phoebus.olog.es.authentication.LoginCredentials;
+import org.phoebus.security.authorization.ServiceAuthenticationException;
 import org.phoebus.security.store.SecureStore;
 import org.phoebus.security.tokens.AuthenticationScope;
 import org.phoebus.security.tokens.ScopedAuthenticationToken;
@@ -35,6 +36,7 @@
 import javax.ws.rs.core.MultivaluedHashMap;
 import javax.ws.rs.core.MultivaluedMap;
 import java.io.InputStream;
+import java.net.ConnectException;
 import java.net.CookieManager;
 import java.net.CookiePolicy;
 import java.net.URI;
@@ -362,7 +364,6 @@ public void groupLogEntries(List<Long> logEntryIds) throws LogbookException {
      * @throws Exception if the login fails, e.g. bad credentials or service off-line.
      */
     public void authenticate(String userName, String password) throws Exception {
-
         String stringBuilder = Preferences.olog_url +
                 "/login";
         HttpRequest request = HttpRequest.newBuilder()
@@ -372,9 +373,7 @@ public void authenticate(String userName, String password) throws Exception {
                 .build();
         HttpResponse<String> response = httpClient.send(request, HttpResponse.BodyHandlers.ofString());
         if (response.statusCode() == 401) {
-            throw new Exception("Failed to login: user unauthorized");
-        } else if (response.statusCode() != 200) {
-            throw new Exception("Failed to login, got HTTP status " + response.statusCode());
+            throw new ServiceAuthenticationException("Failed to login: user not authorized");
         }
     }
 
diff --git a/app/save-and-restore/app/src/main/java/org/phoebus/applications/saveandrestore/authentication/SaveAndRestoreAuthenticationProvider.java b/app/save-and-restore/app/src/main/java/org/phoebus/applications/saveandrestore/authentication/SaveAndRestoreAuthenticationProvider.java
@@ -19,8 +19,9 @@
 
 package org.phoebus.applications.saveandrestore.authentication;
 
-import org.phoebus.applications.saveandrestore.model.UserData;
 import org.phoebus.applications.saveandrestore.ui.SaveAndRestoreService;
+import org.phoebus.security.authorization.AuthenticationStatus;
+import org.phoebus.security.authorization.ServiceAuthenticationException;
 import org.phoebus.security.authorization.ServiceAuthenticationProvider;
 import org.phoebus.security.tokens.AuthenticationScope;
 
@@ -34,28 +35,30 @@
 public class SaveAndRestoreAuthenticationProvider implements ServiceAuthenticationProvider {
 
     private final AuthenticationScope saveAndRestoreAuthenticationScope;
+    private final Logger logger = Logger.getLogger(SaveAndRestoreAuthenticationProvider.class.getName());
 
     public SaveAndRestoreAuthenticationProvider() {
         saveAndRestoreAuthenticationScope = new SaveAndRestoreAuthenticationScope();
     }
 
     @Override
-    public void authenticate(String username, String password) throws ConnectException{
+    public AuthenticationStatus authenticate(String username, String password) {
         SaveAndRestoreService saveAndRestoreService = SaveAndRestoreService.getInstance();
         try {
-            UserData userData = saveAndRestoreService.authenticate(username, password);
-            Logger.getLogger(SaveAndRestoreAuthenticationProvider.class.getName())
-                    .log(Level.INFO, "User " + userData.getUserName() + " successfully signed in");
-        }
-        catch(ConnectException e){
-            throw e;
-        }
-        catch (Exception e) {
+            saveAndRestoreService.authenticate(username, password);
+            logger.log(Level.INFO, "User " + username + " successfully signed in");
+            return AuthenticationStatus.AUTHENTICATED;
+        } catch (ConnectException e) {
+            logger.log(Level.WARNING, "Unable to connect to save&restore service");
+            return AuthenticationStatus.SERVICE_OFFLINE;
+        } catch (ServiceAuthenticationException e) {
+            logger.log(Level.WARNING, "User " + username + " not authenticated");
+            return AuthenticationStatus.BAD_CREDENTIALS;
+        } catch (Exception e) {
             // NOTE!!! Exception message and/or stack trace could contain request URL and consequently
             // user's password, so do not log or propagate it.
-            Logger.getLogger(SaveAndRestoreAuthenticationProvider.class.getName())
-                    .log(Level.WARNING, "Failed to authenticate user " + username + " with save&restore service");
-            throw new RuntimeException("Failed to authenticate user " + username + " with save&restore service");
+            logger.log(Level.WARNING, "Failed to authenticate user " + username + " with save&restore service, reason unknown");
+            return AuthenticationStatus.UNKNOWN_ERROR;
         }
     }
 
diff --git a/app/save-and-restore/app/src/main/java/org/phoebus/applications/saveandrestore/client/SaveAndRestoreClient.java b/app/save-and-restore/app/src/main/java/org/phoebus/applications/saveandrestore/client/SaveAndRestoreClient.java
@@ -30,12 +30,10 @@
 import org.phoebus.applications.saveandrestore.model.SnapshotItem;
 import org.phoebus.applications.saveandrestore.model.Tag;
 import org.phoebus.applications.saveandrestore.model.TagData;
-import org.phoebus.applications.saveandrestore.model.UserData;
 import org.phoebus.applications.saveandrestore.model.search.Filter;
 import org.phoebus.applications.saveandrestore.model.search.SearchResult;
 
 import javax.ws.rs.core.MultivaluedMap;
-import java.net.ConnectException;
 import java.util.List;
 
 /**
@@ -249,10 +247,9 @@ public interface SaveAndRestoreClient {
      *
      * @param userName User's account name
      * @param password User's password
-     * @return A {@link UserData} object if login is successful, otherwise implementation should throw
-     * an exception.
+     * @throws Exception e.g. if service is off-line or if user credentials are rejected.
      */
-    UserData authenticate(String userName, String password) throws ConnectException;
+    void authenticate(String userName, String password) throws Exception;
 
     /**
      * Requests service to restore the specified {@link SnapshotItem}s
diff --git a/app/save-and-restore/app/src/main/java/org/phoebus/applications/saveandrestore/client/SaveAndRestoreClientImpl.java b/app/save-and-restore/app/src/main/java/org/phoebus/applications/saveandrestore/client/SaveAndRestoreClientImpl.java
@@ -24,15 +24,14 @@
 import org.phoebus.applications.saveandrestore.model.SnapshotItem;
 import org.phoebus.applications.saveandrestore.model.Tag;
 import org.phoebus.applications.saveandrestore.model.TagData;
-import org.phoebus.applications.saveandrestore.model.UserData;
 import org.phoebus.applications.saveandrestore.model.search.Filter;
 import org.phoebus.applications.saveandrestore.model.search.SearchResult;
+import org.phoebus.security.authorization.ServiceAuthenticationException;
 import org.phoebus.security.store.SecureStore;
 import org.phoebus.security.tokens.ScopedAuthenticationToken;
 import org.phoebus.util.http.QueryParamsHelper;
 
 import javax.ws.rs.core.MultivaluedMap;
-import java.net.ConnectException;
 import java.net.CookieHandler;
 import java.net.CookieManager;
 import java.net.CookiePolicy;
@@ -577,21 +576,17 @@ public List<Node> deleteTag(TagData tagData) {
      * @return {@inheritDoc}
      */
     @Override
-    public UserData authenticate(String userName, String password) throws ConnectException {
-        try {
-            String stringBuilder = Preferences.jmasarServiceUrl +
-                    "/login";
-            HttpRequest request = HttpRequest.newBuilder()
-                    .uri(URI.create(stringBuilder))
-                    .header("Content-Type", CONTENT_TYPE_JSON)
-                    .POST(HttpRequest.BodyPublishers.ofString(OBJECT_MAPPER.writeValueAsString(new LoginCredentials(userName, password))))
-                    .build();
-            HttpResponse<String> response = CLIENT.send(request, HttpResponse.BodyHandlers.ofString());
-            return OBJECT_MAPPER.readValue(response.body(), UserData.class);
-        } catch (ConnectException e) {
-            throw e;
-        } catch (Exception e) {
-            throw new RuntimeException(e);
+    public void authenticate(String userName, String password) throws Exception {
+        String stringBuilder = Preferences.jmasarServiceUrl +
+                "/login";
+        HttpRequest request = HttpRequest.newBuilder()
+                .uri(URI.create(stringBuilder))
+                .header("Content-Type", CONTENT_TYPE_JSON)
+                .POST(HttpRequest.BodyPublishers.ofString(OBJECT_MAPPER.writeValueAsString(new LoginCredentials(userName, password))))
+                .build();
+        HttpResponse<String> response = CLIENT.send(request, HttpResponse.BodyHandlers.ofString());
+        if (response.statusCode() == 401) {
+            throw new ServiceAuthenticationException("User not authenticated with save&restore service");
         }
     }
 
diff --git a/app/save-and-restore/app/src/main/java/org/phoebus/applications/saveandrestore/ui/SaveAndRestoreService.java b/app/save-and-restore/app/src/main/java/org/phoebus/applications/saveandrestore/ui/SaveAndRestoreService.java
@@ -33,7 +33,6 @@
 import org.phoebus.applications.saveandrestore.model.SnapshotItem;
 import org.phoebus.applications.saveandrestore.model.Tag;
 import org.phoebus.applications.saveandrestore.model.TagData;
-import org.phoebus.applications.saveandrestore.model.UserData;
 import org.phoebus.applications.saveandrestore.model.search.Filter;
 import org.phoebus.applications.saveandrestore.model.search.SearchResult;
 import org.phoebus.core.vtypes.VDisconnectedData;
@@ -46,7 +45,6 @@
 import java.time.Instant;
 import java.util.ArrayList;
 import java.util.List;
-import java.util.Set;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Future;
 import java.util.concurrent.LinkedBlockingQueue;
@@ -316,17 +314,15 @@ public List<Node> deleteTag(TagData tagData) throws Exception {
     }
 
     /**
-     * Authenticate user, needed for all non-GET endpoints if service requires it
+     * Authenticate user, needed for all non-GET endpoints if service requires it. Note that
+     * this is executed in a synchronous manner, i.e. an {@link ExecutorService} is not used.
      *
      * @param userName User's account name
      * @param password User's password
-     * @return A {@link UserData} object
-     * @throws Exception if authentication fails
+     * @throws Exception if authentication fails, e.g. service off-line or invalid credentials
      */
-    public UserData authenticate(String userName, String password) throws Exception {
-        Future<UserData> future =
-                executor.submit(() -> saveAndRestoreClient.authenticate(userName, password));
-        return future.get();
+    public void authenticate(String userName, String password) throws Exception {
+        saveAndRestoreClient.authenticate(userName, password);
     }
 
     /**
diff --git a/app/save-and-restore/app/src/test/java/org/phoebus/applications/saveandrestore/client/HttpClientTestIT.java b/app/save-and-restore/app/src/test/java/org/phoebus/applications/saveandrestore/client/HttpClientTestIT.java
@@ -92,7 +92,7 @@ public static void cleanUp() {
     }
 
     @Test
-    public void testAuthenticate() throws ConnectException {
+    public void testAuthenticate() throws Exception {
         saveAndRestoreClient.authenticate("admin", "adminPass");
     }
 
diff --git a/app/save-and-restore/model/src/main/java/org/phoebus/applications/saveandrestore/model/UserData.java b/app/save-and-restore/model/src/main/java/org/phoebus/applications/saveandrestore/model/UserData.java
diff --git a/core/security/src/main/java/org/phoebus/security/authorization/AuthenticationStatus.java b/core/security/src/main/java/org/phoebus/security/authorization/AuthenticationStatus.java
@@ -0,0 +1,17 @@
+/*
+ * Copyright (C) 2025 European Spallation Source ERIC.
+ */
+
+package org.phoebus.security.authorization;
+
+/**
+ * Enum indicating the current or updated authentication status.
+ */
+public enum AuthenticationStatus {
+    UNDETERMINED,       // Not set, e.g. user not yet logged in
+    AUTHENTICATED,      // Successfully authenticated after login call to service
+    CACHED,             // User authenticated based on cached credentials
+    BAD_CREDENTIALS,    // Not authenticated by service
+    SERVICE_OFFLINE,    // Service not reachable
+    UNKNOWN_ERROR       // Authentication with service failes for unknown reason
+}
diff --git a/core/security/src/main/java/org/phoebus/security/authorization/ServiceAuthenticationProvider.java b/core/security/src/main/java/org/phoebus/security/authorization/ServiceAuthenticationProvider.java
@@ -21,8 +21,6 @@
 import org.phoebus.security.store.SecureStore;
 import org.phoebus.security.tokens.AuthenticationScope;
 
-import java.net.ConnectException;
-
 /**
  * Implementations of this interface are used to announce support for
  * a log in procedure to a service.
@@ -33,8 +31,9 @@ public interface ServiceAuthenticationProvider {
      * Authenticates with the announced service.
      * @param username User name
      * @param password Password
+     * @return An {@link AuthenticationStatus} indicating the outcome.
      */
-    void authenticate(String username, String password) throws ConnectException;
+    AuthenticationStatus authenticate(String username, String password);
 
     /**
      * Signs out user from the service.
diff --git a/core/security/src/main/java/org/phoebus/security/store/SecureStore.java b/core/security/src/main/java/org/phoebus/security/store/SecureStore.java
@@ -167,7 +167,7 @@ public void setScopedAuthentication(ScopedAuthenticationToken scopedAuthenticati
 
     /**
      * Retrieves all {@link ScopedAuthenticationToken}s in the secure store. A {@link ScopedAuthenticationToken}
-     * must be composed of both a user name and a password, i.e. items in the secure store that are not
+     * must be composed of both a username and a password, i.e. items in the secure store that are not
      * considered to be a part of an authentication token are ignored.
      * @return List of {@link ScopedAuthenticationToken}s.
      * @throws Exception on error
diff --git a/core/security/src/test/java/org/phoebus/security/store/ServiceAuthenticationProviderOne.java b/core/security/src/test/java/org/phoebus/security/store/ServiceAuthenticationProviderOne.java
@@ -4,16 +4,15 @@
 
 package org.phoebus.security.store;
 
+import org.phoebus.security.authorization.AuthenticationStatus;
 import org.phoebus.security.authorization.ServiceAuthenticationProvider;
 import org.phoebus.security.tokens.AuthenticationScope;
 
-import java.net.ConnectException;
-
 public class ServiceAuthenticationProviderOne implements ServiceAuthenticationProvider {
 
     @Override
-    public void authenticate(String username, String password) throws ConnectException {
-
+    public AuthenticationStatus authenticate(String username, String password){
+        return AuthenticationStatus.AUTHENTICATED;
     }
 
     @Override
diff --git a/core/security/src/test/java/org/phoebus/security/store/ServiceAuthenticationProviderTwo.java b/core/security/src/test/java/org/phoebus/security/store/ServiceAuthenticationProviderTwo.java
diff --git a/services/save-and-restore/src/main/java/org/phoebus/service/saveandrestore/web/controllers/AuthenticationController.java b/services/save-and-restore/src/main/java/org/phoebus/service/saveandrestore/web/controllers/AuthenticationController.java

Original file line number	Diff line number	Diff line change
`@@ -92,7 +92,7 @@ public static void cleanUp() {`
`92`	`92`	`}`
`93`	`93`
`94`	`94`	`@Test`
`95`		`- public void testAuthenticate() throws ConnectException {`
	`95`	`+ public void testAuthenticate() throws Exception {`
`96`	`96`	`saveAndRestoreClient.authenticate("admin", "adminPass");`
`97`	`97`	`}`
`98`	`98`