Add missing API-level state checks during session cancel (#1223)

* Add missing API-level state checks during session cancel

This validates it's not already canceled or in terminated state.

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)
 

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

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 {