Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

No panic on invalid Statement.Resource type #9

Merged
merged 1 commit into from
Dec 3, 2019
Merged

No panic on invalid Statement.Resource type #9

merged 1 commit into from
Dec 3, 2019

Conversation

ewbankkit
Copy link
Contributor

Fixes #8.

$ go test
PASS
ok  	github.com/jen20/awspolicyequivalence	0.004s

Without the if _, ok := action.(string); !ok code the test panics:

$ go test
--- FAIL: TestPolicyEquivalence (0.00s)
panic: interface conversion: interface {} is float64, not string [recovered]
	panic: interface conversion: interface {} is float64, not string

goroutine 8 [running]:
testing.tRunner.func1(0xc00008c200)
	/usr/local/go/src/testing/testing.go:874 +0x3a3
panic(0x558bc0, 0xc00013b800)
	/usr/local/go/src/runtime/panic.go:679 +0x1b2
github.com/jen20/awspolicyequivalence.newAWSStringSet(0x547cc0, 0xc0001395c0, 0x0, 0x6c2788, 0x0)
	/home/kit/wrk/src/github.com/jen20/awspolicyequivalence/aws_policy_equivalence.go:386 +0x212
github.com/jen20/awspolicyequivalence.(*awsPolicyStatement).equals(0xc00012bcb0, 0xc00012bd40, 0x0)
	/home/kit/wrk/src/github.com/jen20/awspolicyequivalence/aws_policy_equivalence.go:175 +0x299
github.com/jen20/awspolicyequivalence.(*awsPolicyDocument).equals(0xc000128b00, 0xc000128b80, 0x0)
	/home/kit/wrk/src/github.com/jen20/awspolicyequivalence/aws_policy_equivalence.go:114 +0xea
github.com/jen20/awspolicyequivalence.PoliciesAreEquivalent(0x592f2c, 0xbc, 0x592f2c, 0xbc, 0x0, 0x0, 0x0)
	/home/kit/wrk/src/github.com/jen20/awspolicyequivalence/aws_policy_equivalence.go:50 +0x361
github.com/jen20/awspolicyequivalence.TestPolicyEquivalence(0xc00008c200)
	/home/kit/wrk/src/github.com/jen20/awspolicyequivalence/aws_policy_equivalence_test.go:304 +0x115
testing.tRunner(0xc00008c200, 0x5953e8)
	/usr/local/go/src/testing/testing.go:909 +0xc9
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:960 +0x350
exit status 2
FAIL	github.com/jen20/awspolicyequivalence	0.005s

ewbankkit
ewbankkit commented Oct 18, 2019
View reviewed changes
aws_policy_equivalence.go
@@ -383,6 +383,10 @@ func newAWSStringSet(members interface{}) awsStringSet {
}
actions := make([]string, len(multiple))
for i, action := range multiple {
if _, ok := action.(string); !ok {
return nil
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Return nil as this routine returns no error and I didn't want to touch all call sites.

ewbankkit
ewbankkit commented Oct 18, 2019
View reviewed changes
aws_policy_equivalence.go
@@ -396,6 +400,10 @@ func newAWSPrincipalStringSet(members interface{}) awsPrincipalStringSet {
}

func (actions awsStringSet) equals(other awsStringSet) bool {
if actions == nil || other == nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need this or else any two invalid Resource statements (or NotResource, Action, NotAction etc.) would be equal.

ewbankkit
ewbankkit commented Oct 18, 2019
View reviewed changes
aws_policy_equivalence_test.go
@@ -272,6 +272,32 @@ func TestPolicyEquivalence(t *testing.T) {
policy2: policyTest29b,
equivalent: true,
},
{
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also added these tests for missing and incorrect Statement types.

This was referenced Oct 18, 2019
Closed
Closed
@jen20 jen20 merged commit 8b377d2 into jen20:master Dec 3, 2019
@jen20
Copy link
Owner

jen20 commented Dec 3, 2019

Thanks @ewbankkit! I've tagged v1.1.0 of this library too include this fix.

@ewbankkit ewbankkit deleted the issue-8 branch December 3, 2019 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Panic on invalid Statement.Resource value type
2 participants
@ewbankkit @jen20