Repository: SkygearIO/skygear-server
Pull request #1085 (2019-10-31 07:15:10)

Add policy to check user verified
connect #1079 

carmenlau: - Merged `RequireValidUserFunc ` function to merge `RequireAuthenticated` and `DenyDisabled`
- Updated `DenyDisabledUser` to check only if authInfo is nil, handler should use `RequireVerifiedUserFunc` to check user verified

Patch:
diff --git a/pkg/asset/handler/presign_upload.go b/pkg/asset/handler/presign_upload.go
index 5fd9e4a6..61bb204f 100644
--- a/pkg/asset/handler/presign_upload.go
+++ b/pkg/asset/handler/presign_upload.go
@@ -87,8 +87,7 @@ func (h *PresignUploadHandler) ProvideAuthzPolicy() authz.Policy {
 		authz.PolicyFunc(policy.RequireMasterKey),
 		policy.AllOf(
 			authz.PolicyFunc(policy.DenyNoAccessKey),
-			authz.PolicyFunc(policy.RequireAuthenticated),
-			authz.PolicyFunc(policy.DenyDisabledUser),
+			policy.RequireValidUser,
 		),
 	)
 }
diff --git a/pkg/asset/handler/presign_upload_form.go b/pkg/asset/handler/presign_upload_form.go
index 8c9706d7..72def4f7 100644
--- a/pkg/asset/handler/presign_upload_form.go
+++ b/pkg/asset/handler/presign_upload_form.go
@@ -83,8 +83,7 @@ func (h *PresignUploadFormHandler) ProvideAuthzPolicy() authz.Policy {
 		authz.PolicyFunc(policy.RequireMasterKey),
 		policy.AllOf(
 			authz.PolicyFunc(policy.DenyNoAccessKey),
-			authz.PolicyFunc(policy.RequireAuthenticated),
-			authz.PolicyFunc(policy.DenyDisabledUser),
+			policy.RequireValidUser,
 		),
 	)
 }
diff --git a/pkg/auth/handler/change_password.go b/pkg/auth/handler/change_password.go
index 28a1db9e..144bce7c 100644
--- a/pkg/auth/handler/change_password.go
+++ b/pkg/auth/handler/change_password.go
@@ -117,8 +117,7 @@ type ChangePasswordHandler struct {
 func (h ChangePasswordHandler) ProvideAuthzPolicy() authz.Policy {
 	return policy.AllOf(
 		authz.PolicyFunc(policy.DenyNoAccessKey),
-		authz.PolicyFunc(policy.RequireAuthenticated),
-		authz.PolicyFunc(policy.DenyDisabledUser),
+		policy.RequireValidUser,
 	)
 }
 
diff --git a/pkg/auth/handler/disable.go b/pkg/auth/handler/disable.go
index 0f5ec04c..fcec06ac 100644
--- a/pkg/auth/handler/disable.go
+++ b/pkg/auth/handler/disable.go
@@ -10,7 +10,6 @@ import (
 	"github.com/skygeario/skygear-server/pkg/auth/event"
 	authModel "github.com/skygeario/skygear-server/pkg/auth/model"
 	"github.com/skygeario/skygear-server/pkg/core/audit"
-	coreAuth "github.com/skygeario/skygear-server/pkg/core/auth"
 	"github.com/skygeario/skygear-server/pkg/core/auth/authinfo"
 	"github.com/skygeario/skygear-server/pkg/core/auth/authz"
 	"github.com/skygeario/skygear-server/pkg/core/auth/authz/policy"
@@ -110,22 +109,19 @@ func (payload setDisableUserPayload) Validate() error {
 		@Callback user_sync {UserSyncEvent}
 */
 type SetDisableHandler struct {
-	AuthContext      coreAuth.ContextGetter `dependency:"AuthContextGetter"`
-	RequireAuthz     handler.RequireAuthz   `dependency:"RequireAuthz"`
-	AuthInfoStore    authinfo.Store         `dependency:"AuthInfoStore"`
-	UserProfileStore userprofile.Store      `dependency:"UserProfileStore"`
-	AuditTrail       audit.Trail            `dependency:"AuditTrail"`
-	HookProvider     hook.Provider          `dependency:"HookProvider"`
-	TxContext        db.TxContext           `dependency:"TxContext"`
+	RequireAuthz     handler.RequireAuthz `dependency:"RequireAuthz"`
+	AuthInfoStore    authinfo.Store       `dependency:"AuthInfoStore"`
+	UserProfileStore userprofile.Store    `dependency:"UserProfileStore"`
+	AuditTrail       audit.Trail          `dependency:"AuditTrail"`
+	HookProvider     hook.Provider        `dependency:"HookProvider"`
+	TxContext        db.TxContext         `dependency:"TxContext"`
 }
 
 // ProvideAuthzPolicy provides authorization policy of handler
 func (h SetDisableHandler) ProvideAuthzPolicy() authz.Policy {
 	return policy.AllOf(
 		authz.PolicyFunc(policy.DenyNoAccessKey),
-		authz.PolicyFunc(policy.RequireAuthenticated),
 		authz.PolicyFunc(policy.RequireMasterKey),
-		authz.PolicyFunc(policy.DenyDisabledUser),
 	)
 }
 
diff --git a/pkg/auth/handler/logout.go b/pkg/auth/handler/logout.go
index d4e03831..016d7a67 100644
--- a/pkg/auth/handler/logout.go
+++ b/pkg/auth/handler/logout.go
@@ -74,8 +74,7 @@ type LogoutHandler struct {
 func (h LogoutHandler) ProvideAuthzPolicy() authz.Policy {
 	return policy.AllOf(
 		authz.PolicyFunc(policy.DenyNoAccessKey),
-		authz.PolicyFunc(policy.RequireAuthenticated),
-		authz.PolicyFunc(policy.DenyDisabledUser),
+		policy.RequireValidUser,
 	)
 }
 
diff --git a/pkg/auth/handler/me.go b/pkg/auth/handler/me.go
index c9ba7569..2d1af420 100644
--- a/pkg/auth/handler/me.go
+++ b/pkg/auth/handler/me.go
@@ -64,8 +64,7 @@ type MeHandler struct {
 func (h MeHandler) ProvideAuthzPolicy() authz.Policy {
 	return policy.AllOf(
 		authz.PolicyFunc(policy.DenyNoAccessKey),
-		authz.PolicyFunc(policy.RequireAuthenticated),
-		authz.PolicyFunc(policy.DenyDisabledUser),
+		policy.RequireValidUser,
 	)
 }
 
diff --git a/pkg/auth/handler/mfa/delete_authenticator.go b/pkg/auth/handler/mfa/delete_authenticator.go
index 34fb9889..1a0ad3bf 100644
--- a/pkg/auth/handler/mfa/delete_authenticator.go
+++ b/pkg/auth/handler/mfa/delete_authenticator.go
@@ -80,8 +80,7 @@ type DeleteAuthenticatorHandler struct {
 func (h *DeleteAuthenticatorHandler) ProvideAuthzPolicy() authz.Policy {
 	return policy.AllOf(
 		authz.PolicyFunc(policy.DenyNoAccessKey),
-		authz.PolicyFunc(policy.RequireAuthenticated),
-		authz.PolicyFunc(policy.DenyDisabledUser),
+		policy.RequireValidUser,
 	)
 }
 
diff --git a/pkg/auth/handler/mfa/list_recovery_code.go b/pkg/auth/handler/mfa/list_recovery_code.go
index d164df72..38b8bf45 100644
--- a/pkg/auth/handler/mfa/list_recovery_code.go
+++ b/pkg/auth/handler/mfa/list_recovery_code.go
@@ -82,8 +82,7 @@ type ListRecoveryCodeHandler struct {
 func (h *ListRecoveryCodeHandler) ProvideAuthzPolicy() authz.Policy {
 	return policy.AllOf(
 		authz.PolicyFunc(policy.DenyNoAccessKey),
-		authz.PolicyFunc(policy.RequireAuthenticated),
-		authz.PolicyFunc(policy.DenyDisabledUser),
+		policy.RequireValidUser,
 	)
 }
 
diff --git a/pkg/auth/handler/mfa/regenerate_recovery_code.go b/pkg/auth/handler/mfa/regenerate_recovery_code.go
index cf0e33e7..fb024bc9 100644
--- a/pkg/auth/handler/mfa/regenerate_recovery_code.go
+++ b/pkg/auth/handler/mfa/regenerate_recovery_code.go
@@ -81,8 +81,7 @@ type RegenerateRecoveryCodeHandler struct {
 func (h *RegenerateRecoveryCodeHandler) ProvideAuthzPolicy() authz.Policy {
 	return policy.AllOf(
 		authz.PolicyFunc(policy.DenyNoAccessKey),
-		authz.PolicyFunc(policy.RequireAuthenticated),
-		authz.PolicyFunc(policy.DenyDisabledUser),
+		policy.RequireValidUser,
 	)
 }
 
diff --git a/pkg/auth/handler/mfa/revoke_all_bearer_token.go b/pkg/auth/handler/mfa/revoke_all_bearer_token.go
index 6d695326..2dd70130 100644
--- a/pkg/auth/handler/mfa/revoke_all_bearer_token.go
+++ b/pkg/auth/handler/mfa/revoke_all_bearer_token.go
@@ -56,8 +56,7 @@ type RevokeAllBearerTokenHandler struct {
 func (h *RevokeAllBearerTokenHandler) ProvideAuthzPolicy() authz.Policy {
 	return policy.AllOf(
 		authz.PolicyFunc(policy.DenyNoAccessKey),
-		authz.PolicyFunc(policy.RequireAuthenticated),
-		authz.PolicyFunc(policy.DenyDisabledUser),
+		policy.RequireValidUser,
 	)
 }
 
diff --git a/pkg/auth/handler/reset_password.go b/pkg/auth/handler/reset_password.go
index 2cde7233..a9e80a16 100644
--- a/pkg/auth/handler/reset_password.go
+++ b/pkg/auth/handler/reset_password.go
@@ -108,9 +108,7 @@ type ResetPasswordHandler struct {
 func (h ResetPasswordHandler) ProvideAuthzPolicy() authz.Policy {
 	return policy.AllOf(
 		authz.PolicyFunc(policy.DenyNoAccessKey),
-		authz.PolicyFunc(policy.RequireAuthenticated),
 		authz.PolicyFunc(policy.RequireMasterKey),
-		authz.PolicyFunc(policy.DenyDisabledUser),
 	)
 }
 
diff --git a/pkg/auth/handler/session/get.go b/pkg/auth/handler/session/get.go
index d4e1dd5b..d838f35d 100644
--- a/pkg/auth/handler/session/get.go
+++ b/pkg/auth/handler/session/get.go
@@ -102,8 +102,7 @@ type GetHandler struct {
 func (h GetHandler) ProvideAuthzPolicy() authz.Policy {
 	return policy.AllOf(
 		authz.PolicyFunc(policy.DenyNoAccessKey),
-		authz.PolicyFunc(policy.RequireAuthenticated),
-		authz.PolicyFunc(policy.DenyDisabledUser),
+		policy.RequireValidUser,
 	)
 }
 
diff --git a/pkg/auth/handler/session/list.go b/pkg/auth/handler/session/list.go
index e20b9281..d437e458 100644
--- a/pkg/auth/handler/session/list.go
+++ b/pkg/auth/handler/session/list.go
@@ -81,8 +81,7 @@ type ListHandler struct {
 func (h ListHandler) ProvideAuthzPolicy() authz.Policy {
 	return policy.AllOf(
 		authz.PolicyFunc(policy.DenyNoAccessKey),
-		authz.PolicyFunc(policy.RequireAuthenticated),
-		authz.PolicyFunc(policy.DenyDisabledUser),
+		policy.RequireValidUser,
 	)
 }
 
diff --git a/pkg/auth/handler/session/revoke.go b/pkg/auth/handler/session/revoke.go
index dc8aa2d0..a57d7226 100644
--- a/pkg/auth/handler/session/revoke.go
+++ b/pkg/auth/handler/session/revoke.go
@@ -88,8 +88,7 @@ type RevokeHandler struct {
 func (h RevokeHandler) ProvideAuthzPolicy() authz.Policy {
 	return policy.AllOf(
 		authz.PolicyFunc(policy.DenyNoAccessKey),
-		authz.PolicyFunc(policy.RequireAuthenticated),
-		authz.PolicyFunc(policy.DenyDisabledUser),
+		policy.RequireValidUser,
 	)
 }
 
diff --git a/pkg/auth/handler/session/revoke_all.go b/pkg/auth/handler/session/revoke_all.go
index a9c016ce..3a44ee83 100644
--- a/pkg/auth/handler/session/revoke_all.go
+++ b/pkg/auth/handler/session/revoke_all.go
@@ -63,8 +63,7 @@ type RevokeAllHandler struct {
 func (h RevokeAllHandler) ProvideAuthzPolicy() authz.Policy {
 	return policy.AllOf(
 		authz.PolicyFunc(policy.DenyNoAccessKey),
-		authz.PolicyFunc(policy.RequireAuthenticated),
-		authz.PolicyFunc(policy.DenyDisabledUser),
+		policy.RequireValidUser,
 	)
 }
 
diff --git a/pkg/auth/handler/sso/link.go b/pkg/auth/handler/sso/link.go
index 4d4a0b83..dd18c9f8 100644
--- a/pkg/auth/handler/sso/link.go
+++ b/pkg/auth/handler/sso/link.go
@@ -107,8 +107,7 @@ type LinkHandler struct {
 func (h LinkHandler) ProvideAuthzPolicy() authz.Policy {
 	return policy.AllOf(
 		authz.PolicyFunc(policy.DenyNoAccessKey),
-		authz.PolicyFunc(policy.RequireAuthenticated),
-		authz.PolicyFunc(policy.DenyDisabledUser),
+		policy.RequireValidUser,
 	)
 }
 
diff --git a/pkg/auth/handler/sso/unlink.go b/pkg/auth/handler/sso/unlink.go
index 918d9885..475ced82 100644
--- a/pkg/auth/handler/sso/unlink.go
+++ b/pkg/auth/handler/sso/unlink.go
@@ -76,8 +76,7 @@ type UnlinkHandler struct {
 func (h UnlinkHandler) ProvideAuthzPolicy() authz.Policy {
 	return policy.AllOf(
 		authz.PolicyFunc(policy.DenyNoAccessKey),
-		authz.PolicyFunc(policy.RequireAuthenticated),
-		authz.PolicyFunc(policy.DenyDisabledUser),
+		policy.RequireValidUser,
 	)
 }
 
diff --git a/pkg/auth/handler/update_metadata.go b/pkg/auth/handler/update_metadata.go
index 8b0ddd7f..936fda85 100644
--- a/pkg/auth/handler/update_metadata.go
+++ b/pkg/auth/handler/update_metadata.go
@@ -99,8 +99,7 @@ func (h UpdateMetadataHandler) ProvideAuthzPolicy() authz.Policy {
 		authz.PolicyFunc(policy.RequireMasterKey),
 		policy.AllOf(
 			authz.PolicyFunc(policy.DenyNoAccessKey),
-			authz.PolicyFunc(policy.RequireAuthenticated),
-			authz.PolicyFunc(policy.DenyDisabledUser),
+			policy.RequireValidUser,
 		),
 	)
 }
diff --git a/pkg/auth/handler/userverify/verify_code.go b/pkg/auth/handler/userverify/verify_code.go
index 7902592a..3d51035e 100644
--- a/pkg/auth/handler/userverify/verify_code.go
+++ b/pkg/auth/handler/userverify/verify_code.go
@@ -104,8 +104,7 @@ type VerifyCodeHandler struct {
 func (h VerifyCodeHandler) ProvideAuthzPolicy() authz.Policy {
 	return policy.AllOf(
 		authz.PolicyFunc(policy.DenyNoAccessKey),
-		authz.PolicyFunc(policy.RequireAuthenticated),
-		authz.PolicyFunc(policy.DenyDisabledUser),
+		policy.RequireValidUser,
 	)
 }
 
diff --git a/pkg/auth/handler/userverify/verify_request.go b/pkg/auth/handler/userverify/verify_request.go
index 6666136b..3e2c54ab 100644
--- a/pkg/auth/handler/userverify/verify_request.go
+++ b/pkg/auth/handler/userverify/verify_request.go
@@ -134,8 +134,7 @@ type VerifyRequestHandler struct {
 func (h VerifyRequestHandler) ProvideAuthzPolicy() authz.Policy {
 	return policy.AllOf(
 		authz.PolicyFunc(policy.DenyNoAccessKey),
-		authz.PolicyFunc(policy.RequireAuthenticated),
-		authz.PolicyFunc(policy.DenyDisabledUser),
+		policy.RequireValidUser,
 	)
 }
 
diff --git a/pkg/core/auth/authz/error.go b/pkg/core/auth/authz/error.go
index 76ebf468..d0ea837b 100644
--- a/pkg/core/auth/authz/error.go
+++ b/pkg/core/auth/authz/error.go
@@ -8,6 +8,7 @@ var (
 	NotAuthenticated     = skyerr.Unauthorized.WithReason("NotAuthenticated")
 	AccessKeyNotAccepted = skyerr.Unauthorized.WithReason("AccessKeyNotAccepted")
 	UserDisabled         = skyerr.Forbidden.WithReason("UserDisabled")
+	UserNotVerified      = skyerr.Forbidden.WithReason("UserNotVerified")
 )
 
 var ErrNotAuthenticated = NotAuthenticated.New("authentication required")
diff --git a/pkg/core/auth/authz/policy/authinfo.go b/pkg/core/auth/authz/policy/authinfo.go
index 33aedd6a..d3fd6d61 100644
--- a/pkg/core/auth/authz/policy/authinfo.go
+++ b/pkg/core/auth/authz/policy/authinfo.go
@@ -7,7 +7,7 @@ import (
 	"github.com/skygeario/skygear-server/pkg/core/auth/authz"
 )
 
-func RequireAuthenticated(r *http.Request, ctx auth.ContextGetter) error {
+func requireAuthenticated(r *http.Request, ctx auth.ContextGetter) error {
 	authInfo, _ := ctx.AuthInfo()
 	if authInfo == nil {
 		return authz.ErrNotAuthenticated
@@ -18,5 +18,5 @@ func RequireAuthenticated(r *http.Request, ctx auth.ContextGetter) error {
 
 // this ensures that our structure conform to certain interfaces.
 var (
-	_ authz.PolicyFunc = RequireAuthenticated
+	_ authz.PolicyFunc = requireAuthenticated
 )
diff --git a/pkg/core/auth/authz/policy/compound_policy.go b/pkg/core/auth/authz/policy/compound_policy.go
new file mode 100644
index 00000000..472550c0
--- /dev/null
+++ b/pkg/core/auth/authz/policy/compound_policy.go
@@ -0,0 +1,16 @@
+package policy
+
+import (
+	"github.com/skygeario/skygear-server/pkg/core/auth/authz"
+)
+
+var RequireValidUser = AllOf(
+	authz.PolicyFunc(requireAuthenticated),
+	authz.PolicyFunc(DenyDisabledUser),
+)
+
+var RequireVerifiedUser = AllOf(
+	authz.PolicyFunc(requireAuthenticated),
+	authz.PolicyFunc(DenyDisabledUser),
+	authz.PolicyFunc(denyNotVerifiedUser),
+)
diff --git a/pkg/core/auth/authz/policy/disable.go b/pkg/core/auth/authz/policy/disable.go
index ea224e59..ad686dba 100644
--- a/pkg/core/auth/authz/policy/disable.go
+++ b/pkg/core/auth/authz/policy/disable.go
@@ -11,7 +11,7 @@ import (
 
 // DenyDisabledUser denies disabled user.
 // It is not an error if the request does not have an associated user.
-// If you want to enforce enabled user, use both RequireAuthenticated and DenyDisabledUser.
+// If you want to enforce enabled user, use RequireValidUser.
 func DenyDisabledUser(r *http.Request, ctx auth.ContextGetter) error {
 	authInfo, _ := ctx.AuthInfo()
 	if authInfo != nil && authInfo.IsDisabled() {
diff --git a/pkg/core/auth/authz/policy/verified.go b/pkg/core/auth/authz/policy/verified.go
new file mode 100644
index 00000000..ef703ad9
--- /dev/null
+++ b/pkg/core/auth/authz/policy/verified.go
@@ -0,0 +1,23 @@
+package policy
+
+import (
+	"net/http"
+
+	"github.com/skygeario/skygear-server/pkg/core/auth"
+	"github.com/skygeario/skygear-server/pkg/core/auth/authz"
+)
+
+// denyNotVerifiedUser denies not verified user.
+func denyNotVerifiedUser(r *http.Request, ctx auth.ContextGetter) error {
+	authInfo, _ := ctx.AuthInfo()
+
+	if authInfo != nil && !authInfo.Verified {
+		return authz.UserNotVerified.New("user is not verified")
+	}
+
+	return nil
+}
+
+var (
+	_ authz.PolicyFunc = denyNotVerifiedUser
+)


Test patch:
diff --git a/pkg/core/auth/authz/policy/authinfo_test.go b/pkg/core/auth/authz/policy/authinfo_test.go
index ac509d59..06d6bb88 100644
--- a/pkg/core/auth/authz/policy/authinfo_test.go
+++ b/pkg/core/auth/authz/policy/authinfo_test.go
@@ -12,12 +12,12 @@ import (
 )
 
 func TestRequireAuthenticated(t *testing.T) {
-	Convey("Test RequireAuthenticated", t, func() {
+	Convey("Test requireAuthenticated", t, func() {
 		Convey("should return error if auth context has no auth info", func() {
 			req, _ := http.NewRequest("POST", "/", nil)
 			ctx := MemoryContextGetter{}
 
-			err := RequireAuthenticated(req, ctx)
+			err := requireAuthenticated(req, ctx)
 			So(err, ShouldNotBeEmpty)
 		})
 
@@ -32,7 +32,7 @@ func TestRequireAuthenticated(t *testing.T) {
 				},
 			}
 
-			err := RequireAuthenticated(req, ctx)
+			err := requireAuthenticated(req, ctx)
 			So(err, ShouldBeEmpty)
 		})
 	})
diff --git a/pkg/core/auth/authz/policy/verified_test.go b/pkg/core/auth/authz/policy/verified_test.go
new file mode 100644
index 00000000..0e284326
--- /dev/null
+++ b/pkg/core/auth/authz/policy/verified_test.go
@@ -0,0 +1,49 @@
+package policy
+
+import (
+	"net/http"
+	"testing"
+
+	"github.com/skygeario/skygear-server/pkg/core/auth/authinfo"
+
+	. "github.com/smartystreets/goconvey/convey"
+)
+
+func TestDenyNotVerifiedUser(t *testing.T) {
+	Convey("Test denyNotVerifiedUser", t, func() {
+		Convey("should not return error if auth context has no auth info", func() {
+			req, _ := http.NewRequest("POST", "/", nil)
+			ctx := MemoryContextGetter{}
+
+			err := denyNotVerifiedUser(req, ctx)
+			So(err, ShouldBeNil)
+		})
+
+		Convey("should return error if user is not verified", func() {
+			req, _ := http.NewRequest("POST", "/", nil)
+			ctx := MemoryContextGetter{
+				mAuthInfo: &authinfo.AuthInfo{
+					ID:       "ID",
+					Disabled: true,
+				},
+			}
+
+			err := denyNotVerifiedUser(req, ctx)
+			So(err, ShouldNotBeNil)
+		})
+
+		Convey("should pass if user is verified", func() {
+			req, _ := http.NewRequest("POST", "/", nil)
+			ctx := MemoryContextGetter{
+				mAuthInfo: &authinfo.AuthInfo{
+					ID:       "ID",
+					Verified: true,
+				},
+			}
+
+			err := denyNotVerifiedUser(req, ctx)
+			So(err, ShouldBeNil)
+		})
+
+	})
+}