Review Requests: prevent reviews after request is resolved

lib/services/access_request.go

@@ -396,17 +396,19 @@ func ApplyAccessReview(req types.AccessRequest, rev types.AccessReview, author t
 		rev.Created = time.Now()
 	}
 
-	// set threshold indexes and store the review
+	// set threshold indexes
 	rev.ThresholdIndexes = tids
-	req.SetReviews(append(req.GetReviews(), rev))
 
-	// if request has already exited the pending state, then no further work
-	// needs to be done (subsequent reviews have no effect after initial
-	// state-transition).
-	if !req.GetState().IsPending() {
-		return nil
+	// Resolved requests should not be updated.
+	switch {
+	case req.GetState().IsApproved():
+		return trace.AccessDenied("the access request has been already approved")
+	case req.GetState().IsDenied():
+		return trace.AccessDenied("the access request has been already denied")
 	}
 
+	req.SetReviews(append(req.GetReviews(), rev))
+
 	// request is still pending, so check to see if this
 	// review introduces a state-transition.
 	res, err := calculateReviewBasedResolution(req)

lib/services/access_request_test.go

@@ -278,6 +278,8 @@ func TestReviewThresholds(t *testing.T) {
 		propose types.RequestState
 		// expect is the expected post-review state of the request (defaults to pending)
 		expect types.RequestState
+
+		errCheck require.ErrorAssertionFunc
 	}
 
 	tts := []struct {
@@ -314,11 +316,62 @@ func TestReviewThresholds(t *testing.T) {
 					propose: approve,
 					expect:  approve,
 				},
-				{ // adds second denial to all thresholds, no effect since a state-transition was already triggered.
+			},
+		},
+		{
+			desc:      "trying to deny an already approved request",
+			requestor: "alice", // permitted by role populist
+			reviews: []review{
+				{ // cannot review own requests
+					author:   "alice",
+					noReview: true,
+				},
+				{ // no matching allow directives
+					author:   g.user(t, "military"),
+					noReview: true,
+				},
+				{ // adds one approval to all thresholds
+					author:  g.user(t, "proletariat", "intelligentsia", "military"),
+					propose: approve,
+				},
+				{ // adds one denial to all thresholds
 					author:  g.user(t, "proletariat", "intelligentsia", "military"),
 					propose: deny,
+				},
+				{ // adds second approval to all thresholds, triggers "uprising".
+					author:  g.user(t, "proletariat", "intelligentsia", "military"),
+					propose: approve,
 					expect:  approve,
 				},
+				{ // adds second denial but request was already approved.
+					author:  g.user(t, "proletariat", "intelligentsia", "military"),
+					propose: deny,
+					errCheck: func(tt require.TestingT, err error, i ...interface{}) {
+						require.ErrorIs(tt, err, trace.AccessDenied("the access request has been already approved"), i...)
+					},
+				},
+			},
+		},
+		{
+			desc:      "trying to approve an already denied request",
+			requestor: "bob", // permitted by role general
+			reviews: []review{
+				{ // 1 of 2 required denials
+					author:  g.user(t, "military"),
+					propose: deny,
+				},
+				{ // 2 of 2 required denials
+					author:  g.user(t, "military"),
+					propose: deny,
+					expect:  deny,
+				},
+				{ // tries to approve but it was already denied
+					author:  g.user(t, "military"),
+					propose: approve,
+					errCheck: func(tt require.TestingT, err error, i ...interface{}) {
+						require.ErrorIs(tt, err, trace.AccessDenied("the access request has been already denied"), i...)
+					},
+				},
 			},
 		},
 		{
@@ -574,6 +627,11 @@ func TestReviewThresholds(t *testing.T) {
 			require.True(t, ok, "scenario=%q, rev=%d", tt.desc, ri)
 
 			err = ApplyAccessReview(req, rev, author)
+			if rt.errCheck != nil {
+				rt.errCheck(t, err)
+				continue
+			}
+
 			require.NoError(t, err, "scenario=%q, rev=%d", tt.desc, ri)
 			require.Equal(t, rt.expect.String(), req.GetState().String(), "scenario=%q, rev=%d", tt.desc, ri)
 		}