-3

I'm currently working on a password manager. Before making any changes to a certain service, the program will ask the user for a password for authorization and then proceed to show the appropriate dialog, if the password is correct.

The issue that I'm having is that if I go through the cycle of putting in my password to make the change, click "ok", and then proceeding to make changes on the shown dialog, on the next turn if instead of putting the password when prompted I close the prompt, then the program retrieves the password from the previous iteration although it has been explicitly cleared. Resulting in the concurrent dialog showing, which is only supposed to show if you put in the correct password.

private void handleEditButton(MouseEvent event) {
        Optional<String> rslt = passwordConfirmDialog.showAndWait();

        if (rslt.get().equals("")) 
            return; //Do not proceed

        String userInput = rslt.get().trim();

        // Complex expression, but use of && statement is necessary to avoid an
        // unecessary call to db and have return statement on this IF
        if (!(!userInput.isBlank() && isCorrectPassword(userInput))) {
            // show dialog
            AlertConfigs.invalidPasswordTransactionFailed.showAndWait();
            return;
        }

        System.out.println("Edit Handler: Correct password. -> " + userInput);

        //Proceed to show next dialog...
private void initializePasswordConfirmDialog() {

        passwordConfirmDialog.setTitle("User Account Control");
        passwordConfirmDialog.setHeaderText("Please enter your password to continue.");

        // Set the button types.
        ButtonType ok = new ButtonType("Ok", ButtonData.OK_DONE);
        passwordConfirmDialog.getDialogPane().getButtonTypes().addAll(ok, ButtonType.CANCEL);

        final PasswordField psField = new PasswordField();

        GridPane grid = new GridPane();

        grid.setHgap(10);
        grid.setVgap(10);
        grid.setPadding(new Insets(20, 150, 10, 10));

        grid.add(new Label("Please Enter your password"), 0, 0);
        grid.add(psField, 1, 0);

        passwordConfirmDialog.getDialogPane().setContent(grid);

        passwordConfirmDialog.setResultConverter(buttonType -> {

            String rslt = "";
            if (buttonType == ok) {
                rslt = psField.getText();
            }
           
            psField.clear();
            return rslt;
        });
    }

I've posted a video on YouTube to help visualize the problem. https://youtu.be/sgayh7Q7Ne8

The PasswordField in initializePasswordConfirmDialog() is cleared because whenever I run the the prompt the second time, the PasswordField is blank (visually). Nevertheless, for some reason it still grabs the result from the previous iteration.

The initializePasswordConfirmDialog() is called once inside the constructor and is responsible for set the passwordConfirmDialog variable with the adequate properties.

Some additional code:

 HomeController.java
    @FXML
    private GridPane servicesGrid;

    private Dialog<String> passwordConfirmDialog;
    private Dialog<Service> editServiceDialog;

    private final int NUM_COLUMNS = 7;

    public HomeController() {

        passwordConfirmDialog = new Dialog<>();
        initializePasswordConfirmDialog();

        editServiceDialog = new Dialog<>();

    }

    @Override
    public void initialize(URL arg0, ResourceBundle arg1) {
        loadServicesGridpane();
    }
    private void loadServicesGridpane() {
        ArrayList<Service> currS = acct.getServices();
        // int currentRow = 1;

        for (Service s : currS)
            addRowToServiceGrid(s);

    }

    private void addRowToServiceGrid(Service s) {
        int rowIdx = servicesGrid.getChildren().size() / 4;

        Button editButton = new Button("Edit");
        editButton.setOnMouseClicked(event -> {
            handleEditButton(event);
        });

       
        Button deleteButton = new Button("Delete");

        deleteButton.setOnMouseClicked(event -> {
            handleDeleteButton(event);
        });

        deleteButton.setId(s.getServiceName());

        Label currServiceName = new Label(s.getServiceName());
        currServiceName.setId(s.getServiceName());

        Label currUsername = new Label(s.getServiceUsername());

        Label currPassword = new Label(s.getServicePassword());

        Label dateCreated = new Label(s.getDateCreated());
        Label lastPssdChange = new Label(s.getLastPasswordChange());

        servicesGrid.addRow(rowIdx, currServiceName, currUsername, currPassword, dateCreated, lastPssdChange,
                deleteButton, editButton);

    }
trashgod
  • 203,806
  • 29
  • 246
  • 1,045
creativeAxe
  • 99
  • 1
  • 10
  • 3
    Provide a [mcve], then a video isn't really necessary as somebody could just run the app to replicate the issue (please make sure you understand the link, a minimal example showing only the issue which can be replicated through copy/paste without change or addition). – jewelsea Aug 12 '22 at 01:07
  • 2
    You don't seem to be calling a new `PasswordConfirmDialog` each time the edit `Button` is pressed. It looks like you are calling the same one each time the edit `Button` is pressed. What happens if you do `PasswordConfirmDialog passwordConfirmDialog = new PasswordConfirmDialog(); Optional rslt = passwordConfirmDialog.showAndWait();`? – SedJ601 Aug 12 '22 at 01:16
  • @sedJ601 Your suggestion is a great alternative, which works. However, resetting the passwordConfirmDialog each time to a new object is expensive as opposed to clearing the passwordfield inside of the passwordConfirmDialog when looked at from a scaling perspective. – creativeAxe Aug 12 '22 at 01:33
  • @jewelsea There are many things going on in the background that make it difficult to reproduce the code succinctly for a stackoverflow post, however, I've boiled it down to some bare bones. Of course, you'd just need to add your separate bare fxml files to run. -> https://openjfx.io/openjfx-docs/ – creativeAxe Aug 12 '22 at 01:36
  • 2
    How often do you expect the password dialog to be shown? I would think typically only once per application. And in that case, saving a reference to the dialog is actually more expensive because now it can't be GC'd. But even if it's shown more than once, how frequently do you expect this to be the case? My point is, this seems like a premature optimization. Have you profiled an actual problem? If not, just create a new dialog each time. Creating an object like this is not particularly expensive. – Slaw Aug 12 '22 at 04:42
  • 2
    "_you'd just need to add your separate bare fxml files to run_" – We shouldn't need to add anything, especially not anything code/UI related. For one, anything we have to add inherently involves guessing, which means we could easily miss the cause of the problem by accidently "solving" it. Secondly, you are asking for help from unpaid volunteers; please... make it as easy on us as possible. Help us help you. – Slaw Aug 12 '22 at 04:45
  • 1
    I profiled @SedJ601's suggestion and found very modest overhead. – trashgod Aug 12 '22 at 19:59

1 Answers1

1

To study the problem in isolation, I refactored this example to permit reusing the dialog. As shown below, reusing the dialog requires clearing the password field. Replace the parameter dialog with an invocation of createDialog() to see that creating the dialog each time does not require clearing the password field. Comparing the profile of each approach may help you decide which approach is acceptable; in my experiments, reuse added negligible memory overhead (~250 KB), and it protracted garbage collection slightly(~50 ms).

#!/bin/sh
java … DialogTest -reuse &
pid1=$!
java … DialogTest -no-reuse &
pid2=$!
echo $pid1 $pid2
jconsole $pid1 $pid2

Unfortunately, creating the dialog each time may only appear to solve the problem; it may have exposed a latent synchronization problem. In particular, verify that your result converter's callback executes on the JavaFX Application Thread. To illustrate, I've added a call to Platform.isFxApplicationThread() in resultsNotPresent() below.

import javafx.application.Application;
import javafx.application.Platform;
import javafx.scene.Scene;
import javafx.scene.control.Button;
import javafx.scene.control.ButtonType;
import javafx.scene.control.Dialog;
import javafx.scene.control.Label;
import javafx.scene.control.PasswordField;
import javafx.scene.control.TextField;
import javafx.scene.layout.HBox;
import javafx.scene.layout.VBox;
import javafx.stage.Stage;

/**
 * @see https://stackoverflow.com/q/73328282/230513
 * @see https://stackoverflow.com/a/44172143/230513
 */
public class DialogTest extends Application {

    private static boolean REUSE_DIALOG = true;

    private record Results(String text, String pass) {

        private static Results of(String text, String pass) {
            return new Results(text, pass);
        }
    }

    @Override
    public void start(Stage stage) {
        var label = new Label("Reuse: " + REUSE_DIALOG);
        var button = new Button("Button");
        if (REUSE_DIALOG) {
            var dialog = createDialog();
            button.setOnAction(e -> showDialog(dialog));
        } else {
            button.setOnAction(e -> showDialog(createDialog()));
        }
        stage.setScene(new Scene(new HBox(8, label, button)));
        stage.show();
    }

    private Dialog<Results> createDialog() {
        var dialog = new Dialog<Results>();
        dialog.setTitle("Dialog Test");
        dialog.setHeaderText("Please authenticate…");
        var dialogPane = dialog.getDialogPane();
        dialogPane.getButtonTypes().addAll(ButtonType.OK, ButtonType.CANCEL);
        var text = new TextField("Name");
        var pass = new PasswordField();
        dialogPane.setContent(new VBox(8, text, pass));
        dialog.showingProperty().addListener((o, wasShowing, isShowing) -> {
            if (isShowing) {
                Platform.runLater(pass::requestFocus);
            }
        });
        dialog.setResultConverter((ButtonType bt) -> {
            if (ButtonType.OK == bt) {
                var results = Results.of(text.getText(), pass.getText());
                pass.clear();
                return results;
            }
            pass.clear();
            return null;
        });
        return dialog;
    }

    private void showDialog(Dialog<Results> dialog) {
        var optionalResult = dialog.showAndWait();
        optionalResult.ifPresentOrElse(
            (var results) -> System.out.println(results),
            (this::resultsNotPresent));
    }

    private void resultsNotPresent() {
        System.out.println("Canceled on FX application thread: "
            + Platform.isFxApplicationThread());
    }

    public static void main(String[] args) {
        if (args.length > 0) {
            REUSE_DIALOG = args[0].startsWith("-r");
        }
        launch(args);
    }
}
trashgod
  • 203,806
  • 29
  • 246
  • 1,045
  • 1
    Interesting. Do you care to post the results you got? – SedJ601 Aug 12 '22 at 21:11
  • 2
    Usually, I can't resist, but the difference is hard to distinguish from background; _cf._ [this](https://stackoverflow.com/a/6310284/230513). Sadly, I really think it's tangential to what I think is an underlying problem. – trashgod Aug 12 '22 at 23:23