diff --git a/internal/daemon/api_workshopctl.go b/internal/daemon/api_workshopctl.go index de5837dcd..a12325c59 100644 --- a/internal/daemon/api_workshopctl.go +++ b/internal/daemon/api_workshopctl.go @@ -20,7 +20,10 @@ import ( "github.com/jessevdk/go-flags" + "github.com/canonical/workshop/internal/logger" + "github.com/canonical/workshop/internal/overlord/hookstate" "github.com/canonical/workshop/internal/overlord/hookstate/ctlcmd" + "github.com/canonical/workshop/internal/workshop" ) // workshopCtlOptions holds the various options with which workshopctl is invoked. @@ -60,17 +63,18 @@ func v1PostWorkshopCtl(c *Command, r *http.Request, _ *userState) Response { return statusForbidden("cannot get remote user: %w", err) } - // Ignore missing context error to allow 'workshopctl -h' without a context; - // Actual context is validated later by get/set. - context, _ := c.d.overlord.HookManager().Context(reqData.ContextID) + hookContext, response := workshopctlHookContext(c, r, reqData.ContextID) + if response != nil { + return response + } if reqData.Stdin != nil { - context.Lock() - context.Set("stdin", reqData.Stdin) - context.Unlock() + hookContext.Lock() + hookContext.Set("stdin", reqData.Stdin) + hookContext.Unlock() } - stdout, stderr, err := ctlcmd.Run(context, reqData.Args, uid) + stdout, stderr, err := ctlcmd.Run(r.Context(), hookContext, reqData.Args, uid) if err != nil { if e, ok := err.(*flags.Error); ok && e.Type == flags.ErrHelp { stdout = []byte(e.Error()) @@ -86,3 +90,64 @@ func v1PostWorkshopCtl(c *Command, r *http.Request, _ *userState) Response { return SyncResponse(result, http.StatusOK) } + +// workshopctlHookContext returns the hook context used to execute a +// workshopctl command. A supplied cookie selects an existing context; +// otherwise the workshop instance ID is validated before creating an +// ephemeral context. +func workshopctlHookContext( + c *Command, + r *http.Request, + contextID string, +) (*hookstate.Context, Response) { + if contextID != "" { + return workshopctlHookContextFromContextID(c, contextID) + } + return workshopctlHookContextFromInstanceID(c, r) +} + +// workshopctlHookContextFromContextID returns the active hook context +// identified by the request's context ID, supplied to hooks as WORKSHOP_COOKIE. +// An invalid context ID is rejected rather than falling back to instance ID +// authentication. +func workshopctlHookContextFromContextID( + c *Command, + contextID string, +) (*hookstate.Context, Response) { + hookContext, err := c.d.overlord.HookManager().Context(contextID) + if err != nil { + return nil, statusBadRequest("invalid workshop cookie") + } + return hookContext, nil +} + +// workshopctlHookContextFromInstanceID validates that the requesting user owns +// the workshop instance identified by the request context, then creates a +// taskless context for this individual workshopctl invocation. +func workshopctlHookContextFromInstanceID( + c *Command, + r *http.Request, +) (*hookstate.Context, Response) { + instanceID, _ := r.Context(). + Value(workshop.ContextWorkshopInstanceID).(string) + if instanceID == "" { + return nil, statusBadRequest("workshop instance ID not supplied") + } + + valid, err := c.d.overlord.WorkshopManager(). + OwnsWorkshopInstanceID(r.Context(), instanceID) + if err != nil { + logger.Noticef("cannot validate workshop instance ID: %v", err) + return nil, statusInternalError("internal error occurred validating workshop instance id") + } + if !valid { + return nil, statusForbidden("invalid workshop instance ID") + } + + hookContext, err := c.d.overlord.HookManager().NewEphemeralContext() + if err != nil { + logger.Noticef("cannot create workshop context: %v", err) + return nil, statusInternalError("internal error occurred") + } + return hookContext, nil +} diff --git a/internal/daemon/api_workshopctl_test.go b/internal/daemon/api_workshopctl_test.go index 69dba5a79..700186114 100644 --- a/internal/daemon/api_workshopctl_test.go +++ b/internal/daemon/api_workshopctl_test.go @@ -16,14 +16,35 @@ package daemon import ( "bytes" + "context" "net/http" "gopkg.in/check.v1" + + "github.com/canonical/workshop/internal/workshop" + "github.com/canonical/workshop/internal/workshop/fakebackend" ) -func (s *apiSuite) TestWorkshopHelpCtlNoContext(c *check.C) { +func (s *apiSuite) addWorkshopWithInstanceID(instanceID string) { + s.b.Workshops[s.project.ProjectId] = map[string]*fakebackend.FakeWorkshop{ + "test-workshop": { + Workshop: &workshop.Workshop{ + Name: "test-workshop", + Project: s.project, + InstanceID: instanceID, + }, + }, + } +} + +// TestWorkshopHelpCtlWithoutCookie checks that help works through the +// cookie-less workshopctl path. The test relies on the middleware-provided +// workshop instance ID being present in the request context so the handler can +// validate the caller and create an ephemeral hook context. +func (s *apiSuite) TestWorkshopHelpCtlWithoutCookie(c *check.C) { // Setup s.daemon(c) + s.addWorkshopWithInstanceID("instance-id") wctl := apiCmd("/v1/workshopctl") buf := bytes.NewBufferString(`{"args":["-h"]}`) @@ -31,6 +52,12 @@ func (s *apiSuite) TestWorkshopHelpCtlNoContext(c *check.C) { req, err := s.createProjectsRequest("POST", "/v1/workshopctl", buf) c.Assert(err, check.IsNil) + req = req.WithContext(context.WithValue( + req.Context(), + workshop.ContextWorkshopInstanceID, + "instance-id", + )) + // Execute rsp := v1PostWorkshopCtl(wctl, req, nil).(*resp) @@ -41,3 +68,85 @@ func (s *apiSuite) TestWorkshopHelpCtlNoContext(c *check.C) { _, err = rsp.MarshalJSON() c.Assert(err, check.IsNil) } + +// TestWorkshopCtlRequiresInstanceIDWithoutCookie checks that workshopctl +// requests without a hook cookie must identify their workshop instance. +func (s *apiSuite) TestWorkshopCtlRequiresInstanceIDWithoutCookie(c *check.C) { + s.daemon(c) + wctl := apiCmd("/v1/workshopctl") + buf := bytes.NewBufferString(`{"args":["get-secret","sdk.secret"]}`) + req, err := s.createProjectsRequest("POST", "/v1/workshopctl", buf) + c.Assert(err, check.IsNil) + + rsp := v1PostWorkshopCtl(wctl, req, nil).(*resp) + + c.Check(rsp.Status, check.Equals, http.StatusBadRequest) +} + +// TestWorkshopCtlRejectsUnknownInstanceID checks that a request without a hook +// cookie cannot use an instance ID outside the requesting user's workshops. +func (s *apiSuite) TestWorkshopCtlRejectsUnknownInstanceID(c *check.C) { + s.daemon(c) + wctl := apiCmd("/v1/workshopctl") + buf := bytes.NewBufferString(`{"args":["get-secret","sdk.secret"]}`) + req, err := s.createProjectsRequest("POST", "/v1/workshopctl", buf) + c.Assert(err, check.IsNil) + req = req.WithContext(context.WithValue( + req.Context(), + workshop.ContextWorkshopInstanceID, + "unknown-instance", + )) + + rsp := v1PostWorkshopCtl(wctl, req, nil).(*resp) + + c.Check(rsp.Status, check.Equals, http.StatusForbidden) +} + +// TestWorkshopCtlAcceptsOwnedInstanceID checks that a request without a hook +// cookie may run after its instance ID is matched to the requesting user. +func (s *apiSuite) TestWorkshopCtlAcceptsOwnedInstanceID(c *check.C) { + s.daemon(c) + s.addWorkshopWithInstanceID("instance-id") + + wctl := apiCmd("/v1/workshopctl") + buf := bytes.NewBufferString( + `{"args":["get-secret","sdk.secret"],"stdin":"dGVzdA=="}`, + ) + req, err := s.createProjectsRequest("POST", "/v1/workshopctl", buf) + c.Assert(err, check.IsNil) + req = req.WithContext(context.WithValue( + req.Context(), + workshop.ContextWorkshopInstanceID, + "instance-id", + )) + + rsp := v1PostWorkshopCtl(wctl, req, nil).(*resp) + + c.Check(rsp.Status, check.Equals, http.StatusOK) +} + +// TestWorkshopCtlRejectsUnknownCookie checks that an invalid cookie produces +// a generic client error even when a valid workshop instance ID is supplied. +func (s *apiSuite) TestWorkshopCtlRejectsUnknownCookie(c *check.C) { + s.daemon(c) + s.addWorkshopWithInstanceID("instance-id") + wctl := apiCmd("/v1/workshopctl") + buf := bytes.NewBufferString( + `{"context-id":"unknown-cookie","args":["get-secret","sdk.secret"]}`, + ) + req, err := s.createProjectsRequest("POST", "/v1/workshopctl", buf) + c.Assert(err, check.IsNil) + req = req.WithContext(context.WithValue( + req.Context(), + workshop.ContextWorkshopInstanceID, + "instance-id", + )) + + rsp := v1PostWorkshopCtl(wctl, req, nil).(*resp) + + c.Check(rsp.Status, check.Equals, http.StatusBadRequest) + c.Check(rsp.Type, check.Equals, ResponseTypeError) + c.Assert(rsp.Result, check.FitsTypeOf, &errorResult{}) + c.Check(rsp.Result.(*errorResult).Message, check.Equals, + "invalid workshop cookie") +} diff --git a/internal/overlord/hookstate/ctlcmd/ctlcmd.go b/internal/overlord/hookstate/ctlcmd/ctlcmd.go index cc8ccf505..888e0fadc 100644 --- a/internal/overlord/hookstate/ctlcmd/ctlcmd.go +++ b/internal/overlord/hookstate/ctlcmd/ctlcmd.go @@ -20,6 +20,7 @@ package ctlcmd import ( "bytes" + "context" "fmt" "io" @@ -97,7 +98,7 @@ type command interface { setContext(context *hookstate.Context) context() *hookstate.Context - Execute(args []string) error + Execute(ctx context.Context, args []string) error } type commandInfo struct { @@ -142,7 +143,12 @@ func (f ForbiddenCommandError) Error() string { var nonRootAllowed = []string{"get-secret", "set-health"} // Run runs the requested command. -func Run(context *hookstate.Context, args []string, uid uint32) (stdout, stderr []byte, err error) { +func Run( + ctx context.Context, + hookContext *hookstate.Context, + args []string, + uid uint32, +) (stdout, stderr []byte, err error) { if len(args) == 0 { return nil, nil, fmt.Errorf("workshopctl cannot run without args") } @@ -156,12 +162,14 @@ func Run(context *hookstate.Context, args []string, uid uint32) (stdout, stderr // Create stdout/stderr buffers, and make sure commands use them. var stdoutBuffer bytes.Buffer var stderrBuffer bytes.Buffer + activeCommands := make(map[string]command, len(commands)) for name, cmdInfo := range commands { cmd := cmdInfo.generator() cmd.setName(name) cmd.setStdout(&stdoutBuffer) cmd.setStderr(&stderrBuffer) - cmd.setContext(context) + cmd.setContext(hookContext) + activeCommands[name] = cmd theCmd, err := parser.AddCommand(name, cmdInfo.shortHelp, cmdInfo.longHelp, cmd) theCmd.Hidden = cmdInfo.hidden @@ -170,6 +178,14 @@ func Run(context *hookstate.Context, args []string, uid uint32) (stdout, stderr } } + parser.CommandHandler = func(_ flags.Commander, args []string) error { + cmd, ok := activeCommands[parser.Active.Name] + if !ok { + return fmt.Errorf("internal error: active command %q not found", parser.Active.Name) + } + return cmd.Execute(ctx, args) + } + _, err = parser.ParseArgs(args) return stdoutBuffer.Bytes(), stderrBuffer.Bytes(), err } diff --git a/internal/overlord/hookstate/ctlcmd/ctlcmd_test.go b/internal/overlord/hookstate/ctlcmd/ctlcmd_test.go index 6ec5844e4..3efb722a0 100644 --- a/internal/overlord/hookstate/ctlcmd/ctlcmd_test.go +++ b/internal/overlord/hookstate/ctlcmd/ctlcmd_test.go @@ -18,6 +18,7 @@ package ctlcmd_test import ( + "context" "strings" "testing" @@ -55,7 +56,7 @@ func (s *ctlcmdSuite) SetUpTest(c *C) { } func (s *ctlcmdSuite) TestNonExistingCommand(c *C) { - stdout, stderr, err := ctlcmd.Run(s.mockContext, []string{"foo"}, 0) + stdout, stderr, err := ctlcmd.Run(context.TODO(), s.mockContext, []string{"foo"}, 0) c.Check(string(stdout), Equals, "") c.Check(string(stderr), Equals, "") c.Check(err, ErrorMatches, ".*[Uu]nknown command.*") @@ -68,20 +69,43 @@ func (s *ctlcmdSuite) TestCommandOutput(c *C) { mockCommand.FakeStdout = "test stdout" mockCommand.FakeStderr = "test stderr" - stdout, stderr, err := ctlcmd.Run(s.mockContext, []string{"mock", "foo"}, 0) + stdout, stderr, err := ctlcmd.Run(context.TODO(), s.mockContext, []string{"mock", "foo"}, 0) c.Check(err, IsNil) c.Check(string(stdout), Equals, "test stdout") c.Check(string(stderr), Equals, "test stderr") c.Check(mockCommand.Args, DeepEquals, []string{"foo"}) } +// TestRunPassesContextToCommand verifies that Run forwards its context to the +// selected command. Commands rely on this contract to access request-scoped +// values during Execute. +func (s *ctlcmdSuite) TestRunPassesContextToCommand(c *C) { + mockCommand := ctlcmd.AddMockCommand("mock") + defer ctlcmd.RemoveCommand("mock") + + type contextKey struct{} + key := contextKey{} + ctx := context.WithValue(context.Background(), key, "test") + called := false + mockCommand.ExecuteFunc = func(ctx context.Context, _ []string) error { + called = true + c.Check(ctx.Value(key), Equals, "test") + return nil + } + + _, _, err := ctlcmd.Run(ctx, s.mockContext, []string{"mock"}, 0) + + c.Check(err, IsNil) + c.Check(called, Equals, true) +} + func (s *ctlcmdSuite) TestHiddenCommand(c *C) { ctlcmd.AddHiddenMockCommand("mock-hidden") ctlcmd.AddMockCommand("mock-shown") defer ctlcmd.RemoveCommand("mock-hidden") defer ctlcmd.RemoveCommand("mock-shown") - _, _, err := ctlcmd.Run(s.mockContext, []string{"--help"}, 0) + _, _, err := ctlcmd.Run(context.TODO(), s.mockContext, []string{"--help"}, 0) // help message output is returned as *flags.Error with // Type as flags.ErrHelp c.Assert(err, FitsTypeOf, &flags.Error{}) @@ -95,33 +119,33 @@ func (s *ctlcmdSuite) TestHiddenCommand(c *C) { } func (s *ctlcmdSuite) TestRootRequiredCommandFailure(c *C) { - _, _, err := ctlcmd.Run(s.mockContext, []string{"start"}, 1000) + _, _, err := ctlcmd.Run(context.TODO(), s.mockContext, []string{"start"}, 1000) c.Check(err, FitsTypeOf, &ctlcmd.ForbiddenCommandError{}) c.Check(err.Error(), Equals, `cannot use "start" with uid 1000, try with sudo`) } func (s *ctlcmdSuite) TestRunNoArgsFailure(c *C) { - _, _, err := ctlcmd.Run(s.mockContext, []string{}, 0) + _, _, err := ctlcmd.Run(context.TODO(), s.mockContext, []string{}, 0) c.Check(err, NotNil) } func (s *ctlcmdSuite) TestRunOnlyHelp(c *C) { - _, _, err := ctlcmd.Run(s.mockContext, []string{"-h"}, 1000) + _, _, err := ctlcmd.Run(context.TODO(), s.mockContext, []string{"-h"}, 1000) c.Check(err, NotNil) c.Assert(strings.HasPrefix(err.Error(), "Usage:"), Equals, true) - _, _, err = ctlcmd.Run(s.mockContext, []string{"--help"}, 1000) + _, _, err = ctlcmd.Run(context.TODO(), s.mockContext, []string{"--help"}, 1000) c.Check(err, NotNil) c.Assert(strings.HasPrefix(err.Error(), "Usage:"), Equals, true) } func (s *ctlcmdSuite) TestRunHelpAtAnyPosition(c *C) { - _, _, err := ctlcmd.Run(s.mockContext, []string{"set-health", "a", "-h"}, 1000) + _, _, err := ctlcmd.Run(context.TODO(), s.mockContext, []string{"set-health", "a", "-h"}, 1000) c.Check(err, NotNil) c.Assert(strings.HasPrefix(err.Error(), "Usage:"), Equals, true) - _, _, err = ctlcmd.Run(s.mockContext, []string{"set-health", "a", "b", "--help"}, 1000) + _, _, err = ctlcmd.Run(context.TODO(), s.mockContext, []string{"set-health", "a", "b", "--help"}, 1000) c.Check(err, NotNil) c.Assert(strings.HasPrefix(err.Error(), "Usage:"), Equals, true) } @@ -129,7 +153,7 @@ func (s *ctlcmdSuite) TestRunHelpAtAnyPosition(c *C) { func (s *ctlcmdSuite) TestRunNonRootAllowedCommandWithAllowedCmdAsArg(c *C) { // this test protects us against a future refactor introducing a bug that allows // a root-only command to run without root if an arg is in the nonRootAllowed list - _, _, err := ctlcmd.Run(s.mockContext, []string{"set", "get", "a"}, 1000) + _, _, err := ctlcmd.Run(context.TODO(), s.mockContext, []string{"set", "get", "a"}, 1000) c.Check(err, FitsTypeOf, &ctlcmd.ForbiddenCommandError{}) c.Check(err.Error(), Equals, `cannot use "set" with uid 1000, try with sudo`) } diff --git a/internal/overlord/hookstate/ctlcmd/export_test.go b/internal/overlord/hookstate/ctlcmd/export_test.go index 9e0562ded..2f9500a4f 100644 --- a/internal/overlord/hookstate/ctlcmd/export_test.go +++ b/internal/overlord/hookstate/ctlcmd/export_test.go @@ -14,7 +14,10 @@ package ctlcmd -import "fmt" +import ( + "context" + "fmt" +) func AddMockCommand(name string) *MockCommand { return addMockCmd(name, false) @@ -41,9 +44,13 @@ func NewMockCommand() *MockCommand { } } -func (c *MockCommand) Execute(args []string) error { +func (c *MockCommand) Execute(ctx context.Context, args []string) error { c.Args = args + if c.ExecuteFunc != nil { + return c.ExecuteFunc(ctx, args) + } + if c.FakeStdout != "" { err := c.printf("%s", c.FakeStdout) if err != nil { @@ -71,6 +78,7 @@ type MockCommand struct { ExecuteError bool FakeStdout string FakeStderr string + ExecuteFunc func(context.Context, []string) error Args []string } diff --git a/internal/overlord/hookstate/ctlcmd/getsecret.go b/internal/overlord/hookstate/ctlcmd/getsecret.go index b724b03bd..a3be4c933 100644 --- a/internal/overlord/hookstate/ctlcmd/getsecret.go +++ b/internal/overlord/hookstate/ctlcmd/getsecret.go @@ -14,7 +14,11 @@ package ctlcmd -import "github.com/canonical/workshop/internal/logger" +import ( + "context" + + "github.com/canonical/workshop/internal/logger" +) type getSecretCommand struct { baseCommand @@ -50,7 +54,7 @@ func init() { } // Execute runs the get-secret command, writing the secret value to stdout. -func (c *getSecretCommand) Execute([]string) error { +func (c *getSecretCommand) Execute(context.Context, []string) error { // Log the requested identifier only; never the resolved value. logger.Debugf("get-secret request for %q", c.Secret) diff --git a/internal/overlord/hookstate/ctlcmd/getsecret_test.go b/internal/overlord/hookstate/ctlcmd/getsecret_test.go index 14c952f08..ae99b4ab6 100644 --- a/internal/overlord/hookstate/ctlcmd/getsecret_test.go +++ b/internal/overlord/hookstate/ctlcmd/getsecret_test.go @@ -15,6 +15,8 @@ package ctlcmd_test import ( + "context" + "gopkg.in/check.v1" "github.com/canonical/workshop/internal/overlord/hookstate/ctlcmd" @@ -27,7 +29,7 @@ type getSecretSuite struct{} var _ = check.Suite(&getSecretSuite{}) func (s *getSecretSuite) TestGetSecret(c *check.C) { - stdout, stderr, err := ctlcmd.Run(nil, []string{"get-secret", "ollama.ollama-api-key"}, 0) + stdout, stderr, err := ctlcmd.Run(context.TODO(), nil, []string{"get-secret", "ollama.ollama-api-key"}, 0) c.Assert(err, check.IsNil) c.Check(string(stdout), check.Equals, "workshop-placeholder-secret") c.Check(string(stderr), check.Equals, "") @@ -36,7 +38,7 @@ func (s *getSecretSuite) TestGetSecret(c *check.C) { // TestGetSecretMissingArg checks that get-secret requires a secret // identifier argument. func (s *getSecretSuite) TestGetSecretMissingArg(c *check.C) { - _, _, err := ctlcmd.Run(nil, []string{"get-secret"}, 0) + _, _, err := ctlcmd.Run(context.TODO(), nil, []string{"get-secret"}, 0) c.Check(err, check.ErrorMatches, ".*the required argument `.` was not provided.*") } @@ -44,7 +46,7 @@ func (s *getSecretSuite) TestGetSecretMissingArg(c *check.C) { // both the socket-activated systemd path and SDK wrapper scripts invoke it // as the workshop user. func (s *getSecretSuite) TestGetSecretNonRoot(c *check.C) { - stdout, _, err := ctlcmd.Run(nil, []string{"get-secret", "ollama.ollama-api-key"}, 1000) + stdout, _, err := ctlcmd.Run(context.TODO(), nil, []string{"get-secret", "ollama.ollama-api-key"}, 1000) c.Assert(err, check.IsNil) c.Check(string(stdout), check.Equals, "workshop-placeholder-secret") } diff --git a/internal/overlord/hookstate/ctlcmd/health.go b/internal/overlord/hookstate/ctlcmd/health.go index 65710ce23..9b6797806 100644 --- a/internal/overlord/hookstate/ctlcmd/health.go +++ b/internal/overlord/hookstate/ctlcmd/health.go @@ -18,6 +18,7 @@ package ctlcmd import ( + "context" "errors" "fmt" "regexp" @@ -67,7 +68,7 @@ var ( validCode = regexp.MustCompile(`^[a-z](?:-?[a-z0-9])+$`).MatchString ) -func (c *healthCommand) Execute([]string) error { +func (c *healthCommand) Execute(context.Context, []string) error { if c.Status == "okay" && (len(c.Message) > 0 || len(c.Code) > 0) { return fmt.Errorf(`when status is "okay", message and code must be empty`) } diff --git a/internal/overlord/hookstate/ctlcmd/health_test.go b/internal/overlord/hookstate/ctlcmd/health_test.go index 544208b01..5790b4384 100644 --- a/internal/overlord/hookstate/ctlcmd/health_test.go +++ b/internal/overlord/hookstate/ctlcmd/health_test.go @@ -18,6 +18,8 @@ package ctlcmd_test import ( + "context" + "gopkg.in/check.v1" "github.com/canonical/workshop/internal/overlord/healthstate" @@ -102,13 +104,13 @@ func (s *healthSuite) TestBadArgs(c *check.C) { } for i, t := range table { - _, _, err := ctlcmd.Run(nil, t.args, 0) + _, _, err := ctlcmd.Run(context.TODO(), nil, t.args, 0) c.Check(err, check.ErrorMatches, t.err, check.Commentf("%d", i)) } } func (s *healthSuite) TestRegularRun(c *check.C) { - _, _, err := ctlcmd.Run(s.mockContext, []string{"set-health", "waiting", "message", "--code=some-code"}, 0) + _, _, err := ctlcmd.Run(context.TODO(), s.mockContext, []string{"set-health", "waiting", "message", "--code=some-code"}, 0) c.Assert(err, check.IsNil) s.mockContext.Lock() @@ -122,7 +124,7 @@ func (s *healthSuite) TestRegularRun(c *check.C) { } func (s *healthSuite) TestMessageTruncation(c *check.C) { - _, _, err := ctlcmd.Run(s.mockContext, []string{"set-health", "error", "Sometimes messages will get a little bit too verbose and this can lead to some rather nasty UX (as well as potential memory problems in extreme cases) so we kinda have to deal with that", "--code=some-code"}, 0) + _, _, err := ctlcmd.Run(context.TODO(), s.mockContext, []string{"set-health", "error", "Sometimes messages will get a little bit too verbose and this can lead to some rather nasty UX (as well as potential memory problems in extreme cases) so we kinda have to deal with that", "--code=some-code"}, 0) c.Assert(err, check.IsNil) s.mockContext.Lock() @@ -141,7 +143,7 @@ func (s *healthSuite) TestRegularRunIncorrectHook(c *check.C) { ctx, err := hookstate.NewContext(task, s.state, setup, s.mockHandler, "") c.Assert(err, check.IsNil) - _, _, err = ctlcmd.Run(ctx, []string{"set-health", "waiting", "message", "--code=some-code"}, 0) + _, _, err = ctlcmd.Run(context.TODO(), ctx, []string{"set-health", "waiting", "message", "--code=some-code"}, 0) c.Assert(err, check.ErrorMatches, `"set-health" is only allowed from a "check-health" hook`) s.mockContext.Lock() diff --git a/internal/overlord/hookstate/manager.go b/internal/overlord/hookstate/manager.go index 47e77f930..61de27825 100644 --- a/internal/overlord/hookstate/manager.go +++ b/internal/overlord/hookstate/manager.go @@ -15,7 +15,7 @@ package hookstate import ( - "fmt" + "errors" "regexp" "sync" "time" @@ -101,34 +101,20 @@ func (w *HookManager) Ensure() error { return nil } -func (m *HookManager) ephemeralContext(cookieID string) (context *Context, err error) { - var contexts map[string]string - m.state.Lock() - defer m.state.Unlock() - err = m.state.Get("workshop-cookies", &contexts) - if err != nil { - return nil, fmt.Errorf("cannot get workshop cookies: %v", err) - } - if _, ok := contexts[cookieID]; ok { - // create new ephemeral context - context, err = NewContext(nil, m.state, &HookSetup{}, nil, cookieID) - return context, err - } - return nil, fmt.Errorf("invalid workshop cookie requested") +// NewEphemeralContext returns a context that is not associated with a hook +// task. Its data is retained only for the lifetime of the context. +func (m *HookManager) NewEphemeralContext() (*Context, error) { + return NewContext(nil, m.state, &HookSetup{}, nil, "") } -// Context obtains the context for the given cookie ID. +// Context obtains the active hook context for the given cookie ID. func (m *HookManager) Context(cookieID string) (*Context, error) { m.contextsMutex.RLock() defer m.contextsMutex.RUnlock() - var err error context, ok := m.contexts[cookieID] if !ok { - context, err = m.ephemeralContext(cookieID) - if err != nil { - return nil, err - } + return nil, errors.New("invalid workshop cookie requested") } return context, nil diff --git a/internal/overlord/hookstate/manager_test.go b/internal/overlord/hookstate/manager_test.go new file mode 100644 index 000000000..cc70cbc14 --- /dev/null +++ b/internal/overlord/hookstate/manager_test.go @@ -0,0 +1,71 @@ +// Copyright (c) 2026 Canonical Ltd +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License version 3 as +// published by the Free Software Foundation. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . + +package hookstate + +import ( + . "gopkg.in/check.v1" + + "github.com/canonical/workshop/internal/overlord/state" +) + +type managerSuite struct { + state *state.State + manager *HookManager +} + +var _ = Suite(&managerSuite{}) + +func (s *managerSuite) SetUpTest(*C) { + s.state = state.New(nil) + s.manager = &HookManager{state: s.state} +} + +// TestContextReturnsActiveContext checks that cookie lookup returns the +// registered hook context without requiring persisted cookies. +func (s *managerSuite) TestContextReturnsActiveContext(c *C) { + s.state.Lock() + task := s.state.NewTask("run-hook", "Run test hook") + s.state.Unlock() + active, err := NewContext(task, s.state, &HookSetup{}, nil, "cookie-id") + c.Assert(err, IsNil) + s.manager.contexts = map[string]*Context{active.ID(): active} + + ctx, err := s.manager.Context(active.ID()) + + c.Assert(err, IsNil) + c.Check(ctx, Equals, active) + c.Check(ctx.IsEphemeral(), Equals, false) +} + +// TestContextRejectsUnknownCookie checks that an unregistered cookie returns +// an invalid-cookie error rather than a state lookup error. +func (s *managerSuite) TestContextRejectsUnknownCookie(c *C) { + ctx, err := s.manager.Context("unknown-cookie") + + c.Check(ctx, IsNil) + c.Check(err, ErrorMatches, "invalid workshop cookie requested") +} + +// TestNewEphemeralContext checks that the manager creates a context with its +// state but without a hook task, which is the contract required by +// cookie-less workshopctl requests after their instance ID is validated. +func (s *managerSuite) TestNewEphemeralContext(c *C) { + ctx, err := s.manager.NewEphemeralContext() + + c.Assert(err, IsNil) + c.Check(ctx.IsEphemeral(), Equals, true) + c.Check(ctx.State(), Equals, s.state) + c.Check(ctx.ID(), Not(Equals), "") +} diff --git a/internal/overlord/workshopstate/manager.go b/internal/overlord/workshopstate/manager.go index 73a32a111..45e642f08 100644 --- a/internal/overlord/workshopstate/manager.go +++ b/internal/overlord/workshopstate/manager.go @@ -126,6 +126,47 @@ func (w *WorkshopManager) Workshop(ctx context.Context, name, pId string) (*work return workshop, nil } +// OwnsWorkshopInstanceID reports whether the user in ctx owns a workshop with +// the specified backend instance identifier. If ctx does not identify a user, +// it returns false without an error. +func (w *WorkshopManager) OwnsWorkshopInstanceID( + ctx context.Context, + instanceID string, +) (bool, error) { + if instanceID == "" { + return false, nil + } + + projects, err := w.backend.UserProjects(ctx) + if err != nil { + return false, err + } + + for _, project := range projects { + projectCtx := context.WithValue( + ctx, + workshop.ContextProjectId, + project.ProjectId, + ) + workshops, err := w.backend.ProjectWorkshops(projectCtx) + if err != nil { + return false, fmt.Errorf( + "cannot list workshops for project %q: %w", + project.ProjectId, + err, + ) + } + + for _, candidate := range workshops { + if candidate.InstanceID == instanceID { + return true, nil + } + } + } + + return false, nil +} + // Returns latest file for a workshop. The state must be locked, as listing // projects can update project metadata. func (w *WorkshopManager) WorkshopFile(ctx context.Context, name, pId string) (*workshop.File, error) { diff --git a/internal/overlord/workshopstate/manager_test.go b/internal/overlord/workshopstate/manager_test.go index b29441acc..279518d08 100644 --- a/internal/overlord/workshopstate/manager_test.go +++ b/internal/overlord/workshopstate/manager_test.go @@ -15,6 +15,8 @@ package workshopstate_test import ( + "context" + "gopkg.in/check.v1" "github.com/canonical/workshop/internal/overlord/state" @@ -26,7 +28,7 @@ import ( type managerSuite struct { state *state.State - backend workshop.Backend + backend *fakebackend.FakeWorkshopBackend runner *state.TaskRunner manager *workshopstate.WorkshopManager } @@ -43,6 +45,29 @@ func (s *managerSuite) SetUpTest(c *check.C) { s.manager = workshopstate.New(s.state, s.runner) } +func (s *managerSuite) addWorkshop( + c *check.C, + ctx context.Context, + name string, + instanceID string, +) { + project, _, err := s.backend.CreateOrLoadProject(ctx, c.MkDir()) + c.Assert(err, check.IsNil) + + if s.backend.Workshops[project.ProjectId] == nil { + s.backend.Workshops[project.ProjectId] = make( + map[string]*fakebackend.FakeWorkshop, + ) + } + s.backend.Workshops[project.ProjectId][name] = &fakebackend.FakeWorkshop{ + Workshop: &workshop.Workshop{ + Name: name, + Project: *project, + InstanceID: instanceID, + }, + } +} + func (s *managerSuite) TestAddHandlers(c *check.C) { workshopstate.New(s.state, s.runner) @@ -63,3 +88,80 @@ func (s *managerSuite) TestAddHandlers(c *check.C) { "remove-state-storage", }) } + +// TestOwnsWorkshopInstanceIDReturnsTrueForOwnedWorkshop checks that the +// manager finds an instance ID belonging to a workshop owned by the user in +// context. +func (s *managerSuite) TestOwnsWorkshopInstanceIDReturnsTrueForOwnedWorkshop( + c *check.C, +) { + ctx := context.WithValue( + context.Background(), + workshop.ContextUser, + "test-user", + ) + s.addWorkshop(c, ctx, "test-workshop", "instance-id") + + owns, err := s.manager.OwnsWorkshopInstanceID(ctx, "instance-id") + + c.Assert(err, check.IsNil) + c.Check(owns, check.Equals, true) +} + +// TestOwnsWorkshopInstanceIDReturnsFalseForUnknownID checks that the manager +// rejects an instance ID that does not identify one of the user's workshops. +func (s *managerSuite) TestOwnsWorkshopInstanceIDReturnsFalseForUnknownID( + c *check.C, +) { + ctx := context.WithValue( + context.Background(), + workshop.ContextUser, + "test-user", + ) + s.addWorkshop(c, ctx, "test-workshop", "instance-id") + + owns, err := s.manager.OwnsWorkshopInstanceID(ctx, "unknown-id") + + c.Assert(err, check.IsNil) + c.Check(owns, check.Equals, false) +} + +// TestOwnsWorkshopInstanceIDReturnsFalseForEmptyID checks that an empty +// instance ID is not considered to be owned by the user. +func (s *managerSuite) TestOwnsWorkshopInstanceIDReturnsFalseForEmptyID( + c *check.C, +) { + ctx := context.WithValue( + context.Background(), + workshop.ContextUser, + "test-user", + ) + + owns, err := s.manager.OwnsWorkshopInstanceID(ctx, "") + + c.Assert(err, check.IsNil) + c.Check(owns, check.Equals, false) +} + +// TestOwnsWorkshopInstanceIDReturnsFalseForAnotherUsersWorkshop checks that +// workshop ownership is scoped to the user in context. +func (s *managerSuite) TestOwnsWorkshopInstanceIDReturnsFalseForAnotherUsersWorkshop( + c *check.C, +) { + ownerCtx := context.WithValue( + context.Background(), + workshop.ContextUser, + "owner", + ) + s.addWorkshop(c, ownerCtx, "test-workshop", "instance-id") + requestCtx := context.WithValue( + context.Background(), + workshop.ContextUser, + "another-user", + ) + + owns, err := s.manager.OwnsWorkshopInstanceID(requestCtx, "instance-id") + + c.Assert(err, check.IsNil) + c.Check(owns, check.Equals, false) +}