Validate reconfigure before mutating the credential store

[Alexandre Zollinger Chohfi (azchohfi)]

Fixes #166

The problem

CLIConfigurator.ConfigureAsync mutated the OS credential store before it validated the new configuration, and only wrote settings.json on success:

Order	Step
1	Write or clear the stored credential
2	_storeAPIFactory.CreateAsync(config, ct) — the first real validation
3	_configurationManager.SaveAsync(config, ct) — written only on success

The else branch in step 1 called ClearCredentials whenever neither clientSecret nor certificatePassword was supplied. So starting from a working client-secret configuration:

msstore reconfigure --clientAssertion    # same clientId, MSSTORE_CLIENT_ASSERTION not set

deleted the working secret, then failed validation, then never saved — leaving settings.json still in client-secret mode with the secret gone. Entra only reveals a client secret at creation time, so recovery meant minting a new one. --certificateThumbprint reached the same branch, so this was never client-assertion specific.

Why reordering alone doesn't work

Two constraints ruled out simply moving the block:

1. WriteCredential had to stay ahead of validation, because StoreAPIFactory called ReadCredential(clientId) to construct the client it validates with.
2. Deferring only the ClearCredentials branch leaves the previous secret in the store during validation. On the certificate-file path that value is passed straight through as the PKCS#12 password in StoreAPIFactory.LoadCertificate, so a password-less .pfx would silently be opened with the stale client secret — changing behaviour for a mode unrelated to the trigger.

Snapshot-and-restore was also rejected: it's a best-effort undo, and the window it has to cover is the retry loop, which sleeps 10s between attempts and prints "Retrying..." — exactly when a user is most likely to hit Ctrl+C.

The fix

Make the secret an explicit input to validation instead of something the factory goes and fetches.

IStoreAPIFactory gains CreateWithSecretAsync(config, secret, ct), where a null secret unambiguously means "this configuration has no secret" and never falls back to the credential store. CreateAsync now delegates to it after reading the stored credential, so all other call sites are untouched.

ConfigureAsync then:

1. computes the candidate secret (clientSecret ?? certificatePassword) without touching the store,
2. validates the candidate configuration with it,
3. and only once that succeeds: saves settings.json, then writes the new credential or removes the obsolete one.

Persistence goes first because both credential operations are irreversible — WriteCredential overwrites whatever was stored for that client ID, and a client secret cannot be read back from Entra. A saved configuration whose credential is missing is recoverable (the user still has the secret they just supplied); an overwritten secret is not. That yields one invariant: a reconfigure that does not succeed leaves the credential store exactly as it found it.

How this covers all three modes

Mode	Candidate secret	Validated with	On success
Client secret	the secret	the secret	write it
Certificate — file + password	the password	the password (PKCS#12)	write it
Certificate — file, no password	null	null — not the stale secret	clear obsolete
Certificate — thumbprint	null	null	clear obsolete
Client assertion	null	null (assertion comes from env)	clear obsolete

Verified credential removal

ClearCredentials cannot report failure on any platform — Windows wraps the delete in a bare catch { }, and the Linux and macOS implementations discard the native delete status. A credential surviving the clear is therefore silent, and for a password-less certificate file it's harmful: validation runs with a null PKCS#12 password, but every later command reads the leftover secret and hands it to LoadCertificate as the password. Reconfigure would report success for a configuration that doesn't work.

Rather than change the ICredentialManager contract and the error handling of three native code paths, TryClearCredentials reads the credential back after clearing and reports failure with an actionable message if anything remains. The write branch needs no read-back — WriteCredential already throws on failure on all three platforms.

reconfigure --reset had the same defect: it cleared without verifying, then wiped settings.json and reported success regardless. It now clears first and bails before discarding the settings if anything remains, since wiping the configuration while an unremovable credential lingers leaves the machine worse off than it started.

Tests

20 new tests across two files.

ReconfigureCredentialSafetyUnitTests swaps in a dictionary-backed in-memory credential store seeded with a working secret, so assertions look at real store state rather than a call log:

• The regression tests: a failed reconfigure via --clientAssertion, --certificateThumbprint, --clientSecret, and --certificateFilePath each leave the existing secret intact, with WriteCredential, ClearCredentials, and SaveAsync all never called.
• Persistence-failure tests: SaveAsync throwing on the write branch and on the clear branch both leave the store untouched.
• Clear-verification tests: a silently failed ClearCredentials must not report success; a successful one is confirmed by read-back rather than assumed.
• Reset tests (this path had no coverage at all before): a surviving credential blocks the settings wipe; the normal case clears everything.
• ReconfigureShouldValidateBeforeMutatingTheCredentialStore snapshots the store during validation and asserts it still held the old secret, and that ReadCredential is never called.
• PasswordLessCertificateFileReconfigureShouldNotValidateWithTheStaleSecret pins constraint 2 above.
• Success-path tests for each of the five rows in the table.

StoreAPIFactoryUnitTests covers the factory split directly (previously untested): CreateAsync reads the store, CreateWithSecretAsync never does.

Each fix was verified to actually catch its bug by temporarily restoring the old behaviour and confirming the relevant tests fail.

dotnet build MSStore.CLI.UnitTests\MSStore.CLI.UnitTests.csproj -v quiet --nologo
.\MSStore.CLI.UnitTests\bin\Debug\net10.0\MSStore.CLI.UnitTests.exe

• Baseline on main: 170 total, 160 succeeded, 10 skipped, 0 failed
• With this PR: 190 total, 180 succeeded, 10 skipped, 0 failed
• net10.0-windows10.0.17763.0: 190 total, 182 succeeded, 8 skipped, 0 failed
• Full solution builds with 0 warnings

Two incidental changes

CLIConfigurator.ValidationRetryDelay makes the retry back-off overridable, mirroring the existing StorePackagedAPI.DefaultSubmissionPollDelay seam, so failure-path tests don't sit through 3 attempts × 10s of real sleeps. It also fixes the retry message, which now prints the delay actually in use.

The shared ReadCredential test double in BaseCommandLineTest threw on UserNames.Last() once ClearCredentials emptied the lists. Every real implementation returns an empty string for a missing credential, so the fake now matches that contract.

[Copilot]

Pull request overview

Validates reconfiguration candidates without first mutating stored credentials.

Changes:

• Adds explicit-secret Store API creation.
• Defers credential updates until after validation.
• Adds credential-safety tests and configurable retry delay.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
MSStore.CLI/Services/StoreAPIFactory.cs	Adds explicit-secret API creation.
MSStore.CLI/Services/IStoreAPIFactory.cs	Defines the new factory contract.
MSStore.CLI/Services/CLIConfigurator.cs	Reorders validation and credential persistence.
MSStore.CLI.UnitTests/StoreAPIFactoryUnitTests.cs	Tests factory credential-store behavior.
MSStore.CLI.UnitTests/ReconfigureCredentialSafetyUnitTests.cs	Tests reconfiguration credential safety.
MSStore.CLI.UnitTests/BaseCommandLineTest.cs	Configures new mocks and retry seam.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (1)

MSStore.CLI/Services/CLIConfigurator.cs:383

• The suggested --reset recovery is unreliable in the exact failure case detected here. ResetAsync calls the same best-effort ClearCredentials operation without verifying its result (CLIConfigurator.cs:675-680), then clears the saved settings and can report success while the obsolete credential remains. Remove this recommendation, or make reset verify deletion before advertising it as a remedy.

                            ctx.ErrorStatus(ansiConsole, $"The configuration was saved, but the obsolete credential for '{clientIdString}' could not be removed from the credential store. Remove it manually, or run 'msstore reconfigure --reset', before using the CLI.");

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.