Skip to content

Commit 1be65e9

Browse files
joshsmithxrmclaude
joshsmithxrm
and
claude
committed
fix: add AuthenticationOutput for configurable auth messaging (#80)
Addresses issues #7, #9, #15, #27 from code review: - #7: Add thread-safety remarks to ConsoleProgressReporter - #9: Add documentation for role mapping limitation in TieredImporter - #15: Replace Console.WriteLine with AuthenticationOutput in auth library - New AuthenticationOutput class allows consumers to redirect or suppress output - Set AuthenticationOutput.Writer = null to suppress, or provide custom Action<string> - #27: Add validation in CredentialProviderFactory before null-forgiveness - ValidateRequiredFields checks GitHubFederated, AzureDevOpsFederated, UsernamePassword 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 parent 77def06 commit 1be65e9

File tree

7 files changed

+138
-34
lines changed

7 files changed

+138
-34
lines changed

‎src/PPDS.Auth/AuthenticationOutput.cs‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
using System;
2+
3+
namespace PPDS.Auth;
4+
5+
/// <summary>
6+
/// Controls authentication status output for the auth library.
7+
/// </summary>
8+
/// <remarks>
9+
/// By default, authentication status messages are written to the console.
10+
/// Library consumers who don't want console output can set <see cref="Writer"/> to null
11+
/// or provide a custom action to redirect output.
12+
/// </remarks>
13+
public static class AuthenticationOutput
14+
{
15+
private static Action<string>? _writer = Console.WriteLine;
16+
17+
/// <summary>
18+
/// Gets or sets the action used to write authentication status messages.
19+
/// Set to null to suppress all output, or provide a custom action to redirect.
20+
/// Default: Console.WriteLine
21+
/// </summary>
22+
/// <example>
23+
/// <code>
24+
/// // Suppress all auth output
25+
/// AuthenticationOutput.Writer = null;
26+
///
27+
/// // Redirect to a logger
28+
/// AuthenticationOutput.Writer = message => logger.LogInformation(message);
29+
/// </code>
30+
/// </example>
31+
public static Action<string>? Writer
32+
{
33+
get => _writer;
34+
set => _writer = value;
35+
}
36+
37+
/// <summary>
38+
/// Writes a status message using the configured writer.
39+
/// Does nothing if Writer is null.
40+
/// </summary>
41+
/// <param name="message">The message to write.</param>
42+
internal static void WriteLine(string message = "")
43+
{
44+
_writer?.Invoke(message);
45+
}
46+
47+
/// <summary>
48+
/// Resets the writer to the default (Console.WriteLine).
49+
/// </summary>
50+
public static void Reset()
51+
{
52+
_writer = Console.WriteLine;
53+
}
54+
}

‎src/PPDS.Auth/Credentials/CredentialProviderFactory.cs‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@ public static class CredentialProviderFactory
1414
/// <param name="profile">The auth profile.</param>
1515
/// <param name="deviceCodeCallback">Optional callback for device code display (for DeviceCode auth in headless mode).</param>
1616
/// <returns>A credential provider for the profile's auth method.</returns>
17+
/// <exception cref="ArgumentNullException">If profile is null.</exception>
18+
/// <exception cref="ArgumentException">If required profile fields are missing for the auth method.</exception>
1719
/// <exception cref="NotSupportedException">If the auth method is not supported.</exception>
1820
public static ICredentialProvider Create(
1921
AuthProfile profile,
@@ -22,6 +24,9 @@ public static ICredentialProvider Create(
2224
if (profile == null)
2325
throw new ArgumentNullException(nameof(profile));
2426

27+
// Validate required fields based on auth method
28+
ValidateRequiredFields(profile);
29+
2530
return profile.AuthMethod switch
2631
{
2732
AuthMethod.InteractiveBrowser => InteractiveBrowserCredentialProvider.FromProfile(profile),
@@ -40,6 +45,41 @@ public static ICredentialProvider Create(
4045
};
4146
}
4247

48+
/// <summary>
49+
/// Validates that required fields are present for the profile's auth method.
50+
/// </summary>
51+
private static void ValidateRequiredFields(AuthProfile profile)
52+
{
53+
switch (profile.AuthMethod)
54+
{
55+
case AuthMethod.GitHubFederated:
56+
case AuthMethod.AzureDevOpsFederated:
57+
RequireField(profile.ApplicationId, nameof(profile.ApplicationId), profile.AuthMethod);
58+
RequireField(profile.TenantId, nameof(profile.TenantId), profile.AuthMethod);
59+
break;
60+
61+
case AuthMethod.UsernamePassword:
62+
RequireField(profile.Username, nameof(profile.Username), profile.AuthMethod);
63+
RequireField(profile.Password, nameof(profile.Password), profile.AuthMethod);
64+
break;
65+
66+
// Other auth methods validate in their FromProfile methods
67+
}
68+
}
69+
70+
/// <summary>
71+
/// Throws if a required field is null or empty.
72+
/// </summary>
73+
private static void RequireField(string? value, string fieldName, AuthMethod authMethod)
74+
{
75+
if (string.IsNullOrWhiteSpace(value))
76+
{
77+
throw new ArgumentException(
78+
$"Profile field '{fieldName}' is required for {authMethod} authentication.",
79+
fieldName);
80+
}
81+
}
82+
4383
/// <summary>
4484
/// Creates the appropriate interactive provider based on environment.
4585
/// Uses browser authentication by default, falls back to device code for headless environments.

‎src/PPDS.Auth/Credentials/DeviceCodeCredentialProvider.cs‎

Lines changed: 11 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -184,19 +184,15 @@ private async Task<string> GetTokenAsync(string environmentUrl, bool forceIntera
184184
}
185185
else
186186
{
187-
// Default console output
188-
Console.WriteLine();
189-
Console.WriteLine("To sign in, use a web browser to open the page:");
190-
Console.ForegroundColor = ConsoleColor.Cyan;
191-
Console.WriteLine($" {deviceCodeResult.VerificationUrl}");
192-
Console.ResetColor();
193-
Console.WriteLine();
194-
Console.WriteLine("Enter the code:");
195-
Console.ForegroundColor = ConsoleColor.Yellow;
196-
Console.WriteLine($" {deviceCodeResult.UserCode}");
197-
Console.ResetColor();
198-
Console.WriteLine();
199-
Console.WriteLine("Waiting for authentication...");
187+
// Default output via AuthenticationOutput (can be redirected or suppressed)
188+
AuthenticationOutput.WriteLine();
189+
AuthenticationOutput.WriteLine("To sign in, use a web browser to open the page:");
190+
AuthenticationOutput.WriteLine($" {deviceCodeResult.VerificationUrl}");
191+
AuthenticationOutput.WriteLine();
192+
AuthenticationOutput.WriteLine("Enter the code:");
193+
AuthenticationOutput.WriteLine($" {deviceCodeResult.UserCode}");
194+
AuthenticationOutput.WriteLine();
195+
AuthenticationOutput.WriteLine("Waiting for authentication...");
200196
}
201197
return Task.CompletedTask;
202198
})
@@ -205,8 +201,8 @@ private async Task<string> GetTokenAsync(string environmentUrl, bool forceIntera
205201

206202
if (_deviceCodeCallback == null)
207203
{
208-
Console.WriteLine($"Authenticated as: {_cachedResult.Account.Username}");
209-
Console.WriteLine();
204+
AuthenticationOutput.WriteLine($"Authenticated as: {_cachedResult.Account.Username}");
205+
AuthenticationOutput.WriteLine();
210206
}
211207

212208
return _cachedResult.AccessToken;

‎src/PPDS.Auth/Credentials/InteractiveBrowserCredentialProvider.cs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -205,8 +205,8 @@ private async Task<string> GetTokenAsync(string environmentUrl, bool forceIntera
205205
}
206206

207207
// Interactive browser authentication
208-
Console.WriteLine();
209-
Console.WriteLine("Opening browser for authentication...");
208+
AuthenticationOutput.WriteLine();
209+
AuthenticationOutput.WriteLine("Opening browser for authentication...");
210210

211211
try
212212
{
@@ -222,8 +222,8 @@ private async Task<string> GetTokenAsync(string environmentUrl, bool forceIntera
222222
throw new OperationCanceledException("Authentication was canceled by the user.", ex);
223223
}
224224

225-
Console.WriteLine($"Authenticated as: {_cachedResult.Account.Username}");
226-
Console.WriteLine();
225+
AuthenticationOutput.WriteLine($"Authenticated as: {_cachedResult.Account.Username}");
226+
AuthenticationOutput.WriteLine();
227227

228228
return _cachedResult.AccessToken;
229229
}

‎src/PPDS.Auth/Discovery/GlobalDiscoveryService.cs‎

Lines changed: 11 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -168,7 +168,7 @@ private Func<string, Task<string>> CreateTokenProviderFunction(
168168
{
169169
if (_deviceCodeCallback == null)
170170
{
171-
Console.WriteLine("Opening browser for authentication...");
171+
AuthenticationOutput.WriteLine("Opening browser for authentication...");
172172
}
173173

174174
result = await _msalClient!
@@ -192,18 +192,14 @@ private Func<string, Task<string>> CreateTokenProviderFunction(
192192
}
193193
else
194194
{
195-
Console.WriteLine();
196-
Console.WriteLine("To sign in, use a web browser to open the page:");
197-
Console.ForegroundColor = ConsoleColor.Cyan;
198-
Console.WriteLine($" {deviceCodeResult.VerificationUrl}");
199-
Console.ResetColor();
200-
Console.WriteLine();
201-
Console.WriteLine("Enter the code:");
202-
Console.ForegroundColor = ConsoleColor.Yellow;
203-
Console.WriteLine($" {deviceCodeResult.UserCode}");
204-
Console.ResetColor();
205-
Console.WriteLine();
206-
Console.WriteLine("Waiting for authentication...");
195+
AuthenticationOutput.WriteLine();
196+
AuthenticationOutput.WriteLine("To sign in, use a web browser to open the page:");
197+
AuthenticationOutput.WriteLine($" {deviceCodeResult.VerificationUrl}");
198+
AuthenticationOutput.WriteLine();
199+
AuthenticationOutput.WriteLine("Enter the code:");
200+
AuthenticationOutput.WriteLine($" {deviceCodeResult.UserCode}");
201+
AuthenticationOutput.WriteLine();
202+
AuthenticationOutput.WriteLine("Waiting for authentication...");
207203
}
208204
return Task.CompletedTask;
209205
})
@@ -213,8 +209,8 @@ private Func<string, Task<string>> CreateTokenProviderFunction(
213209

214210
if (_deviceCodeCallback == null)
215211
{
216-
Console.WriteLine($"Authenticated as: {result.Account.Username}");
217-
Console.WriteLine();
212+
AuthenticationOutput.WriteLine($"Authenticated as: {result.Account.Username}");
213+
AuthenticationOutput.WriteLine();
218214
}
219215

220216
return result.AccessToken;

‎src/PPDS.Migration/Import/TieredImporter.cs‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -872,6 +872,20 @@ private async Task<Dictionary<string, Guid>> BuildRoleNameCacheAsync(Cancellatio
872872
return cache;
873873
}
874874

875+
/// <summary>
876+
/// Attempts to find a matching role in the target environment by ID.
877+
/// </summary>
878+
/// <remarks>
879+
/// <para>
880+
/// This method only succeeds when the source and target environments share the same role IDs
881+
/// (e.g., same tenant or restored from backup). For cross-environment migrations where role IDs
882+
/// differ, this returns null because we don't have the source role name to look up by name.
883+
/// </para>
884+
/// <para>
885+
/// Known limitation: Proper role mapping requires exporting role names alongside IDs in the
886+
/// migration schema. This is tracked for future enhancement.
887+
/// </para>
888+
/// </remarks>
875889
private async Task<Guid?> LookupRoleByIdAsync(
876890
Guid sourceRoleId,
877891
Dictionary<string, Guid> roleNameCache,

‎src/PPDS.Migration/Progress/ConsoleProgressReporter.cs‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,10 @@ namespace PPDS.Migration.Progress
99
/// <summary>
1010
/// Progress reporter that writes human-readable output to the console.
1111
/// </summary>
12+
/// <remarks>
13+
/// This class is not thread-safe. It is designed for single-threaded CLI usage
14+
/// where progress events are reported sequentially.
15+
/// </remarks>
1216
public class ConsoleProgressReporter : IProgressReporter
1317
{
1418
private const int MaxErrorsToDisplay = 10;

0 commit comments

Comments
 (0)