Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support different PINs, forced batch signing and live settings updates #342

Merged
merged 13 commits into from
Nov 30, 2023

Conversation

jsuchal
Copy link
Member

@jsuchal jsuchal commented Nov 28, 2023

Toto vyzera pomerne tucne, ale az tak velmi sa tam v skutocnosti nezmenilo.

Hlavne kuzla sa deju v NativePkcs11SignatureToken a do konstruktorov vchadzaju nove veci ako PasswordManager, ktory cachuje ak to ma v settings.

Viac komentov ku kodu je napisanych priamo kde to je relevantne.

@jsuchal jsuchal self-assigned this Nov 28, 2023
this.driverDetector = driverDetector;
this.slotId = slotId;
this.shouldDisplayVisualizationError = shouldDisplayVisualizationError;
this.tspSource = tspSource;
this.passwordManager = new PasswordManager(ui, this.settings);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tu uz password manager dostava svoje settings. Aby to boli "uzke" rozhrania, tak dostava ako parameter interface PasswordManagerSettings, kde su len pre neho relevantne nastavenia (ziadne settery). Ocakavanie je, ze "horny" settings bude implementovat vsetky potrebne "slize" settings a posielat ich dole ako cely settings.

package digital.slovensko.autogram.core;

public interface PasswordManagerSettings {
boolean cacheContextSpecificPasswordEnabled();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Toto je to "uzke" rozhranie.

Comment on lines 8 to 18
return true; // TODO
}

@Override
public boolean forceContextSpecificLoginEnabled() {
return true; // TODO
}


public int getSlotId() {
return slotId;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tu budu vsetky mozne nastavovacky - malo by to vo finale byt vlastne existujuce UserSettings s tym, ze sa to moze v case menit (tam su tie nastavenia z nejakeho dovodu immutable).

@@ -57,22 +59,28 @@ private void runContextSpecificLoginIfNeeded(Signature signature, PrivateKey pk)
// TODO cache & short-circuit
var p11 = getP11(signature);
var sessionId = getSessionId(signature);
var keyID = getKeyID(pk);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Toto som strcil do isAlwaysAuthenticate aby to zbytocne nevolalo nejaku pomalu kartovu operaciu ak tam je ten force parameter true (co casto bude).

} catch (PKCS11Exception e) {
if (e.getMessage().equals("CKR_PIN_INCORRECT"))
throw new KeyPinDifferentFromTokenPinException(e);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Toto uplne zmizlo.

Comment on lines 259 to 266
@Override
public char[] getKeystorePassword() {
return System.console().readPassword("Enter keystore password (hidden): ");
}

public char[] getContextSpecificPassword() {
return System.console().readPassword("Enter key password (hidden): ");
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@celuchmarek toto treba vyskusat ci zafunguje

Comment on lines 219 to 240
var futurePassword = new FutureTask<>(() -> {
var controller = new PasswordController("Aký je kód k úložisku klúčov?", "Zadajte kód k úložisku klúčov.", false);
var root = GUIUtils.loadFXML(controller, "password-dialog.fxml");

var stage = new Stage();
stage.setTitle("Načítanie klúčov z úložiska");
stage.setScene(new Scene(root));
stage.setOnCloseRequest(e -> {
refreshKeyOnAllJobs();
enableSigningOnAllJobs();
});
stage.setResizable(false);
stage.initModality(Modality.APPLICATION_MODAL);
stage.showAndWait();

return controller.getPassword();
});

Platform.runLater(futurePassword);

try {
return futurePassword.get();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Toto je to najvacsie kuzlo. Vyrobi to nejaky "async" task, ktory vsak spravi ako blokujuci cez stage.showAndWait, ktory caka az kym sa to okno zatvori. To okno nastavuje password field na controlleri, ktore si potom vytiahnem a vratim z future.

future sa vsak automaticky nespustale ale zavola sa cez platform.runlater (na inom vlakne niekedy neskor). nasledne futurepassword.get je blokujuca operacia az kym sa nevykona ten future (v tomto pripade ked zadas heslo alebo zavries okno)

}


public char[] getContextSpecificPassword() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To iste pre ctx specific password len ine nastavenie dialogu.

@@ -33,10 +58,19 @@ public void onPasswordAction() {
formGroup.getScene().getWindow().sizeToScene();
passwordField.requestFocus();
} else {
this.password = passwordField.getText().toCharArray();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tu by sa patrilo tiez zmaznut ten text.

@jsuchal jsuchal changed the title Add working different pin handling with hardcoded settings Support different PINs, forced batch signing and live settings updates Nov 28, 2023
() -> Collections.singletonList(params.getDriver())
: new DefaultDriverDetector("", false),
params.getSlotId(), params.getTspSource());
var settings = CliSettings.fromCmd(cmd);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Toto som dost prekopal, ale neviem to uplne spustit 😅 cize toto by si mohol skusit pretestovat? @celuchmarek

V podstate som uplne vyrazil tie CliParameters a vyrobil len CliSettings, ktora cosi naviac dava oproti beznemu UserSettings.

private boolean shouldMakeParentDirectories;

public static CliSettings fromCmd(CommandLine cmd) {
var settings = new CliSettings();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do velkej miery len copy&paste a vyuziva to nastavovanie settings

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Toto som si predstavoval trochu inak. Teraz máme UserSettings nad týmto, ktoré majú aj nejaké save a load metódy a môžu mať aj iné parametre. Ja by som radšej spravil interface/abstract class AutogramSettings a ten by bol implementovaný v UserSettings aj CLISettings osobitne. A môže ostať, že interface/abstract class AutogramSettings by implementovali tie tri nové interfaces, čo si vyrobil.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ano, to bol dalsi krok, ktory som uz vcera nezvladol vecer lebo tie settings su zrele na upratovanie lebo aj nazvy su tam vselijake halabala is/has/should/_Enabled

@jsuchal jsuchal marked this pull request as ready for review November 28, 2023 23:36
@celuchmarek celuchmarek self-requested a review November 30, 2023 17:23
@jsuchal jsuchal merged commit ec54852 into slovensko-digital:main Nov 30, 2023
3 checks passed
@jsuchal jsuchal deleted the support-different-pins branch November 30, 2023 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants