diff --git a/README.md b/README.md index 428afa7d..5cc2b68e 100644 --- a/README.md +++ b/README.md @@ -1701,9 +1701,17 @@ Finally, choose what its sessions are visible to, **on the machine itself**: kcap config set default_visibility org_public ``` -Visibility is the machine's own setting, exactly as it is for a person. The -`--visibility` flag on `create` only selects the value printed in the instructions — -it does not configure the runner for you. +Visibility is the machine's own setting, exactly as it is for a person — a machine +records with the `default_visibility` of the profile it runs under, and with no +profile it records `org_public`. It is **not** steered to private. The `--visibility` +flag on `create` only selects the value printed in the setup instructions (defaulting +to your own profile's default, so `create` shows you the value your machine will +actually use) — it does not configure the runner for you. + +> **Heads up:** `KCAP_CLIENT_ID`/`KCAP_CLIENT_SECRET` in your environment divert **all** +> of this CLI's auth onto the machine credential, so those variables belong on a runner, +> not in an interactive shell. `kcap status` prints a line naming them when it detects +> them, so a shell that has accidentally inherited them is easy to spot. Revoking stops a machine authenticating from its next request. A token it already holds stays valid until it expires (up to an hour) but is no longer honoured. To cut diff --git a/src/Capacitor.Cli.Core/Auth/MachineAuth.cs b/src/Capacitor.Cli.Core/Auth/MachineAuth.cs index a3ccd4dd..d36578e9 100644 --- a/src/Capacitor.Cli.Core/Auth/MachineAuth.cs +++ b/src/Capacitor.Cli.Core/Auth/MachineAuth.cs @@ -121,4 +121,24 @@ public static class MachineAuth { return null; } + + /// + /// The one-line kcap status explanation of the auth diversion + /// causes. Returns null when machine auth is not in play (caller prints nothing). + /// + /// Distinguishes the two states (either-var) collapses, because they + /// are not the same to a reader: with BOTH variables present the CLI genuinely records as the + /// machine instead of the signed-in user; with only ONE the credential is incomplete, so + /// refuses it and NOTHING records — the diversion still happens (the token + /// store is bypassed), so kcap login is not the fix. Saying "records as the machine" in + /// the one-variable case, or letting the profile token-store line then advise kcap login, + /// would both be false. Names exactly which variable(s) are present so the message is truthful in + /// every case. + /// + public static string? DescribeDiversion(bool idSet, bool secretSet) => (idSet, secretSet) switch { + (false, false) => null, + (true, true) => $"machine credential ({ClientIdVar} and {ClientSecretVar} set) — kcap records as the machine, not as your login.", + (true, false) => $"machine credential incomplete — {ClientIdVar} is set but {ClientSecretVar} is not. Auth is diverted off your login and will fail until both are set.", + (false, true) => $"machine credential incomplete — {ClientSecretVar} is set but {ClientIdVar} is not. Auth is diverted off your login and will fail until both are set.", + }; } diff --git a/src/Capacitor.Cli.Core/Resources/help-machine.txt b/src/Capacitor.Cli.Core/Resources/help-machine.txt index 034f2742..db477770 100644 --- a/src/Capacitor.Cli.Core/Resources/help-machine.txt +++ b/src/Capacitor.Cli.Core/Resources/help-machine.txt @@ -29,9 +29,13 @@ THE SECRET IS SHOWN ONCE Flags for create: --visibility What the machine's sessions are visible to. One of: - private, org_public, public. Default: private. - Printed in the setup instructions — see below for why it - is set on the machine rather than here. + private, org_public, public. Defaults to your own + profile's default_visibility — or org_public if you have no + profile, or if your profile's value is one a machine can't + record with (e.g. project, which is member-only and a + machine is never a project member). A machine is not steered + to private. Printed in the setup instructions — see below + for why it is set on the machine rather than here. --role The machine's role in Capacitor. Default: member. A machine is never an administrator whatever you pass. diff --git a/src/Capacitor.Cli/Commands/MachineCommand.cs b/src/Capacitor.Cli/Commands/MachineCommand.cs index cc0caf2d..000d577e 100644 --- a/src/Capacitor.Cli/Commands/MachineCommand.cs +++ b/src/Capacitor.Cli/Commands/MachineCommand.cs @@ -4,6 +4,7 @@ using Capacitor.Cli.Core; using Capacitor.Cli.Core.Auth; using Capacitor.Cli.Core.Commands; +using Capacitor.Cli.Core.Config; namespace Capacitor.Cli.Commands; @@ -54,9 +55,15 @@ public static async Task HandleAsync(string baseUrl, string[] args) { static async Task CreateAsync(string baseUrl, string[] args) { if (args.Length < 3 || IsHelp(args[2])) return await PrintCreateUsage(); - var name = args[2].Trim(); - var visibility = GetArg(args, "--visibility") ?? "private"; - var role = GetArg(args, "--role"); + var name = args[2].Trim(); + // The visibility PRINTED in the setup instructions is resolved from the operator's own + // configuration, not steered to private: an explicit flag wins, else this machine records + // with whatever default_visibility the active profile carries (what `kcap setup` wrote), + // else the product default a profile-less runner would use anyway. + var profile = await AppConfig.GetActiveProfileAsync(); + var (visibility, visibilityProvenance) = + ResolveCreateVisibility(GetArg(args, "--visibility"), profile?.DefaultVisibility); + var role = GetArg(args, "--role"); if (string.IsNullOrWhiteSpace(name)) { await Console.Error.WriteLineAsync("A machine name is required."); @@ -198,7 +205,7 @@ await Console.Error.WriteLineAsync( return 1; } - await PrintSetupAsync(registered, provisioned, visibility); + await PrintSetupAsync(registered, provisioned, visibility, visibilityProvenance); return 0; } @@ -313,7 +320,8 @@ static async Task PrintSecretAsync(string name, string secret, CreateMachineAppl /// after — and a failure here costs instructions the help can repeat, not a secret. /// static async Task PrintSetupAsync( - RegisterMachineResponse registered, CreateMachineApplicationResponse provisioned, string visibility) { + RegisterMachineResponse registered, CreateMachineApplicationResponse provisioned, + string visibility, string visibilityProvenance) { await Console.Error.WriteLineAsync(); await Console.Error.WriteLineAsync($"Machine registered as {registered.UserId}. It can now record."); await Console.Error.WriteLineAsync(); @@ -322,7 +330,13 @@ static async Task PrintSetupAsync( await Console.Error.WriteLineAsync($" KCAP_CLIENT_ID={provisioned.ClientId}"); await Console.Error.WriteLineAsync(" KCAP_CLIENT_SECRET="); await Console.Error.WriteLineAsync(); - await Console.Error.WriteLineAsync($" And set what its sessions are visible to (default '{visibility}'):"); + // Describe, don't prescribe: the machine records with the default_visibility of the profile + // on the machine it runs on (a machine with no profile records org_public). The value below + // is labeled with where it came from — the flag, the operator's profile, or the product + // default — and never introduces 'private' on this command's own authority. + await Console.Error.WriteLineAsync( + $" Its sessions are visible per the profile it records with ({visibility} — {visibilityProvenance})."); + await Console.Error.WriteLineAsync(" To confirm or change that on the machine itself:"); await Console.Error.WriteLineAsync(); await Console.Error.WriteLineAsync($" kcap config set default_visibility {visibility}"); await Console.Error.WriteLineAsync(); @@ -332,6 +346,38 @@ await Console.Error.WriteLineAsync( " machine's own setting, exactly as it is for a person."); } + /// + /// The visibility PRINTED in the setup instructions, with a label for where it came from. An + /// explicit --visibility flag wins (validated by the caller — only the flag can be an + /// invalid value); otherwise the active profile's default_visibility when a machine can + /// actually record with it; otherwise the product default org_public, which is what a + /// profile-less runner records with anyway (the server treats an absent + /// default_visibility as org_public). + /// + /// A profile may legitimately carry project (a per-viewer, member-only audience) — + /// but a machine is never a project member, so that value is not one it can record with. Rather + /// than inherit it and then reject the whole create with a message that falsely blames a + /// --visibility flag the operator never passed, it falls back to the product default and + /// the provenance says why. So after this resolver the value is always in the machine-valid set + /// UNLESS it came from an explicit flag, which is the only case the caller's validation rejects. + /// + /// Deliberately never invents private: it appears only because the flag or the + /// operator's own profile chose it, and the provenance label then says which. Pure so it is + /// unit-tested directly, without the profile/HTTP machinery around it. + /// + internal static (string Value, string Provenance) ResolveCreateVisibility( + string? flagValue, string? profileDefault) { + if (flagValue is not null) return (flagValue, "from --visibility"); + + if (profileDefault is not null && Visibilities.Contains(profileDefault, StringComparer.Ordinal)) + return (profileDefault, "your profile default"); + + if (!string.IsNullOrEmpty(profileDefault)) + return ("org_public", $"product default; a machine cannot record with your profile's '{profileDefault}' visibility"); + + return ("org_public", "product default"); + } + // ── list ──────────────────────────────────────────────────────────────────────────────────── static async Task ListAsync(string baseUrl) { diff --git a/src/Capacitor.Cli/Commands/StatusCommand.cs b/src/Capacitor.Cli/Commands/StatusCommand.cs index aecab5eb..2f38d23e 100644 --- a/src/Capacitor.Cli/Commands/StatusCommand.cs +++ b/src/Capacitor.Cli/Commands/StatusCommand.cs @@ -38,24 +38,37 @@ public static async Task HandleAsync(string? baseUrl, string[] args) { } // Auth - Console.Write(" Auth: "); - var tokens = await TokenStore.GetValidTokensAsync(); + // A machine-credential diversion REPLACES the token-store line rather than appending to it: + // with KCAP_CLIENT_ID/KCAP_CLIENT_SECRET in the environment, MachineAuth.Intended bypasses + // the token store entirely, so its state is not what this CLI authenticates with — printing + // both would show a headless runner as "records as the machine" AND "not authenticated (run: + // kcap login)", contradictory and with irrelevant remediation. + var machineLine = MachineAuth.DescribeDiversion( + !string.IsNullOrWhiteSpace(Environment.GetEnvironmentVariable(MachineAuth.ClientIdVar)), + !string.IsNullOrWhiteSpace(Environment.GetEnvironmentVariable(MachineAuth.ClientSecretVar))); + + if (machineLine is not null) { + Console.WriteLine($" Auth: {machineLine}"); + } else { + Console.Write(" Auth: "); + var tokens = await TokenStore.GetValidTokensAsync(); - if (tokens is not null) { - var remaining = tokens.ExpiresAt - DateTimeOffset.UtcNow; + if (tokens is not null) { + var remaining = tokens.ExpiresAt - DateTimeOffset.UtcNow; - var expiryText = remaining.TotalHours > 1 - ? $"expires in {remaining.TotalHours:F0}h" - : $"expires in {remaining.TotalMinutes:F0}m"; - await Console.Out.WriteLineAsync($"{tokens.GitHubUsername} ({tokens.Provider}) ✓ token valid ({expiryText})"); - } else { - var rawTokens = await TokenStore.LoadAsync(); + var expiryText = remaining.TotalHours > 1 + ? $"expires in {remaining.TotalHours:F0}h" + : $"expires in {remaining.TotalMinutes:F0}m"; + await Console.Out.WriteLineAsync($"{tokens.GitHubUsername} ({tokens.Provider}) ✓ token valid ({expiryText})"); + } else { + var rawTokens = await TokenStore.LoadAsync(); - await Console.Out.WriteLineAsync( - rawTokens is not null - ? $"{rawTokens.GitHubUsername} ({rawTokens.Provider}) ✗ token expired (run: kcap login)" - : "not authenticated (run: kcap login)" - ); + await Console.Out.WriteLineAsync( + rawTokens is not null + ? $"{rawTokens.GitHubUsername} ({rawTokens.Provider}) ✗ token expired (run: kcap login)" + : "not authenticated (run: kcap login)" + ); + } } // Hooks diff --git a/test/Capacitor.Cli.Tests.Unit/MachineAuthStatusLineTests.cs b/test/Capacitor.Cli.Tests.Unit/MachineAuthStatusLineTests.cs new file mode 100644 index 00000000..a5f5e548 --- /dev/null +++ b/test/Capacitor.Cli.Tests.Unit/MachineAuthStatusLineTests.cs @@ -0,0 +1,39 @@ +using Capacitor.Cli.Core.Auth; + +namespace Capacitor.Cli.Tests.Unit; + +/// +/// `kcap status` warns when machine-credential environment variables are diverting this CLI's auth +/// off the profile token store. Because triggers on EITHER +/// variable but needs BOTH, the message distinguishes the two: +/// both set means the CLI records as the machine; one set means the credential is incomplete and +/// nothing records (the diversion still bypasses the token store, so `kcap login` is not the fix). +/// It names exactly the variable(s) present and says nothing when neither is set. +/// +public class MachineAuthStatusLineTests { + [Test] + public async Task Both_variables_present_says_it_records_as_the_machine() { + var line = MachineAuth.DescribeDiversion(idSet: true, secretSet: true); + await Assert.That(line).IsNotNull(); + await Assert.That(line!).Contains("KCAP_CLIENT_ID and KCAP_CLIENT_SECRET set"); + await Assert.That(line!).Contains("records as the machine, not as your login"); + } + + [Test] + [Arguments(true, false, "KCAP_CLIENT_ID is set but KCAP_CLIENT_SECRET is not")] + [Arguments(false, true, "KCAP_CLIENT_SECRET is set but KCAP_CLIENT_ID is not")] + public async Task One_variable_present_reports_an_incomplete_credential(bool id, bool secret, string expectedFragment) { + var line = MachineAuth.DescribeDiversion(id, secret); + await Assert.That(line).IsNotNull(); + await Assert.That(line!).Contains("incomplete"); + await Assert.That(line!).Contains(expectedFragment); + await Assert.That(line!).Contains("diverted"); + await Assert.That(line!).DoesNotContain("records as the machine") + .Because("an incomplete credential does not record — TryRead refuses it"); + } + + [Test] + public async Task Silent_when_neither_variable_is_present() { + await Assert.That(MachineAuth.DescribeDiversion(false, false)).IsNull(); + } +} diff --git a/test/Capacitor.Cli.Tests.Unit/MachineCreateVisibilityTests.cs b/test/Capacitor.Cli.Tests.Unit/MachineCreateVisibilityTests.cs new file mode 100644 index 00000000..d133b619 --- /dev/null +++ b/test/Capacitor.Cli.Tests.Unit/MachineCreateVisibilityTests.cs @@ -0,0 +1,54 @@ +using Capacitor.Cli.Commands; + +namespace Capacitor.Cli.Tests.Unit; + +/// +/// `kcap machine create` resolves the visibility PRINTED in its setup instructions from the +/// operator's own configuration instead of steering to private: an explicit --visibility flag wins, +/// else the active profile's default_visibility (what `kcap setup` wrote), else the product default +/// org_public for a machine with no profile. Each carries a provenance label so the printed +/// `kcap config set default_visibility ...` line says where its value came from — and `private` +/// only ever appears because the flag or the operator's own profile chose it, never as this +/// command's own suggestion. +/// +public class MachineCreateVisibilityTests { + [Test] + public async Task Flag_wins_and_is_labeled_as_flag() { + var (value, provenance) = MachineCommand.ResolveCreateVisibility("private", "org_public"); + await Assert.That(value).IsEqualTo("private"); + await Assert.That(provenance).IsEqualTo("from --visibility"); + } + + [Test] + public async Task Profile_default_is_inherited_and_labeled() { + var (value, provenance) = MachineCommand.ResolveCreateVisibility(null, "org_public"); + await Assert.That(value).IsEqualTo("org_public"); + await Assert.That(provenance).IsEqualTo("your profile default"); + } + + [Test] + public async Task Private_profile_default_is_honored_not_overridden() { + var (value, provenance) = MachineCommand.ResolveCreateVisibility(null, "private"); + await Assert.That(value).IsEqualTo("private"); + await Assert.That(provenance).IsEqualTo("your profile default"); + } + + [Test] + public async Task No_profile_falls_back_to_product_default() { + var (value, provenance) = MachineCommand.ResolveCreateVisibility(null, null); + await Assert.That(value).IsEqualTo("org_public"); + await Assert.That(provenance).IsEqualTo("product default"); + } + + [Test] + public async Task Project_profile_default_a_machine_cannot_use_falls_back_without_erroring() { + // 'project' is a valid PROFILE default_visibility (a per-viewer, member-only audience) but a + // machine is never a project member, so it can't record with it. It must fall back to the + // product default rather than inherit a value the create-time validation would then reject + // with a message that falsely blames a --visibility flag the operator never passed. + var (value, provenance) = MachineCommand.ResolveCreateVisibility(null, "project"); + await Assert.That(value).IsEqualTo("org_public"); + await Assert.That(provenance).Contains("project") + .Because("the operator should see why their profile's value was not used"); + } +}