Login to all services #3554

georgweiss · 2025-09-19T13:58:32Z

As per user request...

The credentials manager app now offers a way to sign in to all services (e.g. logbook, save&restore) through a single action.
In case any of the services rejects user's credentials (or is off-line), the dialog stays open (see screen shot) to indicate the problem.

Documentation updated accordingly.

abrahamwolk · 2025-09-22T08:08:35Z

...ain/java/org/phoebus/applications/credentialsmanagement/CredentialsManagementController.java

+                stage.close();
+            }
+        } catch (Exception e) {
+            LOGGER.log(Level.WARNING, "Failed to delete all authentication tokens from key store", e);


Is this error message correct?

Good catch, copy paste mistake

abrahamwolk · 2025-09-22T08:11:55Z

...ain/java/org/phoebus/applications/credentialsmanagement/CredentialsManagementController.java

-                stage.close();
+                        serviceItem.getUsername().get(),
+                        serviceItem.getPassword().get()));
+                loggedInCount.set(loggedInCount.get() + 1);


Reading from and writing to loggedInCount in this way is not thread-safe.

Login (via authenticate) and logout APIs are synchronous, so why should this be thread safe?

What if two concurrent calls to login() get interleaved? I think that if a computer is slow to respond (maybe it is running out of RAM and/or the CPU is busy), then the UI may become unresponsive and the user may click on "Login" several times, and the calls may be interleaved.

That's a good point. Will disable login buttons such that new login attempts cannot be invoked until ongoing has completed.

Disabling the button is a good idea, I think, but I think the calls need to be made synchronous also, since state is being modified. setDisable() may also not finish until the button has already been pressed twice.

Sure, that could happen. Will secure concurrency.

abrahamwolk · 2025-09-22T08:14:53Z

...ain/java/org/phoebus/applications/credentialsmanagement/CredentialsManagementController.java

+        } catch (ServiceAuthenticationException exception) {
+            serviceItem.setLoginResultMessage(Messages.UserNotAuthenticated);
        }
+        serviceItem.setLoginResult(LoginResult.FAILED);


Do I understand the code correctly here: is the login-result always set to LoginResult.FAILED here when login() is called?

You're right, there is a return statement missing.

abrahamwolk · 2025-09-22T08:15:44Z

...ain/java/org/phoebus/applications/credentialsmanagement/CredentialsManagementController.java

-            updateTable();
+            serviceItem.serviceAuthenticationProvider.logout();
+            serviceItem.setLoggedOut();
+            loggedInCount.set(loggedInCount.get() - 1);


This assignment is not thread-safe.

Login (via authenticate) and logout APIs are synchronous, so why should this be thread safe?

abrahamwolk · 2025-09-22T08:24:17Z

...ain/java/org/phoebus/applications/credentialsmanagement/CredentialsManagementController.java

+        private final ObjectProperty<LoginResult> loginResult = new SimpleObjectProperty<>(LoginResult.OK);

        public ServiceItem(ServiceAuthenticationProvider serviceAuthenticationProvider, String username, String password) {
            this.serviceAuthenticationProvider = serviceAuthenticationProvider;


I think it may be a good idea to change the type of serviceAuthenticationProvider from ServiceAuthenticationProvider to Optional<ServiceAuthenticationProvider> since serviceAuthenticationProvider may be null. (See lines 287-290: https://github.com/ControlSystemStudio/phoebus/pull/3554/files#diff-8682c52d79413fbe3e61310d72e1daa306a698af4c7efe00f97ffa2e16952350R287-R290.) This way, the fact that the field may be empty is reflected in its type. See also line 358 [1] and line 364 [2].

[1] https://github.com/ControlSystemStudio/phoebus/pull/3554/files#diff-8682c52d79413fbe3e61310d72e1daa306a698af4c7efe00f97ffa2e16952350R358
[2] https://github.com/ControlSystemStudio/phoebus/pull/3554/files#diff-8682c52d79413fbe3e61310d72e1daa306a698af4c7efe00f97ffa2e16952350R364

In practice the ServiceAuthenticationProvier should never be null as saved tokens are associated only with existing ServiceAuthenticationProviers. I will update the callee code to handle potential nulls.

abrahamwolk · 2025-09-22T09:01:26Z

.../java/org/phoebus/applications/logbook/authentication/OlogServiceAuthenticationProvider.java

+        }
+        catch (Exception e) {
            Logger.getLogger(OlogServiceAuthenticationProvider.class.getName())
                    .log(Level.WARNING, "Failed to authenticate user " + username + " with logbook service", e);


Can the stack trace of e contain sensitive information? The exception originates from the call to OlogHttpClient.builder().build().authenticate(username, password) which contains username and password. I think it may be safer to not include the exception e in the log, and also not to re-throw it (https://github.com/ControlSystemStudio/phoebus/pull/3554/files#diff-1cc8e66eabcdb21075575c28e20439ca032c7b00a5770c4044c369028a6d2eadR44).

No. But I'll remove the exception anyway.

abrahamwolk · 2025-09-22T09:12:38Z

...p/src/main/java/org/phoebus/applications/saveandrestore/client/SaveAndRestoreClientImpl.java

+        } catch (ConnectException e) {
+            throw e;
        } catch (Exception e) {
            throw new RuntimeException(e);


Same as above: is it possible that the exception e contains sensitive information, since it originates in a call that contains userName and password? I think it may be safer if e is not re-thrown (both on this line and on line 592).

No. But I'll remove the exception anyway.

Exception not logged.

abrahamwolk · 2025-09-22T09:15:36Z

...security/src/main/java/org/phoebus/security/authorization/ServiceAuthenticationProvider.java

+            SecureStore secureStore = new SecureStore();
+            secureStore.deleteScopedAuthenticationToken(getAuthenticationScope());
+        } catch (Exception e) {
+            throw new RuntimeException(e);


I think it's safer not to re-throw an exception here, since it originates from a call to secureStore.

The case where exceptions could be thrown is when calling the Java keystore APIs. It would be quite surprising to see those APIs leak sensitive information.

abrahamwolk · 2025-09-22T09:22:47Z

core/security/src/main/java/org/phoebus/security/store/SecureStore.java

-                password = get(scope.getName() + "." + PASSWORD_TAG);
+                scope = findAuthenticationScopeFromProviders(tokens[0]);
+                if(scope == null){
+                    throw new IllegalArgumentException("No authentication provider found matching scope \"" + tokens[0] + "\"");


I think it's not safe to throw exceptions from within a class called SecureStore, since exceptions may contain sensitive information.

Could a failure be handled in a type-safe way in the code by a suitable return-type? Perhaps the function could have the return type Optional<List<ScopedAuthenticationToken>> and return Optional.empty() in case it is not successful.

See previous comment.

abrahamwolk · 2025-09-22T09:31:06Z

...security/src/main/java/org/phoebus/security/authorization/ServiceAuthenticationProvider.java

     * @param password Password
     */
-    void authenticate(String username, String password);
+    void authenticate(String username, String password) throws ConnectException;


I suggest not to throw exceptions in authenticate(), but instead to change its return-type to something that can represent failure. Perhaps boolean would work: true indicates success and false indicates failure to login. The motivation is to avoid accidental printing of sensitive information in stack traces or similar.

The idea is to catch different type of exceptions and differentiate messages to user in the UI, e.g. "User not authenticated" vs "Service offline".

Decided to introduce an AuthenticationResult enum to differentiate between bad credentials and failure to connect. Will update PR shortly.

…readability

georgweiss · 2025-09-24T09:29:14Z

Updates based on peer feed-back. Also some refactoring in the name of simplification and readability.

abrahamwolk · 2025-09-24T11:16:36Z

...p/src/main/java/org/phoebus/applications/saveandrestore/client/SaveAndRestoreClientImpl.java

-            throw e;
-        } catch (Exception e) {
-            throw new RuntimeException(e);
+    public void authenticate(String userName, String password) throws Exception {


The declared exception that may be thrown is now more general (Exception instead of ConnectException) than before (i.e., there is less static information about it): is this correct?

That is correct. The change is about intercepting a true authentication response and turn that into an ServiceAuthenticationException. The SaveAndRestoreAuthenticationProvider then analyses exceptions to return the appropriate AuthenticationStatus

abrahamwolk · 2025-09-24T11:22:06Z

...ain/java/org/phoebus/applications/credentialsmanagement/CredentialsManagementController.java

                        btn.setOnAction((ActionEvent event) -> {
                            ServiceItem serviceItem = getTableRow().getItem();
-                            if (serviceItem != null && serviceItem.userLoggedIn.get()) {
+                            if (serviceItem != null && (serviceItem.authenticationStatus.get().equals(AuthenticationStatus.AUTHENTICATED) ||


I think the action of this button also needs to be synchronized.

The login and logout methods are synchronized.

abrahamwolk · 2025-09-24T11:23:08Z

...ain/java/org/phoebus/applications/credentialsmanagement/CredentialsManagementController.java

        private final StringProperty buttonTextProperty = new SimpleStringProperty();
        private final StringProperty loginResultMessage = new SimpleStringProperty();
-        private final ObjectProperty<LoginResult> loginResult = new SimpleObjectProperty<>(LoginResult.OK);
+        private final ObjectProperty<AuthenticationStatus> authenticationStatus = new SimpleObjectProperty<>();


Should the SimpleObjectProperty be initialized with an initial value?

I do not see the need.

abrahamwolk · 2025-09-24T11:29:40Z

...ain/java/org/phoebus/applications/credentialsmanagement/CredentialsManagementController.java

+                            username.get(),
+                            password.get()));
+                } catch (Exception e) {
+                    throw new RuntimeException(e);


I think it's better not to re-throw an exception here (both username and password are arguments to the call in the try-block). (Also Exception e can maybe be generalized to Throwable t, if the clause is intended to catch all errors.)

Will update to just log the problem. But I fail to see how credentials could be leaked.

abrahamwolk · 2025-09-24T11:30:34Z

.../java/org/phoebus/applications/logbook/authentication/OlogServiceAuthenticationProvider.java

-        catch(ConnectException e){
-            throw e;
+            return AuthenticationStatus.AUTHENTICATED;
+        } catch (ConnectException e) {


Is the catch-clause for ConnectException still relevant after the changes in this commit? (The catch-all clause Exception e should remain, of course).

See previous comment on how exceptions are handled.

abrahamwolk · 2025-09-24T11:35:28Z

.../java/org/phoebus/applications/logbook/authentication/OlogServiceAuthenticationProvider.java

-    public void authenticate(String username, String password) throws ConnectException{
+    public AuthenticationStatus authenticate(String username, String password) {
        try {
            OlogHttpClient.builder().build().authenticate(username, password);


Could the function authenticate() return the authentication status, instead of throwing exceptions when failing?

That is what it does.

abrahamwolk · 2025-09-24T11:37:05Z

app/logbook/olog/client-es/src/main/java/org/phoebus/olog/es/api/OlogHttpClient.java

     * @param password Password, must not be <code>null</code>.
     * @throws Exception if the login fails, e.g. bad credentials or service off-line.
     */
    public void authenticate(String userName, String password) throws Exception {


I suggest to return the authentication status as a value instead of throwing exceptions when there's an error. This makes it much easier to follow the control flow.

You have a point. Will move the exception handling to the http client classes.

abrahamwolk · 2025-09-24T11:37:52Z

.../java/org/phoebus/applications/logbook/authentication/OlogServiceAuthenticationProvider.java

+            logger.log(Level.WARNING, "User " + username + " not authenticated");
+            return AuthenticationStatus.BAD_CREDENTIALS;
        }
        catch (Exception e) {


This catch-all clause can be improved by changing it from Exception e to Throwable t.

Will not change it, see also https://www.baeldung.com/java-catch-throwable-bad-practice

Could still include the exception in the log message to show the reason and stack trace:

logger.log(Level.WARNING, "User " + username + " not authenticated", e);

georgweiss added 7 commits September 11, 2025 12:56

Minor improvements to the credentials management app

808ee62

UI updates to credentials management

7be1d16

Update credentials dialog UI

ce66c6d

CSS fixes for credentials dialog

cb10531

Disallow login attempt if either username or password are not set

77eea0a

Implementation of login to all services in credentials management

46bd059

Fix unit tests

0500c22

georgweiss requested review from shroffk and abrahamwolk September 19, 2025 13:58

abrahamwolk reviewed Sep 22, 2025

View reviewed changes

georgweiss added 2 commits September 22, 2025 13:16

Updated based on review feed-back

8f919fd

Fixes concurrency issues. Refactoring and simplifications to improve …

ac8c88b

…readability

georgweiss requested a review from abrahamwolk September 24, 2025 09:29

abrahamwolk reviewed Sep 24, 2025

View reviewed changes

Updates based on peer feedback

a4d624a

georgweiss requested a review from abrahamwolk September 24, 2025 12:59

Put back removed methods referenced by fxml

b40d471

Login to all services #3554

Are you sure you want to change the base?

Login to all services #3554

Uh oh!

Conversation

georgweiss commented Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

georgweiss Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

georgweiss Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

georgweiss Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abrahamwolk Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

georgweiss Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

georgweiss commented Sep 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

georgweiss commented Sep 19, 2025 •

edited

Loading

georgweiss Sep 23, 2025 •

edited

Loading

georgweiss Sep 22, 2025 •

edited

Loading

georgweiss Sep 22, 2025 •

edited

Loading

abrahamwolk Sep 22, 2025 •

edited

Loading

georgweiss Sep 22, 2025 •

edited

Loading

abrahamwolk Sep 24, 2025 •

edited

Loading

abrahamwolk Sep 24, 2025 •

edited

Loading