From a68791d7075f1ffb7433a7134ab9837a93d8992c Mon Sep 17 00:00:00 2001 From: Cyborus Date: Sat, 8 Nov 2025 22:05:21 +0100 Subject: [PATCH] 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 Co-authored-by: Cyborus Co-committed-by: Cyborus --- .deadcode-out | 1 + models/organization/org.go | 7 ++++++ models/organization/org_user_test.go | 11 ++++++++- models/user/error.go | 19 ++++++++++++++ models/user/fixtures/user.yml | 37 ++++++++++++++++++++++++++++ models/user/user.go | 9 +++++++ models/user/user_test.go | 17 +++++++++++++ 7 files changed, 100 insertions(+), 1 deletion(-) diff --git a/.deadcode-out b/.deadcode-out index b2db98c36b..f79416ebf3 100644 --- a/.deadcode-out +++ b/.deadcode-out @@ -54,6 +54,7 @@ forgejo.org/models/repo WatchRepoMode forgejo.org/models/user + IsErrUserWrongType IsErrExternalLoginUserAlreadyExist IsErrExternalLoginUserNotExist NewFederatedUser diff --git a/models/organization/org.go b/models/organization/org.go index c4df5d4fe1..6da8886c2f 100644 --- a/models/organization/org.go +++ b/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 diff --git a/models/organization/org_user_test.go b/models/organization/org_user_test.go index a1472fde4b..4011e5df88 100644 --- a/models/organization/org_user_test.go +++ b/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{}) } diff --git a/models/user/error.go b/models/user/error.go index a0fc1af2bd..264b3fdcd2 100644 --- a/models/user/error.go +++ b/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 diff --git a/models/user/fixtures/user.yml b/models/user/fixtures/user.yml index 137064a368..9cf73b4306 100644 --- a/models/user/fixtures/user.yml +++ b/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 diff --git a/models/user/user.go b/models/user/user.go index bf9060e0bc..69d86ab740 100644 --- a/models/user/user.go +++ b/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 diff --git a/models/user/user_test.go b/models/user/user_test.go index 50d741c839..080253191e 100644 --- a/models/user/user_test.go +++ b/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) +}