diff --git a/CHANGELOG.md b/CHANGELOG.md index e1292fe42d..519415433f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,9 @@ Canonical reference for changes, improvements, and bugfixes for Boundary. ([Issue 1](https://github.com/hashicorp/boundary/issues/894), [Issue 2](https://github.com/hashicorp/boundary/issues/1055), [PR](https://github.com/hashicorp/boundary/pull/1220)) +* sessions: Add some missing API-level checks when session cancelation was + requested. It's much easier than interpreting the domain-level check failures. + [PR](https://github.com/hashicorp/boundary/pull/1223)) ## 0.2.1 (2021/05/05) diff --git a/internal/servers/controller/handlers/sessions/session_service.go b/internal/servers/controller/handlers/sessions/session_service.go index 7dd58fe458..ecd242393a 100644 --- a/internal/servers/controller/handlers/sessions/session_service.go +++ b/internal/servers/controller/handlers/sessions/session_service.go @@ -209,6 +209,8 @@ func (s Service) CancelSession(ctx context.Context, req *pbs.CancelSessionReques return nil, authResults.Error } + // We'll verify it's not already canceled, but after checking auth so as not + // to leak that information. ses, err := s.getFromRepo(ctx, req.GetId()) if err != nil { return nil, err @@ -235,6 +237,15 @@ func (s Service) CancelSession(ctx context.Context, req *pbs.CancelSessionReques } } + for _, state := range ses.States { + switch state.Status { + case session.StatusCanceling: + return nil, errors.New(errors.InvalidSessionState, op, "session already in canceling state") + case session.StatusTerminated: + return nil, errors.New(errors.InvalidSessionState, op, "session already terminated") + } + } + ses, err = s.cancelInRepo(ctx, req.GetId(), req.GetVersion()) if err != nil { return nil, err diff --git a/internal/servers/controller/handlers/sessions/session_service_test.go b/internal/servers/controller/handlers/sessions/session_service_test.go index 35cd45cbf6..37169f33f1 100644 --- a/internal/servers/controller/handlers/sessions/session_service_test.go +++ b/internal/servers/controller/handlers/sessions/session_service_test.go @@ -2,7 +2,6 @@ package sessions_test import ( "context" - "errors" "fmt" "net/http/httptest" "testing" @@ -12,6 +11,7 @@ import ( "github.com/hashicorp/boundary/internal/auth" "github.com/hashicorp/boundary/internal/authtoken" "github.com/hashicorp/boundary/internal/db" + "github.com/hashicorp/boundary/internal/errors" "github.com/hashicorp/boundary/internal/gen/controller/api/resources/scopes" pb "github.com/hashicorp/boundary/internal/gen/controller/api/resources/sessions" pbs "github.com/hashicorp/boundary/internal/gen/controller/api/services" @@ -517,6 +517,13 @@ func TestCancel(t *testing.T) { req: &pbs.CancelSessionRequest{Id: sess.GetPublicId()}, res: &pbs.CancelSessionResponse{Item: wireSess}, }, + { + name: "Already canceled", + scopeId: sess.ScopeId, + req: &pbs.CancelSessionRequest{Id: sess.GetPublicId()}, + res: nil, + err: errors.New(errors.InvalidSessionState, "sessions.(Service).CancelSession", "session already in canceling state"), + }, { name: "Cancel a non existing Session", req: &pbs.CancelSessionRequest{Id: session.SessionPrefix + "_DoesntExis"}, @@ -548,7 +555,14 @@ func TestCancel(t *testing.T) { got, gErr := s.CancelSession(auth.DisabledAuthTestContext(iamRepoFn, tc.scopeId), tc.req) if tc.err != nil { require.Error(gErr) - assert.True(errors.Is(gErr, tc.err), "GetSession(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + // It's hard to mix and match api/error package errors right now + // so use old/new behavior depending on the type. If validate + // gets updated this can be standardized. + if errors.Match(errors.T(errors.InvalidSessionState), gErr) { + assert.True(errors.Match(errors.T(tc.err), gErr), "GetSession(%+v) got error %#v, wanted %#v", tc.req, gErr, tc.err) + } else { + assert.True(errors.Is(gErr, tc.err), "GetSession(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + } } if tc.res == nil {