bucket: refactor policies and fix bugs related to enforcing policies. (#2766)

This patch also addresses the problem of double caching at
object layer once at XL and another at handler layer.

cmd/bucket-handlers.go

@@ -31,14 +31,26 @@ import (
 // http://docs.aws.amazon.com/AmazonS3/latest/dev/using-with-s3-actions.html
 // Enforces bucket policies for a bucket for a given tatusaction.
 func enforceBucketPolicy(bucket string, action string, reqURL *url.URL) (s3Error APIErrorCode) {
-	if !IsValidBucketName(bucket) {
-		return ErrInvalidBucketName
-	}
 	// Fetch bucket policy, if policy is not set return access denied.
-	policy := globalBucketPolicies.GetBucketPolicy(bucket)
+	policy, err := readBucketPolicy(bucket, newObjectLayerFn())
-	if policy == nil {
+	if err != nil {
+		err = errorCause(err)
+		switch err.(type) {
+		case BucketNameInvalid:
+			// Return error for invalid bucket name.
+			return ErrInvalidBucketName
+		case BucketNotFound:
+			// For no bucket found we return NoSuchBucket instead.
+			return ErrNoSuchBucket
+		case BucketPolicyNotFound:
+			// For no bucket policy found, return AccessDenied, since
+			// anonymous requests are not allowed without bucket policies.
 			return ErrAccessDenied
 		}
+		errorIf(err, "Unable to read bucket policy.")
+		// Return internal error for any other errors so that we can investigate.
+		return ErrInternalError
+	}
 
 	// Construct resource in 'arn:aws:s3:::examplebucket/object' format.
 	resource := AWSResourcePrefix + strings.TrimSuffix(strings.TrimPrefix(reqURL.Path, "/"), "/")

cmd/bucket-handlers_test.go

@@ -30,8 +30,6 @@ func TestGetBucketLocationHandler(t *testing.T) {
 }
 
 func testGetBucketLocationHandler(obj ObjectLayer, instanceType string, t TestErrHandler) {
-	initBucketPolicies(obj)
-
 	// get random bucket name.
 	bucketName := getRandomBucketName()
 	// Create bucket.
@@ -130,8 +128,6 @@ func TestHeadBucketHandler(t *testing.T) {
 }
 
 func testHeadBucketHandler(obj ObjectLayer, instanceType string, t TestErrHandler) {
-	initBucketPolicies(obj)
-
 	// get random bucket name.
 	bucketName := getRandomBucketName()
 	// Create bucket.
@@ -207,8 +203,6 @@ func TestListMultipartUploadsHandler(t *testing.T) {
 
 // testListMultipartUploadsHandler - Tests validate listing of multipart uploads.
 func testListMultipartUploadsHandler(obj ObjectLayer, instanceType string, t TestErrHandler) {
-	initBucketPolicies(obj)
-
 	// get random bucket name.
 	bucketName := getRandomBucketName()
 

cmd/bucket-policy-handlers.go

@@ -196,9 +196,6 @@ func (api objectAPIHandlers) PutBucketPolicyHandler(w http.ResponseWriter, r *ht
 		return
 	}
 
-	// Set the bucket policy in memory.
-	globalBucketPolicies.SetBucketPolicy(bucket, policy)
-
 	// Success.
 	writeSuccessNoContent(w)
 }
@@ -242,9 +239,6 @@ func (api objectAPIHandlers) DeleteBucketPolicyHandler(w http.ResponseWriter, r
 		return
 	}
 
-	// Remove bucket policy.
-	globalBucketPolicies.RemoveBucketPolicy(bucket)
-
 	// Success.
 	writeSuccessNoContent(w)
 }

cmd/bucket-policy-handlers_test.go

@@ -247,8 +247,6 @@ func TestPutBucketPolicyHandler(t *testing.T) {
 // testPutBucketPolicyHandler - Test for Bucket policy end point.
 // TODO: Add exhaustive cases with various combination of statement fields.
 func testPutBucketPolicyHandler(obj ObjectLayer, instanceType string, t TestErrHandler) {
-	initBucketPolicies(obj)
-
 	// get random bucket name.
 	bucketName := getRandomBucketName()
 	// Create bucket.
@@ -313,8 +311,6 @@ func TestGetBucketPolicyHandler(t *testing.T) {
 // testGetBucketPolicyHandler - Test for end point which fetches the access policy json of the given bucket.
 // TODO: Add exhaustive cases with various combination of statement fields.
 func testGetBucketPolicyHandler(obj ObjectLayer, instanceType string, t TestErrHandler) {
-	initBucketPolicies(obj)
-
 	// get random bucket name.
 	bucketName := getRandomBucketName()
 	// Create bucket.
@@ -423,8 +419,6 @@ func TestDeleteBucketPolicyHandler(t *testing.T) {
 // testDeleteBucketPolicyHandler - Test for Delete bucket policy end point.
 // TODO: Add exhaustive cases with various combination of statement fields.
 func testDeleteBucketPolicyHandler(obj ObjectLayer, instanceType string, t TestErrHandler) {
-	initBucketPolicies(obj)
-
 	// get random bucket name.
 	bucketName := getRandomBucketName()
 	// Create bucket.

cmd/bucket-policy.go

@@ -18,110 +18,10 @@ package cmd
 
 import (
 	"bytes"
-	"errors"
 	"io"
 	"path"
-	"sync"
 )
 
-// Variable represents bucket policies in memory.
-var globalBucketPolicies *bucketPolicies
-
-// Global bucket policies list, policies are enforced on each bucket looking
-// through the policies here.
-type bucketPolicies struct {
-	rwMutex *sync.RWMutex
-
-	// Collection of 'bucket' policies.
-	bucketPolicyConfigs map[string]*bucketPolicy
-}
-
-// Fetch bucket policy for a given bucket.
-func (bp bucketPolicies) GetBucketPolicy(bucket string) *bucketPolicy {
-	bp.rwMutex.RLock()
-	defer bp.rwMutex.RUnlock()
-	return bp.bucketPolicyConfigs[bucket]
-}
-
-// Set a new bucket policy for a bucket, this operation will overwrite
-// any previous bucketpolicies for the bucket.
-func (bp *bucketPolicies) SetBucketPolicy(bucket string, policy *bucketPolicy) error {
-	bp.rwMutex.Lock()
-	defer bp.rwMutex.Unlock()
-	if policy == nil {
-		return errors.New("invalid argument")
-	}
-	bp.bucketPolicyConfigs[bucket] = policy
-	return nil
-}
-
-// Remove bucket policy for a bucket, from in-memory map.
-func (bp *bucketPolicies) RemoveBucketPolicy(bucket string) {
-	bp.rwMutex.Lock()
-	defer bp.rwMutex.Unlock()
-	delete(bp.bucketPolicyConfigs, bucket)
-}
-
-// Loads all bucket policies from persistent layer.
-func loadAllBucketPolicies(objAPI ObjectLayer) (policies map[string]*bucketPolicy, err error) {
-	// List buckets to proceed loading all notification configuration.
-	buckets, err := objAPI.ListBuckets()
-	errorIf(err, "Unable to list buckets.")
-	err = errorCause(err)
-	if err != nil {
-		return nil, err
-	}
-
-	policies = make(map[string]*bucketPolicy)
-	var pErrs []error
-	// Loads bucket policy.
-	for _, bucket := range buckets {
-		policy, pErr := readBucketPolicy(bucket.Name, objAPI)
-		if pErr != nil {
-			switch pErr.(type) {
-			case BucketPolicyNotFound:
-				continue
-			}
-			pErrs = append(pErrs, pErr)
-			// Continue to load other bucket policies if possible.
-			continue
-		}
-		policies[bucket.Name] = policy
-	}
-
-	// Look for any errors occurred while reading bucket policies.
-	for _, pErr := range pErrs {
-		if pErr != nil {
-			return policies, pErr
-		}
-	}
-
-	// Success.
-	return policies, nil
-}
-
-// Intialize all bucket policies.
-func initBucketPolicies(objAPI ObjectLayer) error {
-	if objAPI == nil {
-		return errInvalidArgument
-	}
-
-	// Read all bucket policies.
-	policies, err := loadAllBucketPolicies(objAPI)
-	if err != nil {
-		return err
-	}
-
-	// Populate global bucket collection.
-	globalBucketPolicies = &bucketPolicies{
-		rwMutex:             &sync.RWMutex{},
-		bucketPolicyConfigs: policies,
-	}
-
-	// Success.
-	return nil
-}
-
 // getOldBucketsConfigPath - get old buckets config path. (Only used for migrating old bucket policies)
 func getOldBucketsConfigPath() (string, error) {
 	configPath, err := getConfigPath()
@@ -135,8 +35,8 @@ func getOldBucketsConfigPath() (string, error) {
 // if bucket policy is not found.
 func readBucketPolicyJSON(bucket string, objAPI ObjectLayer) (bucketPolicyReader io.Reader, err error) {
 	// Verify if bucket actually exists
-	if e := isBucketExist(bucket, objAPI); e != nil {
+	if err = isBucketExist(bucket, objAPI); err != nil {
-		return nil, e
+		return nil, err
 	}
 
 	policyPath := pathJoin(bucketConfigPrefix, bucket, policyJSON)

cmd/controller-handlers.go

@@ -16,6 +16,11 @@
 
 package cmd
 
+import "errors"
+
+// errServerNotInitialized - server not initialized.
+var errServerNotInitialized = errors.New("Server not initialized, please try again.")
+
 /// Auth operations
 
 // Login - login handler.
@@ -59,7 +64,7 @@ type HealListReply struct {
 func (c *controllerAPIHandlers) ListObjectsHealHandler(args *HealListArgs, reply *HealListReply) error {
 	objAPI := c.ObjectAPI()
 	if objAPI == nil {
-		return errVolumeBusy
+		return errServerNotInitialized
 	}
 	if !isRPCTokenValid(args.Token) {
 		return errInvalidToken
@@ -91,11 +96,11 @@ type HealObjectArgs struct {
 // HealObjectReply - reply by HealObject RPC.
 type HealObjectReply struct{}
 
-// HealObject - heal the object.
+// HealObject - heals an object, returns nil error upon success.
 func (c *controllerAPIHandlers) HealObjectHandler(args *HealObjectArgs, reply *GenericReply) error {
 	objAPI := c.ObjectAPI()
 	if objAPI == nil {
-		return errVolumeBusy
+		return errServerNotInitialized
 	}
 	if !isRPCTokenValid(args.Token) {
 		return errInvalidToken
@@ -103,11 +108,11 @@ func (c *controllerAPIHandlers) HealObjectHandler(args *HealObjectArgs, reply *G
 	return objAPI.HealObject(args.Bucket, args.Object)
 }
 
-// HealObject - heal the object.
+// HealDiskMetadataHandler - heals disks metadata, returns nil error upon success.
 func (c *controllerAPIHandlers) HealDiskMetadataHandler(args *GenericArgs, reply *GenericReply) error {
 	objAPI := c.ObjectAPI()
 	if objAPI == nil {
-		return errVolumeBusy
+		return errServerNotInitialized
 	}
 	if !isRPCTokenValid(args.Token) {
 		return errInvalidToken

cmd/routers.go

@@ -73,10 +73,6 @@ func newObjectLayer(disks, ignoredDisks []string) (ObjectLayer, error) {
 	err = initEventNotifier(objAPI)
 	fatalIf(err, "Unable to initialize event notification.")
 
-	// Initialize and load bucket policies.
-	err = initBucketPolicies(objAPI)
-	fatalIf(err, "Unable to load all bucket policies.")
-
 	// Success.
 	return objAPI, nil
 }

cmd/storage-errors.go

@@ -59,6 +59,3 @@ var errVolumeAccessDenied = errors.New("volume access denied")
 
 // errVolumeAccessDenied - cannot access file, insufficient permissions.
 var errFileAccessDenied = errors.New("file access denied")
-
-// errVolumeBusy - remote disk is not connected to yet.
-var errVolumeBusy = errors.New("volume is busy")

cmd/test-utils_test.go

@@ -1298,11 +1298,17 @@ func initTestAPIEndPoints(objLayer ObjectLayer, apiFunctions []string) http.Hand
 	// initialize a new mux router.
 	// goriilla/mux is the library used to register all the routes and handle them.
 	muxRouter := router.NewRouter()
+
 	// All object storage operations are registered as HTTP handlers on `objectAPIHandlers`.
 	// When the handlers get a HTTP request they use the underlyting ObjectLayer to perform operations.
+	objLayerMutex.Lock()
+	globalObjectAPI = objLayer
+	objLayerMutex.Unlock()
+
 	api := objectAPIHandlers{
-		ObjectAPI: func() ObjectLayer { return objLayer },
+		ObjectAPI: newObjectLayerFn,
 	}
+
 	// API Router.
 	apiRouter := muxRouter.NewRoute().PathPrefix("/").Subrouter()
 	// Bucket router.

cmd/web-handlers.go

@@ -127,7 +127,7 @@ func (web *webAPIHandlers) StorageInfo(r *http.Request, args *GenericArgs, reply
 	reply.UIVersion = miniobrowser.UIVersion
 	objectAPI := web.ObjectAPI()
 	if objectAPI == nil {
-		return &json2.Error{Message: "Server not initialized"}
+		return &json2.Error{Message: "Server not initialized, please try again., please try again."}
 	}
 	reply.StorageInfo = objectAPI.StorageInfo()
 	return nil
@@ -146,7 +146,7 @@ func (web *webAPIHandlers) MakeBucket(r *http.Request, args *MakeBucketArgs, rep
 	reply.UIVersion = miniobrowser.UIVersion
 	objectAPI := web.ObjectAPI()
 	if objectAPI == nil {
-		return &json2.Error{Message: "Server not initialized"}
+		return &json2.Error{Message: "Server not initialized, please try again."}
 	}
 	if err := objectAPI.MakeBucket(args.BucketName); err != nil {
 		return &json2.Error{Message: err.Error()}
@@ -175,7 +175,7 @@ func (web *webAPIHandlers) ListBuckets(r *http.Request, args *WebGenericArgs, re
 	}
 	objectAPI := web.ObjectAPI()
 	if objectAPI == nil {
-		return &json2.Error{Message: "Server not initialized"}
+		return &json2.Error{Message: "Server not initialized, please try again."}
 	}
 	buckets, err := objectAPI.ListBuckets()
 	if err != nil {
@@ -227,7 +227,7 @@ func (web *webAPIHandlers) ListObjects(r *http.Request, args *ListObjectsArgs, r
 	for {
 		objectAPI := web.ObjectAPI()
 		if objectAPI == nil {
-			return &json2.Error{Message: "Server not initialized"}
+			return &json2.Error{Message: "Server not initialized, please try again."}
 		}
 		lo, err := objectAPI.ListObjects(args.BucketName, args.Prefix, marker, "/", 1000)
 		if err != nil {
@@ -269,7 +269,7 @@ func (web *webAPIHandlers) RemoveObject(r *http.Request, args *RemoveObjectArgs,
 	reply.UIVersion = miniobrowser.UIVersion
 	objectAPI := web.ObjectAPI()
 	if objectAPI == nil {
-		return &json2.Error{Message: "Server not initialized"}
+		return &json2.Error{Message: "Server not initialized, please try again."}
 	}
 	if err := objectAPI.DeleteObject(args.BucketName, args.ObjectName); err != nil {
 		return &json2.Error{Message: err.Error()}
@@ -412,7 +412,7 @@ func (web *webAPIHandlers) Upload(w http.ResponseWriter, r *http.Request) {
 
 	objectAPI := web.ObjectAPI()
 	if objectAPI == nil {
-		writeWebErrorResponse(w, errors.New("Server not initialized"))
+		writeWebErrorResponse(w, errors.New("Server not initialized, please try again."))
 		return
 	}
 	if _, err := objectAPI.PutObject(bucket, object, -1, r.Body, metadata); err != nil {
@@ -468,7 +468,7 @@ func (web *webAPIHandlers) Download(w http.ResponseWriter, r *http.Request) {
 
 	objectAPI := web.ObjectAPI()
 	if objectAPI == nil {
-		writeWebErrorResponse(w, errors.New("Server not initialized"))
+		writeWebErrorResponse(w, errors.New("Server not initialized, please try again."))
 		return
 	}
 	objInfo, err := objectAPI.GetObjectInfo(bucket, object)
@@ -567,7 +567,7 @@ func (web *webAPIHandlers) GetBucketPolicy(r *http.Request, args *GetBucketPolic
 
 	objectAPI := web.ObjectAPI()
 	if objectAPI == nil {
-		return &json2.Error{Message: "Server not initialized"}
+		return &json2.Error{Message: "Server not initialized, please try again."}
 	}
 	policyInfo, err := readBucketAccessPolicy(objectAPI, args.BucketName)
 	if err != nil {
@@ -630,7 +630,7 @@ func (web *webAPIHandlers) SetBucketPolicy(r *http.Request, args *SetBucketPolic
 	}
 	objectAPI := web.ObjectAPI()
 	if objectAPI == nil {
-		return &json2.Error{Message: "Server not initialized"}
+		return &json2.Error{Message: "Server not initialized, please try again."}
 	}
 
 	bucketP := policy.BucketPolicy(args.Policy)