From 13251f572e8f78ab52c1009f23a6777852d3041d Mon Sep 17 00:00:00 2001 From: Peter Van Bouwel Date: Sun, 17 Nov 2024 15:43:25 +0100 Subject: [PATCH] tests: improve iamaction and api action tests A convoluted way was created where a stub is created that always returns an error and where the return value is encapsulated in the error message. Given to how error message are build it was tricky to extract them and while this case worked for most actions it did not work for HeadObject. Given that these unittest just run in one execution environment it is easier to just introduce a global which gets updated by the stub. So after calling the stub the global will contain the created value. So this approach is easier and more reliable. Since our stub is called by anonymous requests we should not use session data with valid tags as it makes the tests a bit confusing so changed those. --- cmd/policy-api-action_test.go | 22 +++---- cmd/policy-iam-action.go | 3 + cmd/policy-iam-action_test.go | 108 ++++++++++++++-------------------- 3 files changed, 55 insertions(+), 78 deletions(-) diff --git a/cmd/policy-api-action_test.go b/cmd/policy-api-action_test.go index b1c0a4a..721701f 100644 --- a/cmd/policy-api-action_test.go +++ b/cmd/policy-api-action_test.go @@ -3,10 +3,7 @@ package cmd import ( "errors" "net/http" - "strings" "testing" - - sg "github.com/aws/smithy-go" ) @@ -14,11 +11,14 @@ type StubJustReturnApiAction struct{ t *testing.T } +var globalLastApiActionStubJustReturnApiAction S3ApiAction = "" + func (p *StubJustReturnApiAction) Build(action S3ApiAction, presigned bool) http.HandlerFunc{ return func (w http.ResponseWriter, r *http.Request) { //AWS CLI expects certain structure for ok responses //For error we could use the message field to pass a message regardless //of the api action + globalLastApiActionStubJustReturnApiAction = action writeS3ErrorResponse( buildContextWithRequestID(r), w, @@ -41,18 +41,12 @@ func TestExpectedAPIActionIdentified(t *testing.T) { for _, tc := range getApiAndIAMActionTestCases() { //see policy_iam_action_test err := tc.ApiCall(t) - smityError, ok := err.(*sg.OperationError) - if !ok { - t.Errorf("err was not smithy error %s", err) + if err == nil { + t.Errorf("%s: an error should have been returned", tc.ApiAction) } - accessDeniedParts := strings.Split(smityError.Error(), "AccessDenied: ") - if len(accessDeniedParts) < 2 { - t.Errorf("Encountered unexpected error (not Access Denied) %s", smityError) - continue - } - msg := accessDeniedParts[1] - if msg != tc.ApiAction { - t.Errorf("Expected %s, got %s, bug in router code", tc.ApiAction, msg) + + if tc.ApiAction != string(globalLastApiActionStubJustReturnApiAction) { + t.Errorf("wrong APIAction identified; expected %s, got %s", tc.ApiAction, globalLastApiActionStubJustReturnApiAction) } } } \ No newline at end of file diff --git a/cmd/policy-iam-action.go b/cmd/policy-iam-action.go index 3229bc7..97858cd 100644 --- a/cmd/policy-iam-action.go +++ b/cmd/policy-iam-action.go @@ -68,6 +68,9 @@ func addGenericSessionContextKeys(context map[string]*policy.ConditionValue, ses //Add aws:PrincipalTag/tag-key keys that are added to nearly all requests that contain information about the current session //https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_condition-keys.html#condition-keys-principaltag func addAwsPrincipalTagConditionKeys(context map[string]*policy.ConditionValue, session *PolicySessionData) { + if session == nil { + return + } for tagKey, tagValues := range session.Tags.PrincipalTags { context[fmt.Sprintf("aws:PrincipalTag/%s", tagKey)] = policy.NewConditionValueString(true, tagValues...) } diff --git a/cmd/policy-iam-action_test.go b/cmd/policy-iam-action_test.go index af6c2e2..bc4d2de 100644 --- a/cmd/policy-iam-action_test.go +++ b/cmd/policy-iam-action_test.go @@ -8,13 +8,11 @@ import ( "fmt" "net/http" "reflect" - "strings" "testing" "time" "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/service/s3" - sg "github.com/aws/smithy-go" "github.com/micahhausler/aws-iam-policy/policy" ) @@ -22,13 +20,16 @@ type StubJustReturnIamAction struct{ t *testing.T } +var latestIamActionInStubReturnIamAction []iamAction = nil + func (p *StubJustReturnIamAction) Build(action S3ApiAction, presigned bool) http.HandlerFunc{ return func (w http.ResponseWriter, r *http.Request) { - actions, err := newIamActionsFromS3Request(action, r, testSessionDataTestDepartment) + actions, err := newIamActionsFromS3Request(action, r, nil) if err != nil { p.t.Error(err) return } + latestIamActionInStubReturnIamAction = actions bytes, err := json.Marshal(actions) if err != nil { p.t.Error(err) @@ -146,21 +147,20 @@ func runGetObjectAndReturnError(t *testing.T) error { return err } -// TODO: Check how to get this under test coverage -// func runHeadObjectAndReturnError(t *testing.T) error { -// client, max1Sec, cancel := getAnonymousS3TestClient(t) - -// input := s3.HeadObjectInput{ -// Bucket: &testBucketName, -// Key: &putObjectTestKey, -// } -// defer cancel() -// _, err := client.HeadObject(max1Sec, &input) -// if err == nil { -// t.Error("Should have encountered error but did not") -// } -// return err -// } +func runHeadObjectAndReturnError(t *testing.T) error { + client, max1Sec, cancel := getAnonymousS3TestClient(t) + + input := s3.HeadObjectInput{ + Bucket: &testBucketName, + Key: &putObjectTestKey, + } + defer cancel() + _, err := client.HeadObject(max1Sec, &input) + if err == nil { + t.Error("Should have encountered error but did not") + } + return err +} func runAbortMultipartUploadAndReturnError(t *testing.T) error { client, max1Sec, cancel := getAnonymousS3TestClient(t) @@ -269,7 +269,7 @@ func getApiAndIAMActionTestCases() ([]apiAndIAMActionTestCase) { ApiAction: "ListObjectsV2", ApiCall: runListObjectsV2AndReturnError, ExpectedActions: []iamAction{ - newIamAction(IAMActionS3ListBucket, testBucketARN, testSessionDataTestDepartment).addContext(contextType{ + newIamAction(IAMActionS3ListBucket, testBucketARN, nil).addContext(contextType{ IAMConditionS3Prefix: policy.NewConditionValueString(true, ""), }), }, @@ -278,7 +278,7 @@ func getApiAndIAMActionTestCases() ([]apiAndIAMActionTestCase) { ApiAction: "ListObjectsV2", ApiCall: runListObjectsV2WithPrefixAndReturnError, ExpectedActions: []iamAction{ - newIamAction(IAMActionS3ListBucket, testBucketARN, testSessionDataTestDepartment).addContext(contextType{ + newIamAction(IAMActionS3ListBucket, testBucketARN, nil).addContext(contextType{ IAMConditionS3Prefix: policy.NewConditionValueString(true, listobjectv2_test_prefix), }), }, @@ -287,38 +287,37 @@ func getApiAndIAMActionTestCases() ([]apiAndIAMActionTestCase) { ApiAction: "PutObject", ApiCall: runPutObjectAndReturnError, ExpectedActions: []iamAction{ - newIamAction(IAMActionS3PutObject, putObjectFullObjectARN, testSessionDataTestDepartment), + newIamAction(IAMActionS3PutObject, putObjectFullObjectARN, nil), }, }, { ApiAction: "GetObject", ApiCall: runGetObjectAndReturnError, ExpectedActions: []iamAction{ - newIamAction(IAMActionS3GetObject, putObjectFullObjectARN, testSessionDataTestDepartment), + newIamAction(IAMActionS3GetObject, putObjectFullObjectARN, nil), + }, + }, + { + ApiAction: "HeadObject", + ApiCall: runHeadObjectAndReturnError, + ExpectedActions: []iamAction{ + // https://docs.aws.amazon.com/AmazonS3/latest/API/API_HeadObject.html + // To use HEAD, you must have the s3:GetObject permission. + newIamAction(IAMActionS3GetObject, putObjectFullObjectARN, nil), }, }, - // TODO: HeadObject behaves different client side and overrides the error so our hacky way of testing does not work - // { - // ApiAction: "HeadObject", - // ApiCall: runHeadObjectAndReturnError, - // ExpectedActions: []iamAction{ - // // https://docs.aws.amazon.com/AmazonS3/latest/API/API_HeadObject.html - // // To use HEAD, you must have the s3:GetObject permission. - // NewIamAction(IAMActionS3GetObject, putObjectFullObjectARN, nil), - // }, - // }, { ApiAction: "ListBuckets", ApiCall: runListBucketsAndReturnError, ExpectedActions: []iamAction{ - newIamAction(IAMActionS3ListAllMyBuckets, "*", testSessionDataTestDepartment), + newIamAction(IAMActionS3ListAllMyBuckets, "*", nil), }, }, { ApiAction: "AbortMultipartUpload", ApiCall: runAbortMultipartUploadAndReturnError, ExpectedActions: []iamAction{ - newIamAction(IAMActionS3AbortMultipartUpload, putObjectFullObjectARN, testSessionDataTestDepartment), + newIamAction(IAMActionS3AbortMultipartUpload, putObjectFullObjectARN, nil), }, }, { @@ -327,7 +326,7 @@ func getApiAndIAMActionTestCases() ([]apiAndIAMActionTestCase) { ExpectedActions: []iamAction{ //https://docs.aws.amazon.com/AmazonS3/latest/userguide/mpuoverview.html //You must be allowed to perform the s3:PutObject action on an object to initiate multipart upload. - newIamAction(IAMActionS3PutObject, putObjectFullObjectARN, testSessionDataTestDepartment), + newIamAction(IAMActionS3PutObject, putObjectFullObjectARN, nil), }, }, { @@ -336,7 +335,7 @@ func getApiAndIAMActionTestCases() ([]apiAndIAMActionTestCase) { ExpectedActions: []iamAction{ //https://docs.aws.amazon.com/AmazonS3/latest/userguide/mpuoverview.html //You must be allowed to perform the s3:PutObject action on an object to initiate multipart upload. - newIamAction(IAMActionS3PutObject, putObjectFullObjectARN, testSessionDataTestDepartment), + newIamAction(IAMActionS3PutObject, putObjectFullObjectARN, nil), }, }, { @@ -345,7 +344,7 @@ func getApiAndIAMActionTestCases() ([]apiAndIAMActionTestCase) { ExpectedActions: []iamAction{ //https://docs.aws.amazon.com/AmazonS3/latest/userguide/mpuoverview.html //You must be allowed to perform the s3:PutObject action on an object to initiate multipart upload. - newIamAction(IAMActionS3PutObject, putObjectFullObjectARN, testSessionDataTestDepartment), + newIamAction(IAMActionS3PutObject, putObjectFullObjectARN, nil), }, }, } @@ -365,34 +364,15 @@ func TestExpectedIamActionsAreReturned(t *testing.T) { for _, tc := range getApiAndIAMActionTestCases() { err := tc.ApiCall(t) - smityError, ok := err.(*sg.OperationError) - if !ok { - t.Errorf("err was not smithy error %s", err) - continue - } - accessDeniedParts := strings.Split(smityError.Error(), "AccessDenied: ") - if len(accessDeniedParts) < 2 { - t.Errorf("Encountered unexpected error (not Access Denied) %s", smityError) - continue + if err == nil { + t.Errorf("%s: by design the stub should return an error but we did not get one.", tc.ApiAction) + t.FailNow() } - msg := accessDeniedParts[1] - var actions []iamAction - err = json.Unmarshal([]byte(msg), &actions) - if err != nil { - t.Error(err) - } - if !reflect.DeepEqual(actions, tc.ExpectedActions) { - if len(actions) != len(tc.ExpectedActions) { - printPointerAndJSONStringComparison(t, tc.ApiAction, tc.ExpectedActions, actions) - } else { - //Same amount of actions string and pointer representations might not show the issue let's compare 1-by 1 - for i, action := range actions { - expectedAction := tc.ExpectedActions[i] - if !reflect.DeepEqual(action, expectedAction) { - printPointerAndJSONStringComparison(t, tc.ApiAction, expectedAction, action) - } - } - } + + if !reflect.DeepEqual(latestIamActionInStubReturnIamAction, tc.ExpectedActions) { + printPointerAndJSONStringComparison(t, tc.ApiAction, tc.ExpectedActions, latestIamActionInStubReturnIamAction) + t.Errorf("unexpected actions got %v, expected %v", latestIamActionInStubReturnIamAction, tc.ExpectedActions) } + } } \ No newline at end of file