Merge pull request #9788 from rhafer/filter-usertype

feat(graph): add support for filtering users by type

changelog/unreleased/ocm_users_graph.md

@@ -0,0 +1,14 @@
+Enhancement: OCM related adjustments in graph
+
+The /users enpdoint of the graph service was changed with respect to how
+it handles OCM federeated users:
+- The 'userType' property is now alway returned. As new usertype 'Federated'
+  was introduced. To indicate that the user is a federated user.
+- Supported for filtering users by 'userType' as added. Queries like
+  "$filter=userType eq 'Federated'" are now possible.
+- Federated users are only returned when explicitly requested via filter.
+  When no filter is provider only 'Member' users are returned.
+
+https://github.com/owncloud/ocis/pull/9788
+https://github.com/owncloud/ocis/pull/9757
+https://github.com/owncloud/ocis/issues/9702

services/graph/pkg/service/v0/users.go

@@ -237,7 +237,21 @@ func (g Graph) GetUsers(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	if !ctxHasFullPerms && (odataReq.Query.Filter != nil || odataReq.Query.Apply != nil || odataReq.Query.Expand != nil || odataReq.Query.Compute != nil) {
+	if !ctxHasFullPerms && odataReq.Query.Filter != nil {
+		// regular users are allowed to filter only by userType
+		filter := odataReq.Query.Filter
+		switch {
+		case filter.Tree.Token.Type != godata.ExpressionTokenLogical:
+			fallthrough
+		case filter.Tree.Token.Value != "eq":
+			fallthrough
+		case filter.Tree.Children[0].Token.Value != "userType":
+			logger.Debug().Interface("query", r.URL.Query()).Msg("forbidden filter for a regular user")
+			errorcode.AccessDenied.Render(w, r, http.StatusForbidden, "filter has forbidden elements for regular users")
+			return
+		}
+	}
+	if !ctxHasFullPerms && (odataReq.Query.Apply != nil || odataReq.Query.Expand != nil || odataReq.Query.Compute != nil) {
 		// regular users can't use filter, apply, expand and compute
 		logger.Debug().Interface("query", r.URL.Query()).Msg("forbidden query elements for a regular user")
 		errorcode.AccessDenied.Render(w, r, http.StatusForbidden, "query has forbidden elements for regular users")
@@ -254,34 +268,6 @@ func (g Graph) GetUsers(w http.ResponseWriter, r *http.Request) {
 		users, err = g.identityBackend.GetUsers(r.Context(), odataReq)
 	}
 
-	if g.config.IncludeOCMSharees {
-		gwc, err := g.gatewaySelector.Next()
-		if err != nil {
-			errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, err.Error())
-			return
-		}
-		term, err := identity.GetSearchValues(odataReq.Query)
-		if err != nil {
-			errorcode.GeneralException.Render(w, r, http.StatusBadRequest, err.Error())
-			return
-		}
-
-		remoteUsersRes, err := gwc.FindAcceptedUsers(r.Context(), &invitepb.FindAcceptedUsersRequest{Filter: term})
-		if err != nil {
-			// TODO grpc FindAcceptedUsers call failed
-			errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, err.Error())
-			return
-		}
-		if remoteUsersRes.Status.Code != cs3rpc.Code_CODE_OK {
-			// TODO "error searching remote users"
-			errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, remoteUsersRes.Status.Message)
-			return
-		}
-		for _, user := range remoteUsersRes.GetAcceptedUsers() {
-			users = append(users, identity.CreateUserModelFromCS3(user))
-		}
-	}
-
 	if err != nil {
 		logger.Debug().Err(err).Interface("query", r.URL.Query()).Msg("could not get users from backend")
 		var errcode errorcode.Error
@@ -1014,3 +1000,38 @@ func isValidUserType(userType string) bool {
 
 	return false
 }
+
+func (g Graph) searchOCMAcceptedUsers(ctx context.Context, odataReq *godata.GoDataRequest) ([]*libregraph.User, error) {
+	logger := g.logger.SubloggerWithRequestID(ctx)
+	users := []*libregraph.User{}
+
+	if !g.config.IncludeOCMSharees {
+		logger.Debug().Msg("OCM sharing is disabled")
+		return users, nil
+	}
+
+	gwc, err := g.gatewaySelector.Next()
+	if err != nil {
+		return nil, errorcode.New(errorcode.GeneralException, err.Error())
+	}
+	term, err := identity.GetSearchValues(odataReq.Query)
+	if err != nil {
+		return nil, errorcode.New(errorcode.InvalidRequest, err.Error())
+	}
+
+	remoteUsersRes, err := gwc.FindAcceptedUsers(ctx, &invitepb.FindAcceptedUsersRequest{Filter: term})
+	if err != nil {
+		logger.Error().Err(err).Msg("FindAcceptedUsers call failed")
+		return nil, errorcode.New(errorcode.GeneralException, err.Error())
+	}
+	if remoteUsersRes.GetStatus().GetCode() != cs3rpc.Code_CODE_OK {
+		return nil, errorcode.FromCS3Status(remoteUsersRes.Status, nil)
+	}
+
+	cs3Users := remoteUsersRes.GetAcceptedUsers()
+	users = make([]*libregraph.User, 0, len(cs3Users))
+	for _, user := range cs3Users {
+		users = append(users, identity.CreateUserModelFromCS3(user))
+	}
+	return users, nil
+}

services/graph/pkg/service/v0/users_filter.go

@@ -124,19 +124,19 @@ func (g Graph) applyFilterLogical(ctx context.Context, req *godata.GoDataRequest
 		return users, invalidFilterError()
 	}
 
+	// As we currently don't suppport the 'has' or 'in' operator, all our
+	// currently supported user filters of the ExpressionTokenLogical type
+	// require exactly two operands.
+	if len(root.Children) != 2 {
+		return users, invalidFilterError()
+	}
 	switch root.Token.Value {
 	case "and":
-		// 'and' needs 2 operands
-		if len(root.Children) != 2 {
-			return users, invalidFilterError()
-		}
 		return g.applyFilterLogicalAnd(ctx, req, root.Children[0], root.Children[1])
 	case "or":
-		// 'or' needs 2 operands
-		if len(root.Children) != 2 {
-			return users, invalidFilterError()
-		}
 		return g.applyFilterLogicalOr(ctx, req, root.Children[0], root.Children[1])
+	case "eq":
+		return g.applyFilterEq(ctx, req, root.Children[0], root.Children[1])
 	}
 	logger.Debug().Str("Token", root.Token.Value).Msg("unsupported logical filter")
 	return users, unsupportedFilterError()
@@ -222,6 +222,29 @@ func (g Graph) applyFilterLogicalOr(ctx context.Context, req *godata.GoDataReque
 	}
 	return filteredUsers, nil
 }
+
+func (g Graph) applyFilterEq(ctx context.Context, req *godata.GoDataRequest, operand1 *godata.ParseNode, operand2 *godata.ParseNode) (users []*libregraph.User, err error) {
+	// We only support the 'eq' on 'userType' for now
+	switch {
+	case operand1.Token.Type != godata.ExpressionTokenLiteral:
+		fallthrough
+	case operand1.Token.Value != "userType":
+		fallthrough
+	case operand2.Token.Type != godata.ExpressionTokenString:
+		return users, unsupportedFilterError()
+	}
+
+	// unquote
+	value := strings.Trim(operand2.Token.Value, "'")
+	switch value {
+	case "Member", "Guest":
+		return g.identityBackend.GetUsers(ctx, req)
+	case "Federated":
+		return g.searchOCMAcceptedUsers(ctx, req)
+	}
+	return users, unsupportedFilterError()
+}
+
 func (g Graph) applyFilterLambda(ctx context.Context, req *godata.GoDataRequest, nodes []*godata.ParseNode) (users []*libregraph.User, err error) {
 	logger := g.logger.SubloggerWithRequestID(ctx)
 	if len(nodes) != 2 {

services/graph/pkg/service/v0/users_test.go

@@ -291,6 +291,14 @@ var _ = Describe("Users", func() {
 
 				Expect(rr.Code).To(Equal(http.StatusForbidden))
 			})
+			It("denies filters other that 'userType eq ...' for unprivileged users", func() {
+				permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Return(&settings.GetPermissionByIDResponse{}, nil)
+				r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/users?$search=%22abc%22&$filter="+url.QueryEscape("memberOf/any(n:n/id eq 25cb7bc0-3168-4a0c-adbe-396f478ad494)"), nil)
+				svc.GetUsers(rr, r)
+
+				Expect(rr.Code).To(Equal(http.StatusForbidden))
+			})
+
 			It("only returns a restricted set of attributes for unprivileged users", func() {
 				permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Return(&settings.GetPermissionByIDResponse{}, nil)
 				user := &libregraph.User{}
@@ -503,8 +511,8 @@ var _ = Describe("Users", func() {
 				"appRoleAssignments/all(n:n/appRoleId eq 'id') and appRoleAssignments/any(n:n/appRoleId eq 'id')", http.StatusNotImplemented),
 			Entry("with unsupported appRoleAssignments lambda filter operation",
 				"appRoleAssignments/any(n:n/appRoleId ne 'id') and appRoleAssignments/any(n:n/appRoleId eq 'id')", http.StatusNotImplemented),
-			Entry("with unsupported appRoleAssignments lambda filter operation",
-				"appRoleAssignments/any(n:n/appRoleId eq 1) and appRoleAssignments/any(n:n/appRoleId eq 'id')", http.StatusNotImplemented),
+			Entry("with unsupported userType in filter", "userType eq 'unsupported'", http.StatusNotImplemented),
+			Entry("with unsupported property in eq filter", "unsupported eq 'unsupported'", http.StatusNotImplemented),
 		)
 
 		DescribeTable("With a valid filter",
@@ -1125,7 +1133,28 @@ var _ = Describe("Users", func() {
 			)
 		})
 		Describe("GetUsers", func() {
-			It("lists the users", func() {
+			It("does not list the federated users without a filter", func() {
+				gatewayClient.AssertNotCalled(GinkgoT(), "FindAcceptedUsers, mock.Anything, mock.Anything")
+				permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Return(&settings.GetPermissionByIDResponse{}, nil)
+
+				identityBackend.On("GetUsers", mock.Anything, mock.Anything, mock.Anything).Return(
+					[]*libregraph.User{}, nil,
+				)
+
+				r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/users?$search=user", nil)
+				svc.GetUsers(rr, r)
+
+				Expect(rr.Code).To(Equal(http.StatusOK))
+				data, err := io.ReadAll(rr.Body)
+				Expect(err).ToNot(HaveOccurred())
+
+				res := userList{}
+				err = json.Unmarshal(data, &res)
+				Expect(err).ToNot(HaveOccurred())
+
+				Expect(len(res.Value)).To(Equal(0))
+			})
+			It("does list federated users when the right filter is used", func() {
 				gatewayClient.On("FindAcceptedUsers", mock.Anything, mock.Anything).Return(&invitepb.FindAcceptedUsersResponse{
 					Status: status.NewOK(ctx),
 					AcceptedUsers: []*userv1beta1.User{
@@ -1138,18 +1167,13 @@ var _ = Describe("Users", func() {
 						},
 					},
 				}, nil)
-				permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Return(&settings.GetPermissionByIDResponse{
-					Permission: &settingsmsg.Permission{
-						Operation:  settingsmsg.Permission_OPERATION_UNKNOWN,
-						Constraint: settingsmsg.Permission_CONSTRAINT_ALL,
-					},
-				}, nil)
+				permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Return(&settings.GetPermissionByIDResponse{}, nil)
 
 				identityBackend.On("GetUsers", mock.Anything, mock.Anything, mock.Anything).Return(
 					[]*libregraph.User{}, nil,
 				)
 
-				r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/users", nil)
+				r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/users?$search=federated&$filter="+url.QueryEscape("userType eq 'Federated'"), nil)
 				svc.GetUsers(rr, r)
 
 				Expect(rr.Code).To(Equal(http.StatusOK))
@@ -1164,6 +1188,33 @@ var _ = Describe("Users", func() {
 				Expect(res.Value[0].GetId()).To(Equal("federated"))
 				Expect(res.Value[0].GetUserType()).To(Equal("Federated"))
 			})
+			It("does not list federated users when filtering for 'Member' users", func() {
+				permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Return(&settings.GetPermissionByIDResponse{}, nil)
+
+				user := &libregraph.User{}
+				user.SetId("user1")
+				user.SetUserType("Member")
+				users := []*libregraph.User{user}
+
+				identityBackend.On("GetUsers", mock.Anything, mock.Anything, mock.Anything).Return(
+					users, nil,
+				)
+
+				r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/users?$search=user&$filter="+url.QueryEscape("userType eq 'Member'"), nil)
+				svc.GetUsers(rr, r)
+
+				Expect(rr.Code).To(Equal(http.StatusOK))
+				data, err := io.ReadAll(rr.Body)
+				Expect(err).ToNot(HaveOccurred())
+
+				res := userList{}
+				err = json.Unmarshal(data, &res)
+				Expect(err).ToNot(HaveOccurred())
+
+				Expect(len(res.Value)).To(Equal(1))
+				Expect(res.Value[0].GetId()).To(Equal("user1"))
+				Expect(res.Value[0].GetUserType()).To(Equal("Member"))
+			})
 		})
 	})
 })

tests/TestHelpers/GraphHelper.php

@@ -306,6 +306,32 @@ class GraphHelper {
 		);
 	}
 
+	/**
+	 * @param string $baseUrl
+	 * @param string $xRequestId
+	 * @param string $adminUser
+	 * @param string $adminPassword
+	 * @param string $searchTerm
+	 *
+	 * @return ResponseInterface
+	 */
+	public static function searchFederatedUser(
+		string $baseUrl,
+		string $xRequestId,
+		string $adminUser,
+		string $adminPassword,
+		string $searchTerm
+	): ResponseInterface {
+		$url = self::getFullUrl($baseUrl, "users?\$filter=userType eq 'Federated'&\$search=$searchTerm");
+		return HttpRequestHelper::get(
+			$url,
+			$xRequestId,
+			$adminUser,
+			$adminPassword,
+			self::getRequestHeaders()
+		);
+	}
+
 	/**
 	 * @param string $baseUrl
 	 * @param string $xRequestId

tests/acceptance/features/apiOcm/searchFederationUsers.feature

@@ -18,7 +18,7 @@ Feature: search federation users
     And "Alice" has created the federation share invitation
     And using server "REMOTE"
     And "Brian" has accepted invitation
-    When user "Brian" searches for user "ali" using Graph API
+    When user "Brian" searches for federated user "ali" using Graph API
     Then the HTTP status code should be "200"
     And the JSON data of the response should match
       """
@@ -53,7 +53,7 @@ Feature: search federation users
       }
       """
     And using server "LOCAL"
-    When user "Alice" searches for user "bri" using Graph API
+    When user "Alice" searches for federated user "bri" using Graph API
     Then the HTTP status code should be "200"
     And the JSON data of the response should match
       """
@@ -94,7 +94,7 @@ Feature: search federation users
     And "Alice" has created the federation share invitation
     And using server "REMOTE"
     And "Brian" has accepted invitation
-    When user "Brian" searches for user "%22alice@example.org%22" using Graph API
+    When user "Brian" searches for federated user "%22alice@example.org%22" using Graph API
     Then the HTTP status code should be "200"
     And the JSON data of the response should match
       """
@@ -129,7 +129,7 @@ Feature: search federation users
       }
       """
     And using server "LOCAL"
-    When user "Alice" searches for user "%22brian@example.org%22" using Graph API
+    When user "Alice" searches for federated user "%22brian@example.org%22" using Graph API
     Then the HTTP status code should be "200"
     And the JSON data of the response should match
       """
@@ -170,7 +170,7 @@ Feature: search federation users
     And "Alice" has created the federation share invitation
     And using server "REMOTE"
     And "Brian" has accepted invitation
-    When user "Brian" searches for user "%22carol@example.org%22" using Graph API
+    When user "Brian" searches for federated user "%22carol@example.org%22" using Graph API
     Then the HTTP status code should be "200"
     And the JSON data of the response should match
       """
@@ -189,7 +189,7 @@ Feature: search federation users
       }
       """
     And using server "LOCAL"
-    When user "Carol" searches for user "bria" using Graph API
+    When user "Carol" searches for federated user "bria" using Graph API
     Then the HTTP status code should be "200"
     And the JSON data of the response should match
       """
@@ -325,4 +325,4 @@ Feature: search federation users
       """
 
 
-# TODO try to find federation users after deleting federated conection
+# TODO try to find federation users after deleting federated conection

tests/acceptance/features/bootstrap/GraphContext.php

@@ -1253,6 +1253,27 @@ class GraphContext implements Context {
 		$this->featureContext->setResponse($response);
 	}
 
+	/**
+	 * @When user :byUser searches for federated user :searchTerm using Graph API
+	 *
+	 * @param string $byUser
+	 * @param string $searchTerm
+	 *
+	 * @return void
+	 * @throws GuzzleException
+	 */
+	public function userSearchesForFederatedUserUsingGraphApi(string $byUser, string $searchTerm): void {
+		$credentials = $this->getAdminOrUserCredentials($byUser);
+		$response = GraphHelper::searchFederatedUser(
+			$this->featureContext->getBaseUrl(),
+			$this->featureContext->getStepLineRef(),
+			$credentials['username'],
+			$credentials['password'],
+			$searchTerm,
+		);
+		$this->featureContext->setResponse($response);
+	}
+
 	/**
 	 * @When user :user tries to get all users using the Graph API
 	 * @When user :user gets all users using the Graph API