Repository: SkygearIO/skygear-server (go)

Issue:
Incorrect authz policy order
Currently, some API requires the request contains either a valid session, or master key. This is represented as:
```go
policy.AnyOf(
	authz.PolicyFunc(policy.RequireMasterKey),
	policy.AllOf(
		authz.PolicyFunc(policy.DenyNoAccessKey),
		policy.RequireValidUser,
	),
)
```

However, due to ordering of the policies, if the session is expired, the produced error reason is `AccessKeyNotAccepted`, instead of the expected reason `NotAuthenticated`.  Client SDK would not refresh the session automatically and breaks the session.

To fix it, the order should be changed so that `NotAuthenticated` would be produced.

Pull request:
Fix compound authz policy short-circuit behavior
fix #1218 

Patch:
diff --git a/pkg/asset/handler/presign_upload.go b/pkg/asset/handler/presign_upload.go
index 667a6f09..f5ecb2c6 100644
--- a/pkg/asset/handler/presign_upload.go
+++ b/pkg/asset/handler/presign_upload.go
@@ -82,13 +82,7 @@ type PresignUploadHandler struct {
 }
 
 func (h *PresignUploadHandler) ProvideAuthzPolicy() authz.Policy {
-	return policy.AnyOf(
-		authz.PolicyFunc(policy.RequireMasterKey),
-		policy.AllOf(
-			authz.PolicyFunc(policy.DenyNoAccessKey),
-			policy.RequireValidUser,
-		),
-	)
+	return policy.RequireValidUserOrMasterKey
 }
 
 // @JSONSchema
diff --git a/pkg/asset/handler/presign_upload_form.go b/pkg/asset/handler/presign_upload_form.go
index 72def4f7..75d4b884 100644
--- a/pkg/asset/handler/presign_upload_form.go
+++ b/pkg/asset/handler/presign_upload_form.go
@@ -79,13 +79,7 @@ type PresignUploadFormHandler struct {
 }
 
 func (h *PresignUploadFormHandler) ProvideAuthzPolicy() authz.Policy {
-	return policy.AnyOf(
-		authz.PolicyFunc(policy.RequireMasterKey),
-		policy.AllOf(
-			authz.PolicyFunc(policy.DenyNoAccessKey),
-			policy.RequireValidUser,
-		),
-	)
+	return policy.RequireValidUserOrMasterKey
 }
 
 func (h *PresignUploadFormHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
diff --git a/pkg/auth/handler/update_metadata.go b/pkg/auth/handler/update_metadata.go
index 35d2ae42..ae7944a5 100644
--- a/pkg/auth/handler/update_metadata.go
+++ b/pkg/auth/handler/update_metadata.go
@@ -107,13 +107,7 @@ type UpdateMetadataHandler struct {
 }
 
 func (h UpdateMetadataHandler) ProvideAuthzPolicy() authz.Policy {
-	return policy.AnyOf(
-		authz.PolicyFunc(policy.RequireMasterKey),
-		policy.AllOf(
-			authz.PolicyFunc(policy.DenyNoAccessKey),
-			policy.RequireValidUser,
-		),
-	)
+	return policy.RequireValidUserOrMasterKey
 }
 
 func (h UpdateMetadataHandler) ServeHTTP(resp http.ResponseWriter, req *http.Request) {
diff --git a/pkg/core/auth/authz/policy/compound.go b/pkg/core/auth/authz/policy/compound.go
index 323e00fc..24a75c9a 100644
--- a/pkg/core/auth/authz/policy/compound.go
+++ b/pkg/core/auth/authz/policy/compound.go
@@ -34,19 +34,16 @@ func AnyOf(policies ...authz.Policy) authz.Policy {
 }
 
 func (p Any) IsAllowed(r *http.Request, ctx auth.ContextGetter) error {
-	errs := []error{}
-
+	// return the last authz error
+	var err error
 	for _, policy := range p.policies {
-		var err error
-		if err = policy.IsAllowed(r, ctx); err == nil {
+		err = policy.IsAllowed(r, ctx)
+		if err == nil {
 			return nil
 		}
-
-		errs = append(errs, err)
 	}
 
-	// return the first error
-	return errs[0]
+	return err
 }
 
 // this ensures that our structure conform to certain interfaces.
diff --git a/pkg/core/auth/authz/policy/compound_policy.go b/pkg/core/auth/authz/policy/compound_policy.go
index 472550c0..57d4cba5 100644
--- a/pkg/core/auth/authz/policy/compound_policy.go
+++ b/pkg/core/auth/authz/policy/compound_policy.go
@@ -14,3 +14,11 @@ var RequireVerifiedUser = AllOf(
 	authz.PolicyFunc(DenyDisabledUser),
 	authz.PolicyFunc(denyNotVerifiedUser),
 )
+
+var RequireValidUserOrMasterKey = AnyOf(
+	authz.PolicyFunc(RequireMasterKey),
+	AllOf(
+		authz.PolicyFunc(DenyNoAccessKey),
+		RequireValidUser,
+	),
+)


Test patch:
diff --git a/pkg/core/auth/authz/policy/compound_policy_test.go b/pkg/core/auth/authz/policy/compound_policy_test.go
new file mode 100644
index 00000000..572cf42e
--- /dev/null
+++ b/pkg/core/auth/authz/policy/compound_policy_test.go
@@ -0,0 +1,53 @@
+package policy
+
+import (
+	"net/http"
+	"testing"
+	"time"
+
+	"github.com/skygeario/skygear-server/pkg/core/auth"
+	"github.com/skygeario/skygear-server/pkg/core/auth/authinfo"
+	"github.com/skygeario/skygear-server/pkg/core/model"
+
+	. "github.com/smartystreets/goconvey/convey"
+)
+
+func TestCompoundPolicy(t *testing.T) {
+	Convey("Test RequireValidUserOrMasterKey", t, func() {
+		Convey("should pass if valid user exist", func() {
+			req, _ := http.NewRequest("POST", "/", nil)
+			ctx := MemoryContextGetter{
+				mAuthInfo: &authinfo.AuthInfo{
+					ID: "ID",
+				},
+				mSession: &auth.Session{
+					AccessTokenCreatedAt: time.Date(2016, 10, 1, 0, 0, 0, 0, time.UTC),
+				},
+			}
+
+			err := RequireValidUserOrMasterKey.IsAllowed(req, ctx)
+			So(err, ShouldBeNil)
+		})
+
+		Convey("should pass if master key is used", func() {
+			req, _ := http.NewRequest("POST", "/", nil)
+			ctx := MemoryContextGetter{
+				mAccessKey: model.AccessKey{
+					Type:     model.MasterAccessKeyType,
+					ClientID: "",
+				},
+			}
+
+			err := RequireValidUserOrMasterKey.IsAllowed(req, ctx)
+			So(err, ShouldBeNil)
+		})
+
+		Convey("should fail if no user", func() {
+			req, _ := http.NewRequest("POST", "/", nil)
+			ctx := MemoryContextGetter{}
+
+			err := RequireValidUserOrMasterKey.IsAllowed(req, ctx)
+			So(err, ShouldBeError, "authentication required")
+		})
+	})
+}