Add client assertion authentication - #145
Conversation
| Description = "Specify the client Secret that should be used." | ||
| }; | ||
|
|
||
| ClientAssertionOption = new Option<string>("--clientAssertion", "-ca") |
There was a problem hiding this comment.
Could we get a --clientAssertion case in ReconfigureCommandUnitTests alongside the existing --clientSecret one? Good to have the new option covered end to end.
2f55cfa to
28257f6
Compare
Alexandre Zollinger Chohfi (azchohfi)
left a comment
There was a problem hiding this comment.
This is a big improvement — thanks for taking it on. Storing just the flag, supporting both the variable and a file, checking the assertion branch before the certificate, and short-circuiting the retry loop all look right to me. Three small things left, then I think we're there.
| return false; | ||
| } | ||
|
|
||
| if (config.ClientAssertion) |
There was a problem hiding this comment.
This does the right thing in not sending assertion users into StartOverAsync — thanks for that.
The catch is that it now skips the check entirely, so for publish a missing variable isn't caught here. It surfaces from inside the MSAL call as MSStoreException("Could not retrieve access token"), and the actual reason goes through LogError, which is filtered out at our default level unless you pass --verbose. So the message you wrote in GetClientAssertionAsync only reaches the user on the reconfigure path, via the special case you added in CLIConfigurator.
Could we do the presence check here instead of returning true unconditionally? It's the natural pre-flight spot, and returning false already gives a clean non-zero exit with no prompt.
There was a problem hiding this comment.
Pull request overview
Adds client assertion authentication across the CLI and Store APIs, including environment/file-based assertion retrieval.
Changes:
- Adds client assertion configuration and CLI support.
- Adds MSAL client assertion authentication overloads.
- Adds assertion handling for packaged and unpackaged APIs.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
MSStore.CLI/Services/StoreAPIFactory.cs |
Selects assertion authentication. |
MSStore.CLI/Services/ICLIConfigurator.cs |
Extends configuration contract. |
MSStore.CLI/Services/EnvironmentInfo.cs |
Reads assertions from environment or file. |
MSStore.CLI/Services/Configurations.cs |
Persists authentication mode. |
MSStore.CLI/Services/CLIConfigurator.cs |
Configures and validates assertion authentication. |
MSStore.CLI/MicrosoftStoreCLI.cs |
Recognizes assertion-based configuration. |
MSStore.CLI/Commands/ReconfigureCommand.cs |
Adds the client assertion option. |
MSStore.API/SubmissionClient.cs |
Adds MSAL assertion token acquisition. |
MSStore.API/StoreAPI.cs |
Supports assertions for Store API access. |
MSStore.API/Packaged/StorePackagedAPI.cs |
Supports assertions for Dev Center access. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Alexandre Zollinger Chohfi (azchohfi)
left a comment
There was a problem hiding this comment.
All three from last round look good — thanks. The Func<Task<string>> change is exactly right, and the trim and the pre-flight check both do what I was after. One thing got lost in the last commit though, plus two small ones.
| result.Error.Should().Contain("Awesome! It seems to be working!"); | ||
| } | ||
|
|
||
| [TestMethod] |
There was a problem hiding this comment.
Thanks for adding this. Worth knowing its reach though — FakeStoreAPIFactory is mocked in BaseCommandLineTest.cs:194, so this covers the flag making it into the config but never actually resolves an assertion.
If you feel like it, a few direct tests on GetClientAssertionAsync would cover the parts most likely to break: neither variable set, both set, and the file case trimming its trailing newline. Not a blocker.
There was a problem hiding this comment.
I'm not particularly sure how to approach this. AFAIK Environment.GetEnvironmentVariable isn't mockable directly?
There was a problem hiding this comment.
You're right that Environment.GetEnvironmentVariable isn't mockable — but I don't think you need to mock it. Setting the real variable in the test works here, because Usings.cs:8 already has [assembly: DoNotParallelize], so tests won't stomp on each other.
Clear the variables in both [TestInitialize] and [TestCleanup] — init matters as much as cleanup, since whoever runs the suite may already have them set in their shell:
[TestInitialize]
public void Init() => ClearAssertionVars();
[TestCleanup]
public void Cleanup() => ClearAssertionVars();
private static void ClearAssertionVars()
{
Environment.SetEnvironmentVariable("MSSTORE_CLIENT_ASSERTION", null);
Environment.SetEnvironmentVariable("MSSTORE_CLIENT_ASSERTION_FILE", null);
}Then four straightforward cases: neither set throws, both set throws, the variable is returned as-is, and a file with a trailing newline comes back trimmed. I sketched these out and they pass. Still not a blocker if you'd rather leave it.
Alexandre Zollinger Chohfi (azchohfi)
left a comment
There was a problem hiding this comment.
All three fixed — thanks. One follow-on from the error-reporting change, and an answer to your testing question.
| { | ||
| if (ex is InvalidOperationException) | ||
| { | ||
| ansiConsole.MarkupLine(ex.Message); |
There was a problem hiding this comment.
This needs EscapeMarkup() — ansiConsole.MarkupLine(ex.Message.EscapeMarkup()). The message interpolates the file path from MSSTORE_CLIENT_ASSERTION_FILE, so a path containing [ gets parsed as markup:
MarkupLine(msg) -> InvalidOperationException: Could not find color or style 'build'
MarkupLine(msg.EscapeMarkup()) -> Could not read ... 'C:\tokens\[build]\tok.txt'
ErrorStatus already does this (StatusContextExtensions.cs:14), which is why the reconfigure path is fine. Would be a shame if a missing-token message turned into a crash.
Add support for MSAL Client Assertion authentication. Client Assertion authentication enables users to use GitHub Actions OpenID Connect, which is a more secure way to authenticate to Microsoft services without requiring client secrets.
The Client Assertion mechanism uses the
MSSTORE_CLIENT_ASSERTION/MSSTORE_CLIENT_ASSERTION_FILEenvironment variables, and is activated bymsstore reconfigure --clientAssertion.