Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 35 additions & 1 deletion MSStore.API/Packaged/StorePackagedAPI.cs
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,27 @@ public class StorePackagedAPI : IStorePackagedAPI, IDisposable

public static TimeSpan DefaultSubmissionPollDelay { get; set; } = TimeSpan.FromSeconds(30);

/// <summary>
/// Initializes a new instance of the <see cref="StorePackagedAPI"/> class.
/// </summary>
/// <param name="configurations">An instance of ClientConfiguration that contains all parameters populated</param>
/// <param name="clientAssertionAuthentication">The async delegate that, once completed, provides the client assertion authentication token.</param>
/// <param name="devCenterUrl">The DevCenter URL used to make the API calls.</param>
/// <param name="devCenterScope">The Scope from DevCenter that will be used to request the access token.</param>
/// <param name="logger">ILogger for logs.</param>
public StorePackagedAPI(
StoreConfigurations configurations,
Func<Task<string>> clientAssertionAuthentication,
string? devCenterUrl,
string? devCenterScope,
ILogger? logger = null)
: this(configurations, devCenterUrl, devCenterScope, logger)
{
ClientAssertionAuthentication = clientAssertionAuthentication;
ClientSecret = null;
Certificate = null;
}

/// <summary>
/// Initializes a new instance of the <see cref="StorePackagedAPI"/> class.
/// </summary>
Expand All @@ -63,6 +84,7 @@ public StorePackagedAPI(
ILogger? logger = null)
: this(configurations, devCenterUrl, devCenterScope, logger)
{
ClientAssertionAuthentication = null;
ClientSecret = clientSecret;
Certificate = null;
}
Expand All @@ -83,6 +105,7 @@ public StorePackagedAPI(
ILogger? logger = null)
: this(configurations, devCenterUrl, devCenterScope, logger)
{
ClientAssertionAuthentication = null;
ClientSecret = null;
Certificate = certificate;
}
Expand Down Expand Up @@ -118,6 +141,7 @@ private StorePackagedAPI(

private ILogger? Logger { get; }

private Func<Task<string>>? ClientAssertionAuthentication { get; }
public string? ClientSecret { get; }
public X509Certificate2? Certificate { get; }
public string DevCenterUrl { get; set; }
Expand Down Expand Up @@ -145,7 +169,17 @@ public async Task InitAsync(HttpClient? httpClient = null, CancellationToken ct
// Get authorization token.
Logger?.LogInformation("Getting DevCenter authorization token");
Microsoft.Identity.Client.AuthenticationResult? devCenterAccessToken = null;
if (Certificate != null)
if (ClientAssertionAuthentication != null)
{
devCenterAccessToken = await SubmissionClient.GetClientCredentialAccessTokenAsync(
Config.TenantId!.Value.ToString(),
Config.ClientId!.Value.ToString(),
ClientAssertionAuthentication,
DevCenterScope,
Logger,
ct);
}
else if (Certificate != null)
{
devCenterAccessToken = await SubmissionClient.GetClientCredentialAccessTokenAsync(
Config.TenantId!.Value.ToString(),
Expand Down
36 changes: 35 additions & 1 deletion MSStore.API/StoreAPI.cs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,27 @@ public class StoreAPI : IStoreAPI, IDisposable

private SubmissionClient? _client;

/// <summary>
/// Initializes a new instance of the <see cref="StoreAPI"/> class.
/// </summary>
/// <param name="configurations">An instance of ClientConfiguration that contains all parameters populated</param>
/// <param name="clientAssertionAuthentication">The async delegate that, once completed, provides the client assertion authentication token.</param>
/// <param name="serviceUrl">The Store API URL used to make the API calls.</param>
/// <param name="scope">The Scope from the Store APIs that will be used to request the access token.</param>
/// <param name="logger">ILogger for logs.</param>
public StoreAPI(
StoreConfigurations configurations,
Func<Task<string>> clientAssertionAuthentication,
string? serviceUrl,
string? scope,
ILogger? logger = null)
: this(configurations, serviceUrl, scope, logger)
{
ClientAssertionAuthentication = clientAssertionAuthentication;
ClientSecret = null;
Certificate = null;
}

/// <summary>
/// Initializes a new instance of the <see cref="StoreAPI"/> class.
/// </summary>
Expand All @@ -51,6 +72,7 @@ public StoreAPI(
ILogger? logger = null)
: this(configurations, serviceUrl, scope, logger)
{
ClientAssertionAuthentication = null;
ClientSecret = clientSecret;
Certificate = null;
}
Expand All @@ -71,6 +93,7 @@ public StoreAPI(
ILogger? logger = null)
: this(configurations, serviceUrl, scope, logger)
{
ClientAssertionAuthentication = null;
ClientSecret = null;
Certificate = certificate;
}
Expand Down Expand Up @@ -104,6 +127,7 @@ private StoreAPI(

private ILogger? Logger { get; }

private Func<Task<string>>? ClientAssertionAuthentication { get; }
public string? ClientSecret { get; }
public X509Certificate2? Certificate { get; }
public string ServiceUrl { get; set; }
Expand Down Expand Up @@ -131,7 +155,17 @@ public async Task InitAsync(HttpClient? httpClient = null, CancellationToken ct
// Get authorization token.
Logger?.LogInformation("Getting authorization token");
Microsoft.Identity.Client.AuthenticationResult? accessToken = null;
if (Certificate != null)
if (ClientAssertionAuthentication != null)
{
accessToken = await SubmissionClient.GetClientCredentialAccessTokenAsync(
Config.TenantId!.Value.ToString(),
Config.ClientId!.Value.ToString(),
ClientAssertionAuthentication,
Scope,
Logger,
ct);
}
else if (Certificate != null)
{
accessToken = await SubmissionClient.GetClientCredentialAccessTokenAsync(
Config.TenantId!.Value.ToString(),
Expand Down
25 changes: 25 additions & 0 deletions MSStore.API/SubmissionClient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,31 @@ protected virtual void Dispose(bool disposing)
}
}

/// <summary>
/// Gets the authorization token for the provided client id, client secret, and the scope.
/// This token is usually valid for 1 hour, so if your submission takes longer than that to complete,
/// make sure to get a new one periodically.
/// </summary>
/// <param name="tenantId">The tenantId used to get the access token, specific to your
/// Azure Active Directory app. Example: "d454d300-128e-2d81-334a-27d9b2baf002"</param>
/// <param name="clientId">Client Id of your Azure Active Directory app. Example: "ba3c223b-03ab-4a44-aa32-38aa10c27e32"</param>
/// <param name="clientAssertionAuthentication">The async delegate that, once completed, provides the client assertion authentication token.</param>
/// <param name="scope">Scope. If not provided, default one is used for the production API endpoint.</param>
/// <param name="logger">ILogger for logs.</param>
/// <param name="ct">Cancelation token.</param>
/// <returns>Autorization token. Prepend it with "Bearer: " and pass it in the request header as the
/// value for "Authorization: " header.</returns>
public static Task<AuthenticationResult> GetClientCredentialAccessTokenAsync(
string tenantId,
string clientId,
Func<Task<string>> clientAssertionAuthentication,
string scope,
ILogger? logger = null,
CancellationToken ct = default)
{
return GetClientCredentialAccessTokenAsync(tenantId, clientId, (builder) => builder.WithClientAssertion((AssertionRequestOptions _) => clientAssertionAuthentication()), scope, logger, ct);
}

/// <summary>
/// Gets the authorization token for the provided client id, client secret, and the scope.
/// This token is usually valid for 1 hour, so if your submission takes longer than that to complete,
Expand Down
23 changes: 23 additions & 0 deletions MSStore.CLI.UnitTests/ReconfigureCommandUnitTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -217,5 +217,28 @@ public async Task ReconfigureCommandWithAllInfoAndCertThumbprintShouldReturnZero

result.Error.Should().Contain("Awesome! It seems to be working!");
}

[TestMethod]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I'm not particularly sure how to approach this. AFAIK Environment.GetEnvironmentVariable isn't mockable directly?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

public async Task ReconfigureCommandWithAllInfoAndClientAssertionShouldReturnZero()
{
var result = await ParseAndInvokeAsync(
[
"reconfigure",
"--tenantId",
DefaultOrganization.Id!.Value.ToString(),
"--sellerId",
"12345",
"--clientId",
"3F0BCAEF-6334-48CF-837F-81CB0F1F2C45",
"--clientAssertion"
]);

TokenManager
.Verify(x => x.SelectAccountAsync(It.IsAny<bool>(), It.IsAny<bool>(), It.IsAny<CancellationToken>()), Times.Never);
TokenManager
.Verify(x => x.GetTokenAsync(It.IsAny<string[]>(), It.IsAny<CancellationToken>()), Times.Never);

result.Error.Should().Contain("Awesome! It seems to be working!");
}
}
}
13 changes: 12 additions & 1 deletion MSStore.CLI/Commands/ReconfigureCommand.cs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ internal class ReconfigureCommand : Command
private static readonly Option<string> CertificatePasswordOption;
private static readonly Option<bool> ResetOption;

private static readonly Option<bool> ClientAssertionOption;

static ReconfigureCommand()
{
TenantIdOption = new Option<Guid?>("--tenantId", "-t")
Expand Down Expand Up @@ -67,6 +69,11 @@ static ReconfigureCommand()
{
Description = "Only reset the credentials, without starting over."
};

ClientAssertionOption = new Option<bool>("--clientAssertion", "-ca")
Comment thread
dongle-the-gadget marked this conversation as resolved.
{
Description = "Use client assertion for authentication."
};
}

public ReconfigureCommand()
Expand All @@ -79,6 +86,7 @@ public ReconfigureCommand()
Options.Add(CertificateThumbprintOption);
Options.Add(CertificateFilePathOption);
Options.Add(CertificatePasswordOption);
Options.Add(ClientAssertionOption);
Options.Add(ResetOption);
}

Expand All @@ -97,14 +105,16 @@ public override async Task<int> InvokeAsync(ParseResult parseResult, Cancellatio
var certificateThumbprint = parseResult.GetValue(CertificateThumbprintOption);
var certificateFilePath = parseResult.GetValue(CertificateFilePathOption);
var certificatePassword = parseResult.GetValue(CertificatePasswordOption);
var clientAssertion = parseResult.GetValue(ClientAssertionOption);
var reset = parseResult.GetValue(ResetOption);

bool askConfirmation = tenantId == null ||
sellerId == null ||
clientId == null ||
(clientSecret == null &&
certificateThumbprint == null &&
certificateFilePath == null);
certificateFilePath == null &&
clientAssertion == false);

return await _telemetryClient.TrackCommandEventAsync<Handler>(
(reset == true
Expand All @@ -119,6 +129,7 @@ public override async Task<int> InvokeAsync(ParseResult parseResult, Cancellatio
certificateThumbprint: certificateThumbprint,
certificateFilePath: certificateFilePath?.FullName,
certificatePassword: certificatePassword,
clientAssertion: clientAssertion,
ct: ct)) ? 0 : -1,
new Dictionary<string, string>
{
Expand Down
18 changes: 18 additions & 0 deletions MSStore.CLI/MicrosoftStoreCLI.cs
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,24 @@ internal static async Task<bool> InitAsync(IAnsiConsole ansiConsole, IConfigurat
return false;
}

if (config.ClientAssertion)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

{
try
{
await EnvironmentInfo.GetClientAssertionAsync();
return true;
}
catch (Exception ex)
{
if (ex is InvalidOperationException)
{
ansiConsole.MarkupLine(ex.Message);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

}
logger.LogCritical(ex, "Failed to get client assertion.");
Comment thread
dongle-the-gadget marked this conversation as resolved.
}
return false;
}

var secret = credentialManager.ReadCredential(config.ClientId.Value.ToString());
if (string.IsNullOrEmpty(config.CertificateFilePath)
&& string.IsNullOrEmpty(config.CertificateThumbprint)
Expand Down
17 changes: 15 additions & 2 deletions MSStore.CLI/Services/CLIConfigurator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ internal class CLIConfigurator(
private readonly ITokenManager _tokenManager = tokenManager ?? throw new ArgumentNullException(nameof(tokenManager));
private readonly ILogger _logger = logger ?? throw new ArgumentNullException(nameof(logger));

public async Task<bool> ConfigureAsync(IAnsiConsole ansiConsole, bool askConfirmation, Guid? tenantId = null, string? sellerId = null, Guid? clientId = null, string? clientSecret = null, string? certificateThumbprint = null, string? certificateFilePath = null, string? certificatePassword = null, CancellationToken ct = default)
public async Task<bool> ConfigureAsync(IAnsiConsole ansiConsole, bool askConfirmation, Guid? tenantId = null, string? sellerId = null, Guid? clientId = null, string? clientSecret = null, string? certificateThumbprint = null, string? certificateFilePath = null, string? certificatePassword = null, bool clientAssertion = false, CancellationToken ct = default)
{
if (askConfirmation &&
!await _consoleReader.YesNoConfirmationAsync(
Expand Down Expand Up @@ -80,6 +80,11 @@ public async Task<bool> ConfigureAsync(IAnsiConsole ansiConsole, bool askConfirm
config.ClientId = clientId;
}

if (clientAssertion)
{
config.ClientAssertion = true;
}

if (certificateThumbprint != null)
{
config.CertificateThumbprint = certificateThumbprint;
Expand All @@ -92,7 +97,8 @@ public async Task<bool> ConfigureAsync(IAnsiConsole ansiConsole, bool askConfirm

if (config.ClientId == null || (clientSecret == null &&
config.CertificateThumbprint == null &&
config.CertificateFilePath == null))
config.CertificateFilePath == null &&
config.ClientAssertion == false))
Comment thread
dongle-the-gadget marked this conversation as resolved.
{
string GetDisplayName(string sufix) => $"MSStoreCLIAccess - {sufix}";
string RandomString() => Path.GetFileNameWithoutExtension(Path.GetRandomFileName());
Expand Down Expand Up @@ -324,6 +330,13 @@ public async Task<bool> ConfigureAsync(IAnsiConsole ansiConsole, bool askConfirm
catch (Exception ex)
{
_logger.LogInformation(ex, "Error while creating StoreAPI.");

if (ex is MSStoreException && config.ClientAssertion && ex.InnerException is InvalidOperationException ioe)
{
ctx.ErrorStatus(ansiConsole, ioe.Message);
return false;
}

if (i + 1 == maxRetry)
{
break;
Expand Down
3 changes: 3 additions & 0 deletions MSStore.CLI/Services/Configurations.cs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ internal class Configurations
[JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
public string? PublisherDisplayName { get; set; }

[JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingDefault)]
public bool ClientAssertion { get; set; }

public StoreConfigurations GetStoreConfigurations() => new()
{
SellerId = SellerId,
Expand Down
35 changes: 35 additions & 0 deletions MSStore.CLI/Services/EnvironmentInfo.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using System.Collections.Generic;
using System.Threading.Tasks;

namespace MSStore.CLI.Services
{
Expand All @@ -21,6 +22,12 @@ internal class EnvironmentInfo
"CLI" // Running inside the CLI
};

// Environment variable for client assertion, used for authentication.
public static readonly string ClientAssertionEnvironmentVariable = "MSSTORE_CLIENT_ASSERTION";

// Environment variable for client assertion file path, used for authentication.
public static readonly string ClientAssertionFileEnvironmentVariable = "MSSTORE_CLIENT_ASSERTION_FILE";

// Cached environment information, loaded only once
private static readonly Lazy<string> _cachedEnvironmentInfo = new Lazy<string>(ComputeEnvironmentInfo);

Expand All @@ -38,6 +45,34 @@ public static string GetEnvironmentInfo()
return _cachedEnvironmentInfo.Value;
}

/// <summary>
/// Gets the client assertion from the environment variable.
/// </summary>
/// <exception cref="InvalidOperationException">Thrown if neither the client assertion environment variable nor the client assertion file environment variable is set, or if both are set.</exception>
/// <returns>The client assertion string.</returns>
public static async Task<string> GetClientAssertionAsync()
{
string? file = Environment.GetEnvironmentVariable(ClientAssertionFileEnvironmentVariable);
string? value = Environment.GetEnvironmentVariable(ClientAssertionEnvironmentVariable);

if (string.IsNullOrEmpty(file) && string.IsNullOrEmpty(value))
{
throw new InvalidOperationException($"Client assertion is configured, but neither {ClientAssertionEnvironmentVariable} nor {ClientAssertionFileEnvironmentVariable} environment variables are set.");
}

if (!string.IsNullOrEmpty(file) && !string.IsNullOrEmpty(value))
{
throw new InvalidOperationException($"Both {ClientAssertionEnvironmentVariable} and {ClientAssertionFileEnvironmentVariable} environment variables are set. Please specify only one");
}

if (!string.IsNullOrEmpty(file))
{
return (await System.IO.File.ReadAllTextAsync(file)).Trim();
}

return value!;
}

/// <summary>
/// Computes the environment information by detecting CI/CD environment variables.
/// This method is called only once when the lazy value is first accessed.
Expand Down
Loading