Remove unauthorized predicates from query

edgraph/access_ee.go

@@ -460,7 +460,10 @@ func extractUserAndGroups(ctx context.Context) ([]string, error) {
 	return validateToken(accessJwt[0])
 }
 
-func authorizePreds(userId string, groupIds, preds []string, aclOp *acl.Operation) error {
+func authorizePreds(userId string, groupIds, preds []string,
+	aclOp *acl.Operation) map[string]struct{} {
+
+	blockedPreds := make(map[string]struct{})
 	for _, pred := range preds {
 		if err := aclCachePtr.authorizePredicate(groupIds, pred, aclOp); err != nil {
 			logAccess(&accessEntry{
@@ -471,21 +474,10 @@ func authorizePreds(userId string, groupIds, preds []string, aclOp *acl.Operatio
 				allowed:   false,
 			})
 
-			var op string
-			switch aclOp.Name {
-			case acl.OpModify:
-				op = "alter"
-			case acl.OpWrite:
-				op = "mutate"
-			case acl.OpRead:
-				op = "query"
-			}
-
-			return status.Errorf(codes.PermissionDenied,
-				"unauthorized to %s the predicate: %v", op, err)
+			blockedPreds[pred] = struct{}{}
 		}
 	}
-	return nil
+	return blockedPreds
 }
 
 // authorizeAlter parses the Schema in the operation and authorizes the operation
@@ -521,30 +513,35 @@ func authorizeAlter(ctx context.Context, op *api.Operation) error {
 	// as a byproduct, it also sets the userId, groups variables
 	doAuthorizeAlter := func() error {
 		userData, err := extractUserAndGroups(ctx)
-		switch {
-		case err == errNoJwt:
-			// treat the user as an anonymous guest who has not joined any group yet
-			// such a user can still get access to predicates that have no ACL rule defined, per the
-			// fail open approach
-		case err != nil:
+		if err != nil {
+			// We don't follow fail open approach anymore.
 			return status.Error(codes.Unauthenticated, err.Error())
-		default:
-			userId = userData[0]
-			groupIds = userData[1:]
+		}
 
-			if x.IsGuardian(groupIds) {
-				// Members of guardian group are allowed to alter anything.
-				return nil
-			}
+		userId = userData[0]
+		groupIds = userData[1:]
+
+		if x.IsGuardian(groupIds) {
+			// Members of guardian group are allowed to alter anything.
+			return nil
 		}
 
-		// if we get here, we know the user is not Groot.
+		// if we get here, we know the user is not a guardian.
 		if isDropAll(op) || op.DropOp == api.Operation_DATA {
 			return errors.Errorf(
-				"only Groot is allowed to drop all data, but the current user is %s", userId)
+				"only guardians are allowed to drop all data, but the current user is %s", userId)
 		}
 
-		return authorizePreds(userId, groupIds, preds, acl.Modify)
+		blockedPreds := authorizePreds(userId, groupIds, preds, acl.Modify)
+		if len(blockedPreds) > 0 {
+			var msg strings.Builder
+			for key := range blockedPreds {
+				msg.WriteString(key + " ")
+			}
+			return status.Errorf(codes.PermissionDenied,
+				"unauthorized to alter following predicates: %s\n", msg.String())
+		}
+		return nil
 	}
 
 	err := doAuthorizeAlter()
@@ -621,33 +618,41 @@ func authorizeMutation(ctx context.Context, gmu *gql.Mutation) error {
 	// as a byproduct, it also sets the userId and groups
 	doAuthorizeMutation := func() error {
 		userData, err := extractUserAndGroups(ctx)
-		switch {
-		case err == errNoJwt:
-			// treat the user as an anonymous guest who has not joined any group yet
-			// such a user can still get access to predicates that have no ACL rule defined
-		case err != nil:
+		if err != nil {
+			// We don't follow fail open approach anymore.
 			return status.Error(codes.Unauthenticated, err.Error())
-		default:
-			userId = userData[0]
-			groupIds = userData[1:]
-
-			if x.IsGuardian(groupIds) {
-				// Members of guardians group are allowed to mutate anything
-				// (including delete) except the permission of the acl predicates.
-				switch {
-				case isAclPredMutation(gmu.Set):
-					return errors.Errorf("the permission of ACL predicates can not be changed")
-				case isAclPredMutation(gmu.Del):
-					return errors.Errorf("ACL predicates can't be deleted")
-				}
-				return nil
+		}
+
+		userId = userData[0]
+		groupIds = userData[1:]
+
+		if x.IsGuardian(groupIds) {
+			// Members of guardians group are allowed to mutate anything
+			// (including delete) except the permission of the acl predicates.
+			switch {
+			case isAclPredMutation(gmu.Set):
+				return errors.Errorf("the permission of ACL predicates can not be changed")
+			case isAclPredMutation(gmu.Del):
+				return errors.Errorf("ACL predicates can't be deleted")
+			}
+			return nil
+		}
+
+		blockedPreds := authorizePreds(userId, groupIds, preds, acl.Write)
+		if len(blockedPreds) > 0 {
+			var msg strings.Builder
+			for key := range blockedPreds {
+				msg.WriteString(key + " ")
 			}
+			return status.Errorf(codes.PermissionDenied,
+				"unauthorized to mutate following predicates: %s\n", msg.String())
 		}
 
-		return authorizePreds(userId, groupIds, preds, acl.Write)
+		return nil
 	}
 
 	err := doAuthorizeMutation()
+
 	span := otrace.FromContext(ctx)
 	if span != nil {
 		span.Annotatef(nil, (&accessEntry{
@@ -714,35 +719,26 @@ func authorizeQuery(ctx context.Context, parsedReq *gql.Result) error {
 	var userId string
 	var groupIds []string
 	preds := parsePredsFromQuery(parsedReq.Query)
-	isSchemaQuery := parsedReq != nil && parsedReq.Schema != nil
 
-	doAuthorizeQuery := func() error {
+	doAuthorizeQuery := func() (map[string]struct{}, error) {
 		userData, err := extractUserAndGroups(ctx)
-		switch {
-		case err == errNoJwt:
-			// Do not allow schema queries unless the user has logged in.
-			if isSchemaQuery {
-				return status.Error(codes.Unauthenticated, err.Error())
-			}
+		if err != nil {
+			return nil, status.Error(codes.Unauthenticated, err.Error())
+		}
 
-			// Treat the user as an anonymous guest who has not joined any group yet
-			// such a user can still get access to predicates that have no ACL rule defined.
-		case err != nil:
-			return status.Error(codes.Unauthenticated, err.Error())
-		default:
-			userId = userData[0]
-			groupIds = userData[1:]
+		userId = userData[0]
+		groupIds = userData[1:]
 
-			if x.IsGuardian(groupIds) {
-				// Members of guardian groups are allowed to query anything.
-				return nil
-			}
+		if x.IsGuardian(groupIds) {
+			// Members of guardian groups are allowed to query anything.
+			return nil, nil
 		}
 
-		return authorizePreds(userId, groupIds, preds, acl.Read)
+		return authorizePreds(userId, groupIds, preds, acl.Read), nil
 	}
 
-	err := doAuthorizeQuery()
+	blockedPreds, err := doAuthorizeQuery()
+
 	if span := otrace.FromContext(ctx); span != nil {
 		span.Annotatef(nil, (&accessEntry{
 			userId:    userId,
@@ -753,7 +749,15 @@ func authorizeQuery(ctx context.Context, parsedReq *gql.Result) error {
 		}).String())
 	}
 
-	return err
+	if err != nil {
+		return err
+	}
+
+	if len(blockedPreds) != 0 {
+		parsedReq.Query = removePredsFromQuery(parsedReq.Query, blockedPreds)
+	}
+
+	return nil
 }
 
 // authorizeState authorizes the State operation
@@ -785,3 +789,27 @@ func authorizeState(ctx context.Context) error {
 
 	return doAuthorizeState()
 }
+
+func removePredsFromQuery(gqls []*gql.GraphQuery,
+	blockedPreds map[string]struct{}) []*gql.GraphQuery {
+
+	filter := gqls[:0]
+	for _, gq := range gqls {
+		if gq.Func != nil && len(gq.Func.Attr) > 0 {
+			if _, ok := blockedPreds[gq.Func.Attr]; ok {
+				continue
+			}
+		}
+
+		if len(gq.Attr) > 0 {
+			if _, ok := blockedPreds[gq.Attr]; ok {
+				continue
+			}
+		}
+
+		gq.Children = removePredsFromQuery(gq.Children, blockedPreds)
+		filter = append(filter, gq)
+	}
+
+	return filter
+}

ee/acl/acl_curl_test.go

@@ -44,15 +44,15 @@ func TestCurlAuthorization(t *testing.T) {
 	})
 	require.NoError(t, err, "login failed")
 
-	// No ACL rules are specified, so everything should fail.
+	// No ACL rules are specified, so query should return empty response,
+	// alter and mutate should fail.
 	queryArgs := func(jwt string) []string {
 		return []string{"-H", fmt.Sprintf("X-Dgraph-AccessToken:%s", jwt),
 			"-H", "Content-Type: application/graphql+-",
 			"-d", query, curlQueryEndpoint}
 	}
 	testutil.VerifyCurlCmd(t, queryArgs(accessJwt), &testutil.CurlFailureConfig{
-		ShouldFail:   true,
-		DgraphErrMsg: "PermissionDenied",
+		ShouldFail: false,
 	})
 
 	mutateArgs := func(jwt string) []string {
@@ -116,11 +116,10 @@ func TestCurlAuthorization(t *testing.T) {
 		RefreshJwt: refreshJwt,
 	})
 	require.NoError(t, err, fmt.Sprintf("login through refresh token failed: %v", err))
-	// verify that with an ACL rule defined, all the operations should be denied when the acsess JWT
-	// does not have the required permissions
+	// verify that with an ACL rule defined, all the operations except query should
+	// does not have the required permissions be denied when the acsess JWT
 	testutil.VerifyCurlCmd(t, queryArgs(accessJwt), &testutil.CurlFailureConfig{
-		ShouldFail:   true,
-		DgraphErrMsg: "PermissionDenied",
+		ShouldFail: false,
 	})
 	testutil.VerifyCurlCmd(t, mutateArgs(accessJwt), &testutil.CurlFailureConfig{
 		ShouldFail:   true,