fix: UserTypeRemoteUser is an eligible organization member (#11326)

A [safeguard][0] was added to ensure a team member is not an organization. A [discussion][1] concluded that remote users should not yet be allowed to be team members, at least until it is implemented (by F3 or ActivityPub).

As [gof3 v3.11.25][2] has support for [teams][3], remote users need to be added as an eligible team member, otherwise compliance tests for the Forgejo internal driver [implementation][4] fail.

The IsUserByID function is renamed to IsAnEligibleTeamMemberByID to clarify its purpose. The TestIsUserConsistency is removed because the IsUser function is used in [an entirely different context][5] and there is no reason to ensure both are consistent. A unit test is added for IsAnEligibleTeamMemberByID as a replacement and also verifies organizations are not considered eligible.

[0]: https://codeberg.org/forgejo/forgejo/pulls/9757
[1]: https://codeberg.org/forgejo/forgejo/pulls/9757#issuecomment-7802255
[2]: https://code.forgejo.org/f3/gof3/releases/tag/v3.11.25
[3]: https://code.forgejo.org/f3/gof3/pulls/484/files
[4]: 9c565a9b37 (diff-241521b6ebc5a1da660421daea23da1dd1086158)
[5]: b66a76686d/routers/api/v1/api.go (L299-L308)

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11326
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
Co-authored-by: limiting-factor <limiting-factor@posteo.com>
Co-committed-by: limiting-factor <limiting-factor@posteo.com>

models/organization/org.go

@@ -510,10 +510,10 @@ func ChangeOrgUserStatus(ctx context.Context, orgID, uid int64, public bool) err
 
 // AddOrgUser adds new user to given organization.
 func AddOrgUser(ctx context.Context, orgID, uid int64) error {
-	isUser, err := user_model.IsUserByID(ctx, uid)
+	eligible, err := IsAnEligibleTeamMemberByID(ctx, uid)
 	if err != nil {
 		return err
-	} else if !isUser {
+	} else if !eligible {
 		return user_model.ErrUserWrongType{UID: uid}
 	}
 

models/organization/org_user.go

@@ -112,6 +112,15 @@ func IsUserOrgOwner(ctx context.Context, users user_model.UserList, orgID int64)
 	return results
 }
 
+// Returns true if the given user ID is allowed to be a team member
+func IsAnEligibleTeamMemberByID(ctx context.Context, uid int64) (bool, error) {
+	return db.GetEngine(ctx).
+		Where("id=?", uid).
+		In("type", user_model.UserTypeIndividual, user_model.UserTypeBot, user_model.UserTypeRemoteUser).
+		Table("user").
+		Exist()
+}
+
 func loadOrganizationOwners(ctx context.Context, users user_model.UserList, orgID int64) (map[int64]*TeamUser, error) {
 	if len(users) == 0 {
 		return nil, nil

models/organization/org_user_test.go

@@ -162,3 +162,41 @@ func TestAddOrgUser(t *testing.T) {
 
 	unittest.CheckConsistencyFor(t, &user_model.User{}, &organization.Team{})
 }
+
+func TestIsAnEligibleTeamMemberByID(t *testing.T) {
+	defer unittest.OverrideFixtures("models/user/fixtures/")()
+	require.NoError(t, unittest.PrepareTestDatabase())
+
+	for _, testCase := range []struct {
+		name     string
+		id       int64
+		eligible bool
+	}{
+		{
+			name:     "Regular user",
+			id:       1,
+			eligible: true,
+		},
+		{
+			name:     "Bot user",
+			id:       1042,
+			eligible: true,
+		},
+		{
+			name:     "Organization",
+			id:       3,
+			eligible: false,
+		},
+		{
+			name:     "F3 Remote user",
+			id:       1041,
+			eligible: true,
+		},
+	} {
+		t.Run(testCase.name, func(t *testing.T) {
+			eligible, err := organization.IsAnEligibleTeamMemberByID(t.Context(), testCase.id)
+			require.NoError(t, err)
+			assert.Equal(t, testCase.eligible, eligible)
+		})
+	}
+}

models/user/user.go

@@ -464,15 +464,6 @@ func (u *User) IsUser() bool {
 	return u.Type == UserTypeIndividual || u.Type == UserTypeBot
 }
 
-// Returns true if the given user ID belongs to an actual user, not an organization
-func IsUserByID(ctx context.Context, uid int64) (bool, error) {
-	return db.GetEngine(ctx).
-		Where("id=?", uid).
-		In("type", UserTypeIndividual, UserTypeBot).
-		Table("user").
-		Exist()
-}
-
 // IsBot returns whether or not the user is of type bot
 func (u *User) IsBot() bool {
 	return u.Type == UserTypeBot

models/user/user_test.go

@@ -1113,20 +1113,3 @@ func TestGetUserByEmailSimple(t *testing.T) {
 		assert.Nil(t, u)
 	})
 }
-
-func TestIsUserConsistency(t *testing.T) {
-	defer unittest.OverrideFixtures("models/user/fixtures/")()
-	require.NoError(t, unittest.PrepareTestDatabase())
-
-	test := func(userID int64) {
-		user, err := user_model.GetUserByID(t.Context(), userID)
-		require.NoError(t, err)
-		isUser, err := user_model.IsUserByID(t.Context(), userID)
-		require.NoError(t, err)
-		assert.Equal(t, user.IsUser(), isUser)
-	}
-
-	test(1)
-	test(1041)
-	test(1042)
-}