Skip to content

Commit

Permalink
tests: improve iamaction and api action tests
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
Peter Van Bouwel committed Nov 17, 2024
1 parent 9a1fe97 commit 13251f5
Show file tree
Hide file tree
Showing 3 changed files with 55 additions and 78 deletions.
22 changes: 8 additions & 14 deletions cmd/policy-api-action_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,22 @@ package cmd
import (
"errors"
"net/http"
"strings"
"testing"

sg "github.com/aws/smithy-go"
)


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,
Expand All @@ -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)
}
}
}
3 changes: 3 additions & 0 deletions cmd/policy-iam-action.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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...)
}
Expand Down
108 changes: 44 additions & 64 deletions cmd/policy-iam-action_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,27 +8,28 @@ 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"
)

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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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, ""),
}),
},
Expand All @@ -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),
}),
},
Expand All @@ -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),
},
},
{
Expand All @@ -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),
},
},
{
Expand All @@ -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),
},
},
{
Expand All @@ -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),
},
},
}
Expand All @@ -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)
}

}
}

0 comments on commit 13251f5

Please sign in to comment.