diff --git a/lib/services/access_request.go b/lib/services/access_request.go index 6d254b5887ca0..79054abd9dd6f 100644 --- a/lib/services/access_request.go +++ b/lib/services/access_request.go @@ -378,17 +378,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) diff --git a/lib/services/access_request_test.go b/lib/services/access_request_test.go index 5eae157d8df1b..f423d891ea6dc 100644 --- a/lib/services/access_request_test.go +++ b/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) }