diff --git a/cmd/volcano/grant_test.go b/cmd/volcano/grant_test.go new file mode 100644 index 00000000..335a3a5e --- /dev/null +++ b/cmd/volcano/grant_test.go @@ -0,0 +1,172 @@ +package main + +import ( + "bytes" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "sync" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/Kong/volcano-cli/internal/api" + rootcmd "github.com/Kong/volcano-cli/internal/cmd/root" + cliconfig "github.com/Kong/volcano-cli/internal/config" + cliruntime "github.com/Kong/volcano-cli/internal/runtime" +) + +// testGrant is a delegated grant as a worker gets it. Every test here checks +// that it never shows up in the output. +const testGrant = cliconfig.DelegatedGrantPrefix + "grant-secret-value" + +// grantRun is one command run the way a Builder worker runs the CLI. +type grantRun struct { + code int + stdout string + stderr string + requests []string + auth []string +} + +// runAsWorker runs the real command tree with the worker's environment: the +// grant in VOLCANO_TOKEN, the API in VOLCANO_API_URL, the project from the +// directory's link file, telemetry off and no prompts. No config loader is +// injected, so the CLI reads its configuration the way it does in the VM. +func runAsWorker(t *testing.T, handler http.HandlerFunc, args ...string) grantRun { + t.Helper() + resetInstructions(t) + api.ResetLastRefusalForTest() + t.Cleanup(api.ResetLastRefusalForTest) + + var mu sync.Mutex + result := grantRun{} + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + result.requests = append(result.requests, r.Method+" "+r.URL.Path) + result.auth = append(result.auth, r.Header.Get("Authorization")) + mu.Unlock() + handler(w, r) + })) + t.Cleanup(server.Close) + + home := t.TempDir() + workspace := filepath.Join(home, "workspace", "app") + require.NoError(t, os.MkdirAll(filepath.Join(workspace, ".volcano"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(workspace, ".volcano", "project.json"), + []byte(`{"project_id":"`+mainTestProjectAlpha+`"}`), 0o644)) + t.Setenv("HOME", home) + t.Chdir(workspace) + t.Setenv("VOLCANO_TOKEN", testGrant) + t.Setenv("VOLCANO_API_URL", server.URL) + t.Setenv("VOLCANO_PROJECT_ID", "") + t.Setenv("VOLCANO_TELEMETRY_DISABLED", "1") + t.Setenv("VOLCANO_NONINTERACTIVE", "1") + + deps := cliruntime.Deps{HTTPClient: server.Client()} + root := rootcmd.New(deps) + var stdout, stderr bytes.Buffer + root.SetOut(&stdout) + root.SetErr(&stderr) + root.SetArgs(args) + result.code = run(root, deps) + result.stdout, result.stderr = stdout.String(), stderr.String() + + assert.NotContains(t, result.stdout, "grant-secret-value", "the grant was printed") + assert.NotContains(t, result.stderr, "grant-secret-value", "the grant was printed") + for _, request := range result.requests { + assert.NotContains(t, request, "/cli/telemetry", "the grant reached telemetry") + assert.NotContains(t, request, "/service-keys", "a grant must never cause a service key") + assert.NotContains(t, request, "/user", "a grant must never reach an account route") + } + return result +} + +// projectFunctionsPath is the path `cloud functions list` reads. +const projectFunctionsPath = "/projects/" + mainTestProjectAlpha + "/functions" + +func TestGrantCallsProjectRoutesWithTheGrant(t *testing.T) { + run := runAsWorker(t, func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == projectFunctionsPath { + respondExitTestJSON(w, http.StatusOK, `{"data":[],"has_more":false,"page":1,"limit":100,"total":0}`) + return + } + respondExitTestJSON(w, http.StatusForbidden, `{"error":"delegated grants cannot be used on this endpoint","code":"grant_route_not_allowed"}`) + }, "cloud", "functions", "list") + + assert.Equal(t, 0, run.code, run.stderr) + assert.Equal(t, []string{"GET " + projectFunctionsPath}, run.requests) + assert.Equal(t, []string{"Bearer " + testGrant}, run.auth) + assert.NotContains(t, run.stderr, "VOLCANO_PROJECT_ID is deprecated", "the link file chose the project") +} + +// Hosting's refusals keep their exit codes and messages. Only a spent budget +// gets its own code, because a new grant is the fix. +func TestGrantRefusalsExitCodes(t *testing.T) { + for _, tc := range []struct { + name string + status int + body string + code int + want string + }{ + { + "route not allowed", http.StatusForbidden, + `{"error":"delegated grants cannot be used on this endpoint","code":"grant_route_not_allowed"}`, + exitPermissionDenied, "delegated grants cannot be used on this endpoint", + }, + { + "permission missing", http.StatusForbidden, + `{"error":"the delegated grant does not carry the permission this endpoint needs","code":"grant_permission_missing"}`, + exitPermissionDenied, "the delegated grant does not carry the permission this endpoint needs", + }, + { + "budget exhausted", http.StatusTooManyRequests, + `{"error":"the delegated grant has no mutations left; ask the app for a new grant","code":"grant_budget_exhausted"}`, + exitGrantBudgetExhausted, "the delegated grant has no mutations left; ask the app for a new grant", + }, + // Any other 429, such as the per-grant rate limit, is not the budget. + { + "rate limited", http.StatusTooManyRequests, + `{"error":"rate limit exceeded","code":"rate_limit_exceeded"}`, exitFailure, "rate limit exceeded", + }, + { + "expired or revoked", http.StatusUnauthorized, + `{"error":"invalid or expired token"}`, exitNotAuthenticated, "invalid or expired token", + }, + } { + t.Run(tc.name, func(t *testing.T) { + run := runAsWorker(t, func(w http.ResponseWriter, _ *http.Request) { + respondExitTestJSON(w, tc.status, tc.body) + }, "cloud", "functions", "list") + + assert.Equal(t, tc.code, run.code, run.stderr) + assert.Contains(t, run.stderr, tc.want) + }) + } +} + +// A grant was an unknown prefix, so an account token, before the CLI knew it. +// Account commands now refuse it before any request, as they refuse a pt-. +func TestGrantIsRefusedOnAccountCommandsBeforeAnyRequest(t *testing.T) { + for _, args := range [][]string{ + {"projects", "list"}, + {"cloud", "functions", "list", "--project", "some-project-name"}, + } { + t.Run(strings.Join(args, " "), func(t *testing.T) { + run := runAsWorker(t, func(w http.ResponseWriter, r *http.Request) { + t.Errorf("an account call reached the API: %s %s", r.Method, r.URL.Path) + respondExitTestJSON(w, http.StatusForbidden, `{"error":"forbidden"}`) + }, args...) + + assert.Equal(t, exitPermissionDenied, run.code, run.stderr) + assert.Contains(t, run.stderr, "this command needs an account token") + assert.Contains(t, run.stderr, "the current credential is a delegated grant (pg-)") + assert.NotContains(t, run.stderr, "project access token") + assert.Empty(t, run.requests) + }) + } +} diff --git a/cmd/volcano/main.go b/cmd/volcano/main.go index 4ac048ee..8f8ef02d 100644 --- a/cmd/volcano/main.go +++ b/cmd/volcano/main.go @@ -37,6 +37,9 @@ const ( exitNoProject = 3 exitNotFound = 4 exitPermissionDenied = 5 + // exitGrantBudgetExhausted is the API's 429 grant_budget_exhausted: a + // delegated grant used all of its mutations, and a new grant is the fix. + exitGrantBudgetExhausted = 6 ) func main() { @@ -187,6 +190,8 @@ func printProjectEnvDeprecation(w io.Writer, used bool) { func exitCode(err error) int { status := api.Status(err) switch { + case api.GrantBudgetExhausted(err): + return exitGrantBudgetExhausted case errors.Is(err, config.ErrNotAuthenticated), status == http.StatusUnauthorized: return exitNotAuthenticated case errors.Is(err, config.ErrNoProjectSelected), errors.Is(err, config.ErrNoProjectSelectedForProjectToken): diff --git a/cmd/volcano/main_test.go b/cmd/volcano/main_test.go index 640311ae..e5986a86 100644 --- a/cmd/volcano/main_test.go +++ b/cmd/volcano/main_test.go @@ -55,6 +55,8 @@ func TestRunMapsErrorsToStableExitCodes(t *testing.T) { {name: "missing resource", err: fmt.Errorf("get: %w", &api.Error{StatusCode: http.StatusNotFound}), want: exitNotFound}, {name: "refused", err: fmt.Errorf("delete: %w", &api.Error{StatusCode: http.StatusForbidden}), want: exitPermissionDenied}, {name: "account token required", err: cliconfig.ErrAccountTokenRequired, want: exitPermissionDenied}, + {name: "account token required, not a grant", err: cliconfig.ErrAccountTokenRequiredForGrant, want: exitPermissionDenied}, + {name: "no project for a grant", err: cliconfig.ErrNoProjectSelectedForGrant, want: exitNoProject}, {name: "unknown resource", err: fmt.Errorf("frontend %q %w", "web", api.ErrNotFound), want: exitNotFound}, {name: "rewritten 404", err: api.WithCause(errors.New("no such thing"), &api.Error{StatusCode: http.StatusNotFound}), want: exitNotFound}, {name: "rewritten refusal", err: api.WithCause(errors.New("ask a person"), cliconfig.ErrAccountTokenRequired), want: exitPermissionDenied}, diff --git a/internal/api/grant_errors.go b/internal/api/grant_errors.go new file mode 100644 index 00000000..e8275caa --- /dev/null +++ b/internal/api/grant_errors.go @@ -0,0 +1,17 @@ +package api + +import ( + "errors" + "net/http" +) + +// GrantBudgetExhaustedCode is the API's 429 code for a change made after a +// delegated grant used all of its mutations. A new grant gets a new budget. +const GrantBudgetExhaustedCode = "grant_budget_exhausted" + +// GrantBudgetExhausted reports whether err is the API's 429 for a delegated +// grant with no mutations left. +func GrantBudgetExhausted(err error) bool { + apiErr, ok := errors.AsType[*Error](err) + return ok && apiErr.StatusCode == http.StatusTooManyRequests && apiErr.Code == GrantBudgetExhaustedCode +} diff --git a/internal/cmd/connect/connect.go b/internal/cmd/connect/connect.go index f5b8b8b1..3da8e3bb 100644 --- a/internal/cmd/connect/connect.go +++ b/internal/cmd/connect/connect.go @@ -384,9 +384,9 @@ func ensureSession(ctx context.Context, opts options, sessions clisession.Factor } if token := cfg.Token(); token != "" && !opts.signup && !refused { - // A project access token can't be checked without a project; the - // project lookup below reports it if it is wrong. - if config.IsProjectToken(token) { + // A project access token or a delegated grant can't be checked without a + // project; the project lookup below reports it if it is wrong. + if config.IsProjectBound(token) { return nil } client, err := sessions.APIClient(sessions.APIURL(cfg), token) diff --git a/internal/cmd/durable/approvals/approvals.go b/internal/cmd/durable/approvals/approvals.go index 2dec2d3c..8962616c 100644 --- a/internal/cmd/durable/approvals/approvals.go +++ b/internal/cmd/durable/approvals/approvals.go @@ -31,7 +31,7 @@ one with ctx.waitForApproval and suspends until a person approves or denies it, or until its timeout passes. Either way the workflow resumes with the outcome. Anyone with access to the project can list approvals and read their stats. -Only a person can decide one: approve and deny refuse a project access token.`, +Only a person can decide one: approve and deny refuse a project access token or a delegated grant.`, } cmd.AddCommand(newList(deps)) cmd.AddCommand(newGet(deps)) diff --git a/internal/cmd/durable/approvals/approvals_test.go b/internal/cmd/durable/approvals/approvals_test.go index 07c8a708..431d1759 100644 --- a/internal/cmd/durable/approvals/approvals_test.go +++ b/internal/cmd/durable/approvals/approvals_test.go @@ -867,15 +867,25 @@ func TestApprovalsDecideRefusesAProjectAccessToken(t *testing.T) { } } -// A credential the CLI knows is a project access token is refused before the -// prompt and before the approval is read: confirming a decision the API will -// refuse wastes the person's yes, and an approval that does not exist would -// otherwise answer with a 404 instead of saying who can decide. +// A credential the CLI knows is a project access token or a delegated grant is +// refused before the prompt and before the approval is read: confirming a +// decision the API will refuse wastes the person's yes, and an approval that +// does not exist would otherwise answer with a 404 instead of saying who can +// decide. func TestApprovalsDecideRefusesAProjectTokenBeforeAnyRequest(t *testing.T) { - for _, args := range [][]string{{"approve", approvalID}, {"deny", approvalID, "--yes"}} { - t.Run(args[0], func(t *testing.T) { + for _, tc := range []struct{ token, command string }{ + {"PT-approvals-test", "approve"}, + {"PT-approvals-test", "deny"}, + {"pg-approvals-test", "approve"}, + {"pg-approvals-test", "deny"}, + } { + args := []string{"approve", approvalID} + if tc.command == "deny" { + args = []string{"deny", approvalID, "--yes"} + } + t.Run(tc.token[:3]+tc.command, func(t *testing.T) { setApprovalsTestHome(t) - t.Setenv("VOLCANO_TOKEN", "PT-approvals-test") + t.Setenv("VOLCANO_TOKEN", tc.token) server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { t.Errorf("a project access token's decision must not reach the API: %s %s", r.Method, r.URL.Path) respondJSON(t, w, http.StatusNotFound, map[string]any{"error": "not found"}) diff --git a/internal/cmd/durable/approvals/approve.go b/internal/cmd/durable/approvals/approve.go index 8646e065..d4e22c1b 100644 --- a/internal/cmd/durable/approvals/approve.go +++ b/internal/cmd/durable/approvals/approve.go @@ -22,7 +22,7 @@ was denied, expired, or cancelled cannot be approved; the command says which and exits non-zero. Only a person can decide: log in with 'volcano login'. A project access token -is refused. +or a delegated grant is refused. By default this command prompts for confirmation, showing the approval's title. Use --yes to skip the prompt.`, diff --git a/internal/cmd/durable/approvals/deny.go b/internal/cmd/durable/approvals/deny.go index ff83d340..ab3040a6 100644 --- a/internal/cmd/durable/approvals/deny.go +++ b/internal/cmd/durable/approvals/deny.go @@ -21,7 +21,7 @@ approved, expired, or cancelled cannot be denied; the command says which and exits non-zero. Only a person can decide: log in with 'volcano login'. A project access token -is refused. +or a delegated grant is refused. By default this command prompts for confirmation, showing the approval's title. Use --yes to skip the prompt.`, diff --git a/internal/cmd/feedback/feedback.go b/internal/cmd/feedback/feedback.go index c671b75f..eac46f18 100644 --- a/internal/cmd/feedback/feedback.go +++ b/internal/cmd/feedback/feedback.go @@ -77,13 +77,14 @@ carries the selected project ID and this machine's telemetry ID, and is linked to your account. With telemetry off and feedback turned on with volcano feedback enable, it carries neither and is not linked to your account. Every report is sent with your login, which the server checks, so sending one -requires volcano login. A project access token cannot send reports. +requires volcano login. A project access token or a delegated grant cannot +send reports. A report is not sent, and the command exits 0 with no output other than the first-run notice when this run shows it, while feedback is off, when you are -not logged in or use a project access token, or before the first-run notice -has been shown. --dry-run says whether a report would be sent, and volcano -feedback status says why not. +not logged in or use a project access token or a delegated grant, or before +the first-run notice has been shown. --dry-run says whether a report would be +sent, and volcano feedback status says why not. Feedback follows telemetry: turning telemetry off also turns feedback off, unless you run volcano feedback enable. Turn it off with volcano feedback @@ -127,8 +128,8 @@ disable or VOLCANO_FEEDBACK_DISABLED=1.`, return nil } // The server takes reports only from a logged-in account, so a - // logged-out run, or one with a project access token, sends - // nothing, like feedback that is off. + // logged-out run, or one with a project access token or a + // delegated grant, sends nothing, like feedback that is off. token := cfg.Token() loggedIn := telemetry.CanSendFeedback(token) if !loggedIn && !dryRun { @@ -239,6 +240,10 @@ func printStatus(cmd *cobra.Command) error { fmt.Fprintln(w, "Feedback is enabled, but no report is sent until you log in with `volcano login`.") return nil } + if config.IsDelegatedGrant(cfg.Token()) { + fmt.Fprintln(w, "Feedback is enabled, but a delegated grant cannot send reports. Log in with `volcano login`.") + return nil + } if config.IsProjectToken(cfg.Token()) { fmt.Fprintln(w, "Feedback is enabled, but a project access token cannot send reports. Log in with `volcano login`.") return nil @@ -338,7 +343,7 @@ func saveReport(payload []byte) error { // printReport prints report for --dry-run, and says so when a real run would // send nothing: feedback is off, the first-run notice has not been shown, or -// the user is logged out or uses a project access token. +// the user is logged out or uses a project access token or a delegated grant. func printReport(cmd *cobra.Command, report feedback.Report, sends bool) error { out, err := encodeReport(report, " ") if err != nil { diff --git a/internal/cmd/feedback/feedback_test.go b/internal/cmd/feedback/feedback_test.go index a2610ae6..81af0bdc 100644 --- a/internal/cmd/feedback/feedback_test.go +++ b/internal/cmd/feedback/feedback_test.go @@ -309,22 +309,29 @@ func TestFeedback_LoggedOutSendsNothing(t *testing.T) { } func TestFeedback_ProjectTokenSendsNothing(t *testing.T) { - reports := setup(t, &config.TelemetryConfig{DeviceID: "d", NoticeShown: true}) - t.Setenv("VOLCANO_TOKEN", "pt-abc") + for token, credential := range map[string]string{ + "pt-abc": "a project access token", + "pg-abc": "a delegated grant", + } { + t.Run(token, func(t *testing.T) { + reports := setup(t, &config.TelemetryConfig{DeviceID: "d", NoticeShown: true}) + t.Setenv("VOLCANO_TOKEN", token) - stdout, stderr, err := run(t, reportArgs...) - require.NoError(t, err, "exit 0, so an agent does not retry") - assert.Empty(t, stdout) - assert.Empty(t, stderr) - assert.Empty(t, *reports) + stdout, stderr, err := run(t, reportArgs...) + require.NoError(t, err, "exit 0, so an agent does not retry") + assert.Empty(t, stdout) + assert.Empty(t, stderr) + assert.Empty(t, *reports) - stdout, _, err = run(t, "status") - require.NoError(t, err) - assert.Equal(t, "Feedback is enabled, but a project access token cannot send reports. Log in with `volcano login`.\n", stdout) + stdout, _, err = run(t, "status") + require.NoError(t, err) + assert.Equal(t, "Feedback is enabled, but "+credential+" cannot send reports. Log in with `volcano login`.\n", stdout) - _, stderr, err = run(t, append([]string{"--dry-run"}, reportArgs...)...) - require.NoError(t, err) - assert.Contains(t, stderr, "This report would not be sent.") + _, stderr, err = run(t, append([]string{"--dry-run"}, reportArgs...)...) + require.NoError(t, err) + assert.Contains(t, stderr, "This report would not be sent.") + }) + } } func TestFeedback_SettingSurvivesLogout(t *testing.T) { diff --git a/internal/cmd/lavaflow/disable.go b/internal/cmd/lavaflow/disable.go index 49a0ed19..d8cbfd81 100644 --- a/internal/cmd/lavaflow/disable.go +++ b/internal/cmd/lavaflow/disable.go @@ -24,7 +24,7 @@ func newDisable(deps cliruntime.Deps) *cobra.Command { Short: "Turn LavaFlow off", Long: `Turn LavaFlow off for the current project. No new run starts; past runs and their records stay readable. Only the project's owner can, from their own -session: a project access token is refused.`, +session: a project access token or a delegated grant is refused.`, Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, _ []string) error { opts.deps = deps diff --git a/internal/cmd/lavaflow/enable.go b/internal/cmd/lavaflow/enable.go index e6d03b3b..bf5feb86 100644 --- a/internal/cmd/lavaflow/enable.go +++ b/internal/cmd/lavaflow/enable.go @@ -38,7 +38,8 @@ This turns LavaFlow on for the current project. Before it does, it prints the sentence above and: ` + clilavaflow.EnableNotice + ` and asks you to confirm; --yes skips the question. Only the project's owner -can turn it on, from their own session: a project access token is refused. The +can turn it on, from their own session: a project access token or a delegated +grant is refused. The owner's plan has to include LavaFlow. Connect a repository with 'volcano git connect' so LavaFlow can open fix pull requests. Choose how far it goes with '` + cliruntime.CommandPath(deps, "lavaflow settings set --autonomy") + `'.`, diff --git a/internal/cmd/lavaflow/enable_test.go b/internal/cmd/lavaflow/enable_test.go index ac69ab9c..528cf9ff 100644 --- a/internal/cmd/lavaflow/enable_test.go +++ b/internal/cmd/lavaflow/enable_test.go @@ -58,7 +58,7 @@ func TestChangesWithAProjectTokenNeedAPersonsSession(t *testing.T) { t.Helper() t.Setenv("VOLCANO_TOKEN", cliconfig.ProjectTokenPrefix+"secret") }, - "changing LavaFlow's settings needs a person's session: a project access token can't. " + + "changing LavaFlow's settings needs a person's session: a project access token or a delegated grant can't. " + "Unset VOLCANO_TOKEN and sign in with 'volcano login', then run this again", }, "saved": { @@ -69,7 +69,7 @@ func TestChangesWithAProjectTokenNeedAPersonsSession(t *testing.T) { CurrentProject: &cliconfig.ProjectConfig{ID: lavaflowProjectID, Name: "Shop"}, }).Save()) }, - "changing LavaFlow's settings needs a person's session: a project access token can't. " + + "changing LavaFlow's settings needs a person's session: a project access token or a delegated grant can't. " + "Sign in with 'volcano login' and run this again", }, } { @@ -216,7 +216,7 @@ func TestAPersonRequiredRefusalKeepsVolcanosWords(t *testing.T) { for message, want := range map[string]string{ tokenNeverExpiresRefusal: "LavaFlow's settings weren't changed: " + tokenNeverExpiresRefusal, signInRefusal: "LavaFlow's settings weren't changed: " + signInRefusal, - "": "changing LavaFlow's settings needs a person's session: a project access token can't. " + + "": "changing LavaFlow's settings needs a person's session: a project access token or a delegated grant can't. " + "Sign in with 'volcano login' and run this again", } { fake, deps := newLavaflowServer(t) diff --git a/internal/cmd/lavaflow/runs_start_test.go b/internal/cmd/lavaflow/runs_start_test.go index 2dbba362..410140ec 100644 --- a/internal/cmd/lavaflow/runs_start_test.go +++ b/internal/cmd/lavaflow/runs_start_test.go @@ -50,18 +50,23 @@ func TestRunStartRefusesAnInvalidIssueID(t *testing.T) { fake.set(func(s *lavaflowServer) { assert.Empty(t, s.started) }) } -// A project access token can't start a run, so the CLI says how the owner -// does before it sends anything, with the exit code of a permission refused. +// A project access token or a delegated grant can't start a run, so the CLI +// says how the owner does before it sends anything, with the exit code of a +// permission refused. func TestRunStartWithAProjectTokenNeedsTheOwnersSession(t *testing.T) { - fake, deps := newLavaflowServer(t) - t.Setenv("VOLCANO_TOKEN", cliconfig.ProjectTokenPrefix+"secret") + for _, prefix := range []string{cliconfig.ProjectTokenPrefix, cliconfig.DelegatedGrantPrefix} { + t.Run(prefix, func(t *testing.T) { + fake, deps := newLavaflowServer(t) + t.Setenv("VOLCANO_TOKEN", prefix+"secret") - _, err := executeLavaflowCommand(t, New(deps), "runs", "start", lavaflowIssueID) - require.ErrorIs(t, err, clilavaflow.ErrPersonStarts) - require.ErrorIs(t, err, cliconfig.ErrAccountTokenRequired, "the exit code reports a permission refused") - assert.Equal(t, "starting a LavaFlow run needs the project owner's session: a project access token can't. "+ - "Unset VOLCANO_TOKEN and sign in with 'volcano login', then run this again", err.Error()) - fake.set(func(s *lavaflowServer) { assert.Empty(t, s.started) }) + _, err := executeLavaflowCommand(t, New(deps), "runs", "start", lavaflowIssueID) + require.ErrorIs(t, err, clilavaflow.ErrPersonStarts) + require.ErrorIs(t, err, cliconfig.ErrAccountTokenRequired, "the exit code reports a permission refused") + assert.Equal(t, "starting a LavaFlow run needs the project owner's session: a project access token or a delegated grant can't. "+ + "Unset VOLCANO_TOKEN and sign in with 'volcano login', then run this again", err.Error()) + fake.set(func(s *lavaflowServer) { assert.Empty(t, s.started) }) + }) + } } // Each refusal says what to do about it, keeping the API's status for the diff --git a/internal/cmd/lavaflow/settings_set.go b/internal/cmd/lavaflow/settings_set.go index 9691dc6f..5ea2cda0 100644 --- a/internal/cmd/lavaflow/settings_set.go +++ b/internal/cmd/lavaflow/settings_set.go @@ -34,7 +34,7 @@ func newSettingsSet(deps cliruntime.Deps) *cobra.Command { Short: "Change LavaFlow's settings", Long: fmt.Sprintf(`Change LavaFlow's settings for the current project. A setting you don't pass keeps its value. Only the project's owner can, from their own session: a -project access token is refused. +project access token or a delegated grant is refused. --autonomy decides how far LavaFlow goes: %[1]s diagnoses issues and leaves any fix to a person; %[2]s also opens fix pull requests for review; %[3]s diff --git a/internal/config/config.go b/internal/config/config.go index c8bdd77c..cb2b6a76 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -35,6 +35,9 @@ const ( // ProjectTokenPrefix marks a project access token, which reaches only the // project it was minted in. ProjectTokenPrefix = "pt-" + // DelegatedGrantPrefix marks a delegated grant: a short-lived credential a + // first-party app's worker gets for one run on one project. + DelegatedGrantPrefix = "pg-" ) var ( @@ -52,8 +55,35 @@ var ( ErrAccountTokenRequired = errors.New("this command needs an account token (" + AccountTokenPrefix + ") but the current credential is a project access token (" + ProjectTokenPrefix + "), which only reaches the project it was minted in. Run 'volcano login', or set VOLCANO_TOKEN to an account token") + // ErrNoProjectSelectedForGrant is ErrNoProjectSelectedForProjectToken + // worded for a delegated grant. It matches that error, so the exit code is + // the same. + ErrNoProjectSelectedForGrant error = grantRefusal{ + message: "no project selected. A delegated grant (" + DelegatedGrantPrefix + + ") is scoped to one project: run 'volcano use ' with that project's ID", + matches: ErrNoProjectSelectedForProjectToken, + } + // ErrAccountTokenRequiredForGrant is ErrAccountTokenRequired worded for a + // delegated grant. It matches that error, so the exit code is the same. + ErrAccountTokenRequiredForGrant error = grantRefusal{ + message: "this command needs an account token (" + AccountTokenPrefix + + ") but the current credential is a delegated grant (" + DelegatedGrantPrefix + + "), which only reaches the project it was granted for. Run 'volcano login', or set VOLCANO_TOKEN to an account token", + matches: ErrAccountTokenRequired, + } ) +// grantRefusal is a refusal worded for a delegated grant that matches the +// project access token's refusal with errors.Is. +type grantRefusal struct { + message string + matches error +} + +func (e grantRefusal) Error() string { return e.message } + +func (e grantRefusal) Is(target error) bool { return target == e.matches } + // These variables are intentionally settable with -ldflags -X. var ( compiledDefaultAPIURL = defaultCompiledAPIURL @@ -622,17 +652,23 @@ func (c *Config) RequireProject() error { if strings.TrimSpace(c.ProjectID()) != "" { return nil } + if IsDelegatedGrant(c.Token()) { + return ErrNoProjectSelectedForGrant + } if IsProjectToken(c.Token()) { return ErrNoProjectSelectedForProjectToken } return ErrNoProjectSelected } -// RequireAccountToken rejects a project access token on a command that needs -// account-wide access. Any other credential is treated as an account token: the -// server is the authority on what a token may do, and tokens minted before the -// prefixes existed carry neither. +// RequireAccountToken rejects a project access token or a delegated grant on a +// command that needs account-wide access. Any other credential is treated as an +// account token: the server is the authority on what a token may do, and tokens +// minted before the prefixes existed carry none. func (c *Config) RequireAccountToken() error { + if IsDelegatedGrant(c.Token()) { + return ErrAccountTokenRequiredForGrant + } if IsProjectToken(c.Token()) { return ErrAccountTokenRequired } @@ -649,6 +685,19 @@ func IsProjectToken(token string) bool { return strings.HasPrefix(strings.ToLower(strings.TrimSpace(token)), ProjectTokenPrefix) } +// IsDelegatedGrant reports whether token is a delegated grant. Case-insensitive +// for the same reason as IsProjectToken. +func IsDelegatedGrant(token string) bool { + return strings.HasPrefix(strings.ToLower(strings.TrimSpace(token)), DelegatedGrantPrefix) +} + +// IsProjectBound reports whether token reaches only one project: a project +// access token or a delegated grant. Without it, a grant would fall to the +// unknown-prefix default and be treated as an account token. +func IsProjectBound(token string) bool { + return IsProjectToken(token) || IsDelegatedGrant(token) +} + // FirstPartyDeviceClientID resolves the first-party OAuth device client ID. func FirstPartyDeviceClientID() (string, error) { if clientID := strings.TrimSpace(os.Getenv(envFirstPartyDeviceID)); clientID != "" { diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 795cfa07..b5fb5b91 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -514,3 +514,41 @@ func TestIsProjectToken(t *testing.T) { }) } } + +func TestIsDelegatedGrantAndIsProjectBound(t *testing.T) { + for token, want := range map[string][2]bool{ + DelegatedGrantPrefix + "abc": {true, true}, + " PG-abc": {true, true}, + ProjectTokenPrefix + "abc": {false, true}, + AccountTokenPrefix + "abc": {false, false}, + "legacy-token": {false, false}, + "": {false, false}, + } { + t.Run(token, func(t *testing.T) { + assert.Equal(t, want[0], IsDelegatedGrant(token), "IsDelegatedGrant") + assert.Equal(t, want[1], IsProjectBound(token), "IsProjectBound") + }) + } +} + +// A grant was once an unknown prefix, and so an account token. It is refused +// where a pt- token is, with the same sentinels, in words that name a grant. +func TestRequireAccountTokenAndProjectRefuseADelegatedGrant(t *testing.T) { + t.Setenv(envToken, "") + t.Setenv(envProjectID, "") + cfg := &Config{UserToken: DelegatedGrantPrefix + "secret-grant"} + for name, err := range map[string]error{ + "account token": cfg.RequireAccountToken(), + "project": cfg.RequireProject(), + } { + t.Run(name, func(t *testing.T) { + require.Error(t, err) + assert.Contains(t, err.Error(), "delegated grant ("+DelegatedGrantPrefix+")") + assert.NotContains(t, err.Error(), "project access token") + assert.NotContains(t, err.Error(), ProjectTokenPrefix) + }) + } + require.ErrorIs(t, cfg.RequireAccountToken(), ErrAccountTokenRequired) + require.ErrorIs(t, cfg.RequireProject(), ErrNoProjectSelectedForProjectToken) + assert.NotErrorIs(t, cfg.RequireProject(), ErrAccountTokenRequired) +} diff --git a/internal/durable/durable.go b/internal/durable/durable.go index 9eae75ae..3a2ee815 100644 --- a/internal/durable/durable.go +++ b/internal/durable/durable.go @@ -487,10 +487,11 @@ func (s Service) DecideApproval( return nil, approvalError(action, approvalID, err) } -// errApprovalNeedsAPerson is the answer to a project access token deciding. +// errApprovalNeedsAPerson is the answer to a project access token or a +// delegated grant deciding. // Approvals exist so that a person signs off, which is why the API refuses // any credential that is not a person's. VOLCANO_TOKEN overrides a login, and -// is how a project access token usually arrives. It matches +// is how either usually arrives. It matches // config.ErrAccountTokenRequired, so the exit code reports a refused // credential. The API's 403 is left out: its generic body would add a hint // about the token's project, which is not the problem here. @@ -498,15 +499,15 @@ var errApprovalNeedsAPerson = api.WithCause(errors.New( "approvals are decided by a person. Run `volcano login` with VOLCANO_TOKEN unset, then decide again"), config.ErrAccountTokenRequired) -// RequirePerson refuses a credential the CLI knows is a project access token, -// so a decision is not confirmed only to be refused. DecideApproval still maps +// RequirePerson refuses a credential the CLI knows is a project access token +// or a delegated grant, so a decision is not confirmed only to be refused. DecideApproval still maps // the API's refusal, for credentials without a recognizable prefix. func (s Service) RequirePerson() error { cfg, err := s.sessions.Config() if err != nil { return err } - if config.IsProjectToken(cfg.Token()) { + if config.IsProjectBound(cfg.Token()) { return errApprovalNeedsAPerson } return nil diff --git a/internal/lavaflow/lavaflow.go b/internal/lavaflow/lavaflow.go index 8fa9746e..b460166a 100644 --- a/internal/lavaflow/lavaflow.go +++ b/internal/lavaflow/lavaflow.go @@ -106,10 +106,10 @@ var ErrCantTurnOn = errors.New("LavaFlow can't be turned on for this project: " "it isn't open to the owner's account, or the owner's plan doesn't include it") // ErrPersonDecides refuses changing LavaFlow's settings with a project access -// token. Turning LavaFlow on, off or changing how far it goes is a person's +// token or a delegated grant. Turning LavaFlow on, off or changing how far it goes is a person's // decision, so the API takes it from a person's session only, and the CLI // says so before asking. -var ErrPersonDecides = errors.New("changing LavaFlow's settings needs a person's session: a project access token can't") +var ErrPersonDecides = errors.New("changing LavaFlow's settings needs a person's session: a project access token or a delegated grant can't") // ErrOwnerDecides refuses changing LavaFlow's settings when the settings say // the caller can't, before the CLI asks or sends anything. Only the project's @@ -409,7 +409,8 @@ func validMergePolicy(policy *apiclient.LavaflowMergePolicyUpdate) error { return nil } -// update refuses a project access token before sending anything. Every +// update refuses a project access token or a delegated grant before sending +// anything. Every // change reads the settings first, which refuses a project without LavaFlow // records with the route's reason, and checks can_update before it asks or // sends. A change that turns LavaFlow on, or changes autonomy to merge, then @@ -425,7 +426,7 @@ func (s Service) update(ctx context.Context, verb string, update apiclient.Lavaf if err != nil { return nil, err } - if config.IsProjectToken(authenticated.Config.Token()) { + if config.IsProjectBound(authenticated.Config.Token()) { return nil, personDecides(authenticated.Config, config.ErrAccountTokenRequired) } current, err := authenticated.API.GetLavaflowSettings(ctx, authenticated.ProjectID) diff --git a/internal/lavaflow/start.go b/internal/lavaflow/start.go index 3c07cb70..4060ff8e 100644 --- a/internal/lavaflow/start.go +++ b/internal/lavaflow/start.go @@ -16,14 +16,16 @@ import ( clisession "github.com/Kong/volcano-cli/internal/session" ) -// ErrPersonStarts refuses starting a run with a project access token. A run +// ErrPersonStarts refuses starting a run with a project access token or a +// delegated grant. A run // sends the project's logs and source code to an AI service and can open a // pull request, so the API takes it from the owner's own session only. -var ErrPersonStarts = errors.New("starting a LavaFlow run needs the project owner's session: a project access token can't") +var ErrPersonStarts = errors.New("starting a LavaFlow run needs the project owner's session: a project access token or a delegated grant can't") // ErrStartNeedsLiveSignIn reports the start's person_required for a token // the CLI sent as a person's: a platform token that never expires, or one -// whose sign-in ended. The CLI refuses a project access token before it asks. +// whose sign-in ended. The CLI refuses a project access token or a delegated +// grant before it asks. var ErrStartNeedsLiveSignIn = errors.New("starting a LavaFlow run needs the project owner's live sign-in") // ErrStartUnavailable reports an API that serves LavaFlow but can't start a @@ -61,7 +63,7 @@ func (s Service) StartRun(ctx context.Context, issueID string) (*apiclient.Lavaf if err != nil { return nil, nil, err } - if config.IsProjectToken(authenticated.Config.Token()) { + if config.IsProjectBound(authenticated.Config.Token()) { return nil, nil, personStarts(authenticated.Config, config.ErrAccountTokenRequired) } run, body, err := authenticated.API.CreateLavaflowRun(ctx, authenticated.ProjectID, id) diff --git a/internal/telemetry/client_test.go b/internal/telemetry/client_test.go index a8fa7d3b..36f56057 100644 --- a/internal/telemetry/client_test.go +++ b/internal/telemetry/client_test.go @@ -125,6 +125,12 @@ func TestScrubText(t *testing.T) { assert.Equal(t, want, ScrubText(in, "/home/dev")) } +// A delegated grant pasted into a feedback report is removed like any other +// Volcano token. +func TestScrubText_DelegatedGrant(t *testing.T) { + assert.Equal(t, "VOLCANO_TOKEN=[redacted] failed", ScrubText("VOLCANO_TOKEN=pg-0123456789abcdefABCD failed", "")) +} + func TestScrubText_BearerAcrossLines(t *testing.T) { assert.Equal(t, "Authorization: Bearer [redacted]\nnext", ScrubText("Authorization: Bearer\nabc.def\nnext", "")) } diff --git a/internal/telemetry/fields.go b/internal/telemetry/fields.go index a9355ade..81ad7768 100644 --- a/internal/telemetry/fields.go +++ b/internal/telemetry/fields.go @@ -177,7 +177,7 @@ const redacted = "[redacted]" var ( bearerPattern = regexp.MustCompile(`(?i)\bbearer\s+\S+`) jwtPattern = regexp.MustCompile(`eyJ[A-Za-z0-9_-]+\.[A-Za-z0-9_-]+\.[A-Za-z0-9_-]*`) - keyPattern = regexp.MustCompile(`\b(?:(?:(?:pk|pt|sk|ak)-|vbt_|vpg_|vcb_)[A-Za-z0-9_-]{16,}|sbp1\.[A-Za-z0-9+/=:]{16,})`) + keyPattern = regexp.MustCompile(`\b(?:(?:(?:pk|pt|pg|sk|ak)-|vbt_|vpg_|vcb_|vgt_)[A-Za-z0-9_-]{16,}|sbp1\.[A-Za-z0-9+/=:]{16,})`) userPattern = regexp.MustCompile(`([a-zA-Z][a-zA-Z0-9+.-]*://)[^\s/?#]*@`) emailPattern = regexp.MustCompile(`[A-Za-z0-9._%+-]+@[A-Za-z0-9-]+(?:\.[A-Za-z0-9-]+)*\.[A-Za-z]{2,}`) queryPattern = regexp.MustCompile(`(https?://[^\s?#]+)\?[^\s#]*`) diff --git a/internal/telemetry/fields_test.go b/internal/telemetry/fields_test.go index a5706e6a..4d5b6560 100644 --- a/internal/telemetry/fields_test.go +++ b/internal/telemetry/fields_test.go @@ -49,6 +49,7 @@ func TestResources(t *testing.T) { {"positional wins over flag", "get ", []string{"a", "--function", "b"}, map[string]string{"function": "a"}}, {"slice flag value is dropped", "list", []string{"--database", "a,b"}, nil}, {"secret-shaped value is dropped", "get ", []string{"pk-abcdefghijklmnopqrstuvwx"}, nil}, + {"grant-shaped value is dropped", "get ", []string{"pg-abcdefghijklmnopqrstuvwx"}, nil}, {"odd characters are dropped", "get ", []string{"a;rm -rf"}, nil}, } for _, tc := range cases { @@ -102,6 +103,8 @@ func TestRedact(t *testing.T) { "token " + jwt + " expired": "token [redacted] expired", "key pk-0123456789abcdefABCD rejected": "key [redacted] rejected", "used pt-0123456789abcdefABCD, sk-0123456789abcdefABCD": "used [redacted], [redacted]", + "grant pg-0123456789abcdefABCD is spent": "grant [redacted] is spent", + "x-api-key vgt_0123456789abcdefABCD refused": "x-api-key [redacted] refused", "pk-short": "pk-short", "proxy sbp1.v2:0123456789abcdef:AbC+dEf/GhI=jKl= denied": "proxy [redacted] denied", "proxy sbp1.AbC+dEf/GhI0123456789= denied": "proxy [redacted] denied", diff --git a/internal/telemetry/send.go b/internal/telemetry/send.go index 56f65ba2..5aacdeff 100644 --- a/internal/telemetry/send.go +++ b/internal/telemetry/send.go @@ -77,7 +77,9 @@ func Flush(ctx context.Context, r io.Reader, client *http.Client) error { } req.Header.Set("Content-Type", "application/json") req.Header.Set("User-Agent", "volcano-cli/"+version.Version) - if token := cfg.Token(); token != "" { + // A delegated grant is never sent: the server ignores it here, and the + // flush is a detached process the grant's run does not control. + if token := cfg.Token(); token != "" && !config.IsDelegatedGrant(token) { req.Header.Set("Authorization", "Bearer "+token) } resp, err := client.Do(req) diff --git a/internal/telemetry/telemetry.go b/internal/telemetry/telemetry.go index 99e31eed..3f7268ef 100644 --- a/internal/telemetry/telemetry.go +++ b/internal/telemetry/telemetry.go @@ -238,17 +238,18 @@ func FeedbackDebugByEnv(getenv func(string) string) bool { } // LoggedIn reports whether the config holds a login token that can send a -// feedback report. `volcano feedback` sends nothing without one. A project -// access token does not count: the server takes reports only from an account. +// feedback report. `volcano feedback` sends nothing without one. A credential +// tied to one project, a project access token or a delegated grant, does not +// count: the server takes reports only from an account. func (r *Recorder) LoggedIn() bool { r.setup() return r.cfg != nil && CanSendFeedback(r.cfg.Token()) } // CanSendFeedback reports whether token can send a feedback report: it is set -// and is not a project access token. +// and is not tied to one project (a project access token or a delegated grant). func CanSendFeedback(token string) bool { - return token != "" && !config.IsProjectToken(token) + return token != "" && !config.IsProjectBound(token) } // FeedbackOn reports whether `volcano feedback` may send a report: the config diff --git a/internal/telemetry/telemetry_test.go b/internal/telemetry/telemetry_test.go index 9b2911de..cd3060cf 100644 --- a/internal/telemetry/telemetry_test.go +++ b/internal/telemetry/telemetry_test.go @@ -814,3 +814,32 @@ func TestAnnounceFeedback_Off(t *testing.T) { assert.False(t, h.recorder().AnnounceFeedback(io.Discard), "feedback follows telemetry") assert.Zero(t, h.store.saves) } + +// A delegated grant never leaves the machine on a telemetry request. +func TestFlush_DelegatedGrantSendsNoAuthorization(t *testing.T) { + for _, token := range []string{config.DelegatedGrantPrefix + "grant-secret"} { + t.Run(token[:3], func(t *testing.T) { + var gotAuth string + var gotBody []byte + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotAuth = r.Header.Get("Authorization") + gotBody, _ = io.ReadAll(r.Body) + w.WriteHeader(http.StatusAccepted) + })) + t.Cleanup(srv.Close) + t.Setenv("HOME", t.TempDir()) + t.Setenv("VOLCANO_API_URL", srv.URL) + t.Setenv("VOLCANO_TOKEN", token) + + require.NoError(t, Flush(t.Context(), strings.NewReader(`{"device_id":"d"}`), srv.Client())) + assert.Empty(t, gotAuth) + assert.NotContains(t, string(gotBody), token) + }) + } +} + +func TestCanSendFeedbackRefusesProjectBoundTokens(t *testing.T) { + assert.True(t, CanSendFeedback(config.AccountTokenPrefix+"abc")) + assert.False(t, CanSendFeedback(config.ProjectTokenPrefix+"abc")) + assert.False(t, CanSendFeedback(config.DelegatedGrantPrefix+"abc")) +}