fix: prevent orgs from being added as members of orgs (#9757)

Resolves #4167

Changes it to check that the user being added is `User` or `Bot` type before allowing it

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/9757
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
Co-authored-by: Cyborus <cyborus@cyborus.xyz>
Co-committed-by: Cyborus <cyborus@cyborus.xyz>

.deadcode-out

@@ -54,6 +54,7 @@ forgejo.org/models/repo
 	WatchRepoMode
 
 forgejo.org/models/user
+	IsErrUserWrongType
 	IsErrExternalLoginUserAlreadyExist
 	IsErrExternalLoginUserNotExist
 	NewFederatedUser

models/organization/org.go

@@ -510,6 +510,13 @@ 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)
+	if err != nil {
+		return err
+	} else if !isUser {
+		return user_model.ErrUserWrongType{UID: uid}
+	}
+
 	isAlreadyMember, err := IsOrganizationMember(ctx, orgID, uid)
 	if err != nil || isAlreadyMember {
 		return err

models/organization/org_user_test.go

@@ -142,6 +142,15 @@ func TestAddOrgUser(t *testing.T) {
 		org = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: orgID})
 		assert.Equal(t, expectedNumMembers, org.NumMembers)
 	}
+	testFailure := func(orgID, userID int64, isPublic bool) {
+		org := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: orgID})
+		expectedNumMembers := org.NumMembers
+		require.ErrorIs(t, organization.AddOrgUser(db.DefaultContext, orgID, userID), user_model.ErrUserWrongType{UID: userID})
+		ou := &organization.OrgUser{OrgID: orgID, UID: userID}
+		unittest.AssertNotExistsBean(t, ou)
+		org = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: orgID})
+		assert.Equal(t, expectedNumMembers, org.NumMembers)
+	}
 
 	setting.Service.DefaultOrgMemberVisible = false
 	testSuccess(3, 5, false)
@@ -149,7 +158,7 @@ func TestAddOrgUser(t *testing.T) {
 	testSuccess(6, 2, false)
 
 	setting.Service.DefaultOrgMemberVisible = true
-	testSuccess(6, 3, true)
+	testFailure(6, 3, true)
 
 	unittest.CheckConsistencyFor(t, &user_model.User{}, &organization.Team{})
 }

models/user/error.go

@@ -50,6 +50,25 @@ func (err ErrUserNotExist) Unwrap() error {
 	return util.ErrNotExist
 }
 
+// ErrUserWrongType is returned if the user is of the wrong type (i.e. is an org when a user was expected)
+type ErrUserWrongType struct {
+	UID int64
+}
+
+func IsErrUserWrongType(err error) bool {
+	_, ok := err.(ErrUserNotExist)
+	return ok
+}
+
+func (err ErrUserWrongType) Error() string {
+	return fmt.Sprintf("user is the wrong user type [uid: %d]", err.UID)
+}
+
+// Unwrap unwraps this error as a ErrNotExist error
+func (err ErrUserWrongType) Unwrap() error {
+	return util.ErrInvalidArgument
+}
+
 // ErrUserProhibitLogin represents a "ErrUserProhibitLogin" kind of error.
 type ErrUserProhibitLogin struct {
 	UID  int64

models/user/fixtures/user.yml

@@ -35,3 +35,40 @@
   repo_admin_change_team_access: false
   theme: ""
   keep_activity_private: false
+
+-
+  id: 1042
+  lower_name: bot01
+  name: bot01
+  full_name: Bot01
+  email: bot01@example.com
+  keep_email_private: false
+  email_notifications_preference: onmention
+  passwd: ZogKvWdyEx:password
+  passwd_hash_algo: dummy
+  must_change_password: false
+  login_source: 0
+  login_name: bot01
+  type: 4
+  salt: ZogKvWdyEx
+  max_repo_creation: -1
+  is_active: true
+  is_admin: false
+  is_restricted: false
+  allow_git_hook: false
+  allow_import_local: false
+  allow_create_organization: true
+  prohibit_login: true
+  avatar: ""
+  avatar_email: avatarbot01@example.com
+  use_custom_avatar: false
+  num_followers: 0
+  num_following: 0
+  num_stars: 0
+  num_repos: 0
+  num_teams: 0
+  num_members: 0
+  visibility: 0
+  repo_admin_change_team_access: false
+  theme: ""
+  keep_activity_private: false

models/user/user.go

@@ -461,6 +461,15 @@ 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

@@ -1056,3 +1056,20 @@ 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)
+}