refactor(bots): extract shared bot identity, share labels between sibling bots

pkg/db/fixtures/labels.yml

@@ -64,3 +64,9 @@
   created_by_id: 21
   updated: 2018-12-02 15:13:12
   created: 2018-12-01 15:13:12
+# Must remain unattached so sibling-access tests exercise identity access, not task access.
+- id: 12
+  title: 'Label #12 - created by bot 25, sibling of bot 23'
+  created_by_id: 25
+  updated: 2018-12-02 15:13:12
+  created: 2018-12-01 15:13:12

pkg/db/fixtures/users.yml

@@ -1,9 +1,13 @@
+# Humans carry an explicit bot_owner_id: 0 because that is what CreateUser
+# writes. NULL here silently disarms the bot_owner_id > 0 identity guard.
+# User 20 is the one deliberate exception - see there.
 -
   id: 1
   username: 'user1'
   password: '$2a$04$X4aRMEt0ytgPwMIgv36cI..7X9.nhY/.tYwxpqSi0ykRHx2CwQ0S6' # 12345678
   email: 'user1@example.com'
   issuer: local
+  bot_owner_id: 0
   updated: 2018-12-02 15:13:12
   created: 2018-12-01 15:13:12
   export_file_id: 1
@@ -13,6 +17,7 @@
   password: '$2a$04$X4aRMEt0ytgPwMIgv36cI..7X9.nhY/.tYwxpqSi0ykRHx2CwQ0S6' # 12345678
   email: 'user2@example.com'
   issuer: local
+  bot_owner_id: 0
   default_project_id: 4
   updated: 2018-12-02 15:13:12
   created: 2018-12-01 15:13:12
@@ -22,6 +27,7 @@
   password: '$2a$04$X4aRMEt0ytgPwMIgv36cI..7X9.nhY/.tYwxpqSi0ykRHx2CwQ0S6' # 12345678
   email: 'user3@example.com'
   issuer: local
+  bot_owner_id: 0
   default_project_id: 4
   updated: 2018-12-02 15:13:12
   created: 2018-12-01 15:13:12
@@ -32,6 +38,7 @@
   email: 'user4@example.com'
   status: 1
   issuer: local
+  bot_owner_id: 0
   updated: 2018-12-02 15:13:12
   created: 2018-12-01 15:13:12
 -
@@ -41,6 +48,7 @@
   email: 'user5@example.com'
   status: 1
   issuer: local
+  bot_owner_id: 0
   updated: 2018-12-02 15:13:12
   created: 2018-12-01 15:13:12
 # This use is used to create a whole bunch of projects which are then shared directly with a user
@@ -49,13 +57,15 @@
   password: '$2a$04$X4aRMEt0ytgPwMIgv36cI..7X9.nhY/.tYwxpqSi0ykRHx2CwQ0S6' # 12345678
   email: 'user6@example.com'
   issuer: local
+  bot_owner_id: 0
   updated: 2018-12-02 15:13:12
   created: 2018-12-01 15:13:12
 - id: 7
   username: 'user7'
   password: '$2a$04$X4aRMEt0ytgPwMIgv36cI..7X9.nhY/.tYwxpqSi0ykRHx2CwQ0S6' # 12345678
   email: 'user7@example.com'
   issuer: local
+  bot_owner_id: 0
   discoverable_by_email: true
   updated: 2018-12-02 15:13:12
   created: 2018-12-01 15:13:12
@@ -64,20 +74,23 @@
   password: '$2a$04$X4aRMEt0ytgPwMIgv36cI..7X9.nhY/.tYwxpqSi0ykRHx2CwQ0S6' # 12345678
   email: 'user8@example.com'
   issuer: local
+  bot_owner_id: 0
   updated: 2018-12-02 15:13:12
   created: 2018-12-01 15:13:12
 - id: 9
   username: 'user9'
   password: '$2a$04$X4aRMEt0ytgPwMIgv36cI..7X9.nhY/.tYwxpqSi0ykRHx2CwQ0S6' # 12345678
   email: 'user9@example.com'
   issuer: local
+  bot_owner_id: 0
   updated: 2018-12-02 15:13:12
   created: 2018-12-01 15:13:12
 - id: 10
   username: 'user10'
   password: '$2a$04$X4aRMEt0ytgPwMIgv36cI..7X9.nhY/.tYwxpqSi0ykRHx2CwQ0S6' # 12345678
   email: 'user10@example.com'
   issuer: local
+  bot_owner_id: 0
   updated: 2018-12-02 15:13:12
   created: 2018-12-01 15:13:12
 - id: 11
@@ -86,6 +99,7 @@
   password: '$2a$04$X4aRMEt0ytgPwMIgv36cI..7X9.nhY/.tYwxpqSi0ykRHx2CwQ0S6' # 12345678
   email: 'user11@example.com'
   issuer: local
+  bot_owner_id: 0
   updated: 2018-12-02 15:13:12
   created: 2018-12-01 15:13:12
 - id: 12
@@ -94,6 +108,7 @@
   password: '$2a$04$X4aRMEt0ytgPwMIgv36cI..7X9.nhY/.tYwxpqSi0ykRHx2CwQ0S6' # 12345678
   email: 'user12@example.com'
   issuer: local
+  bot_owner_id: 0
   discoverable_by_name: true
   updated: 2018-12-02 15:13:12
   created: 2018-12-01 15:13:12
@@ -102,6 +117,7 @@
   password: '$2a$04$X4aRMEt0ytgPwMIgv36cI..7X9.nhY/.tYwxpqSi0ykRHx2CwQ0S6' # 12345678
   email: 'user14@example.com'
   issuer: local
+  bot_owner_id: 0
   updated: 2018-12-02 15:13:12
   created: 2018-12-01 15:13:12
 - id: 14
@@ -110,20 +126,23 @@
   email: 'user15@some.service.com'
   issuer: 'https://some.service.com'
   subject: '12345'
+  bot_owner_id: 0
   updated: 2018-12-02 15:13:12
   created: 2018-12-01 15:13:12
 - id: 15
   username: 'user15'
   password: '$2a$04$X4aRMEt0ytgPwMIgv36cI..7X9.nhY/.tYwxpqSi0ykRHx2CwQ0S6' # 12345678
   email: 'user15@example.com'
   issuer: local
+  bot_owner_id: 0
   updated: 2018-12-02 15:13:12
   created: 2018-12-01 15:13:12
 - id: 16
   username: 'user16'
   password: '$2a$04$X4aRMEt0ytgPwMIgv36cI..7X9.nhY/.tYwxpqSi0ykRHx2CwQ0S6' # 12345678
   email: 'user16@example.com'
   issuer: local
+  bot_owner_id: 0
   default_project_id: 37
   updated: 2018-12-02 15:13:12
   created: 2018-12-01 15:13:12
@@ -134,6 +153,7 @@
   email: 'user17@example.com'
   status: 2
   issuer: local
+  bot_owner_id: 0
   updated: 2018-12-02 15:13:12
   created: 2018-12-01 15:13:12
 # Locked user for security tests
@@ -143,9 +163,13 @@
   email: 'user18@example.com'
   status: 3
   issuer: local
+  bot_owner_id: 0
   updated: 2018-12-02 15:13:12
   created: 2018-12-01 15:13:12
 # User scheduled for deletion — used by admin reassign-owner guard tests.
+# The only human left without bot_owner_id, i.e. NULL: migration 20260405194817
+# adds the column without a backfill, so every account predating it looks like
+# this in production. Keeps the IsNull branch of the user-search bot filter live.
 - id: 20
   username: 'user20'
   password: '$2a$04$X4aRMEt0ytgPwMIgv36cI..7X9.nhY/.tYwxpqSi0ykRHx2CwQ0S6' # 12345678
@@ -161,6 +185,7 @@
   email: 'user_openid_avatar@example.com'
   issuer: 'https://some.service.com'
   subject: 'openid-avatar-test'
+  bot_owner_id: 0
   avatar_provider: 'openid'
   updated: 2018-12-02 15:13:12
   created: 2018-12-01 15:13:12
@@ -173,13 +198,15 @@
   password: '$2a$04$X4aRMEt0ytgPwMIgv36cI..7X9.nhY/.tYwxpqSi0ykRHx2CwQ0S6'
   email: 'user_bot_owner_a@example.com'
   issuer: local
+  bot_owner_id: 0
   updated: 2018-12-02 15:13:12
   created: 2018-12-01 15:13:12
 - id: 22
   username: 'user_bot_owner_b'
   password: '$2a$04$X4aRMEt0ytgPwMIgv36cI..7X9.nhY/.tYwxpqSi0ykRHx2CwQ0S6'
   email: 'user_bot_owner_b@example.com'
   issuer: local
+  bot_owner_id: 0
   updated: 2018-12-02 15:13:12
   created: 2018-12-01 15:13:12
 # Bot owned by user 21 — used to assert own bots are always returned
@@ -199,3 +226,10 @@
   bot_owner_id: 22
   updated: 2018-12-02 15:13:12
   created: 2018-12-01 15:13:12
+- id: 25
+  username: 'bot-owner-a-scheduler'
+  name: 'Owner A Scheduler'
+  issuer: local
+  bot_owner_id: 21
+  updated: 2018-12-02 15:13:12
+  created: 2018-12-01 15:13:12

pkg/models/api_tokens.go

@@ -112,12 +112,11 @@ func (t *APIToken) Create(s *xorm.Session, a web.Auth) (err error) {
 	if t.OwnerID == 0 {
 		t.OwnerID = caller.ID
 	} else if t.OwnerID != caller.ID {
-		// If OwnerID is set to someone else, verify it's a bot owned by the caller.
 		botUser, err := user.GetUserByID(s, t.OwnerID)
 		if err != nil {
 			return err
 		}
-		if !botUser.IsBot() || botUser.BotOwnerID != caller.ID {
+		if !botUser.IsBotOwnedBy(caller) {
 			return &user.ErrBotNotOwned{UserID: t.OwnerID}
 		}
 	}
@@ -169,12 +168,11 @@ func (t *APIToken) ReadAll(s *xorm.Session, a web.Auth, search string, page int,
 
 	ownerID := caller.ID
 	if t.OwnerID != 0 && t.OwnerID != caller.ID {
-		// If filtering by a different owner, verify it's a bot owned by the caller.
 		botUser, lookupErr := user.GetUserByID(s, t.OwnerID)
 		if lookupErr != nil {
 			return nil, 0, 0, lookupErr
 		}
-		if !botUser.IsBot() || botUser.BotOwnerID != caller.ID {
+		if !botUser.IsBotOwnedBy(caller) {
 			return nil, 0, 0, &user.ErrBotNotOwned{UserID: t.OwnerID}
 		}
 		ownerID = t.OwnerID

pkg/models/api_tokens_permissions.go

@@ -38,15 +38,14 @@ func (t *APIToken) CanDelete(s *xorm.Session, a web.Auth) (bool, error) {
 		return true, nil
 	}
 
-	// Allow deletion if the token belongs to a bot owned by the caller.
 	botUser, err := user.GetUserByID(s, token.OwnerID)
 	if err != nil {
 		if user.IsErrUserDoesNotExist(err) {
 			return false, nil
 		}
 		return false, err
 	}
-	if botUser.IsBot() && botUser.BotOwnerID == caller.ID {
+	if botUser.IsBotOwnedBy(caller) {
 		*t = *token
 		return true, nil
 	}

pkg/models/bot_users_permissions.go

@@ -45,9 +45,15 @@ func (b *BotUser) CanUpdate(s *xorm.Session, a web.Auth) (bool, error) { return
 func (b *BotUser) CanDelete(s *xorm.Session, a web.Auth) (bool, error) { return b.isOwner(s, a) }
 
 func (b *BotUser) isOwner(s *xorm.Session, a web.Auth) (bool, error) {
-	if _, is := a.(*LinkSharing); is {
+	// A link share is not a user and can never own a bot: a plain denial, same
+	// shape as any other non-owner, not an error.
+	caller, err := user.GetFromAuth(a)
+	if user.IsErrMustNotBeLinkShare(err) {
 		return false, nil
 	}
+	if err != nil {
+		return false, err
+	}
 
 	u, err := user.GetUserByID(s, b.ID)
 	if err != nil {
@@ -56,5 +62,5 @@ func (b *BotUser) isOwner(s *xorm.Session, a web.Auth) (bool, error) {
 		}
 		return false, err
 	}
-	return u.BotOwnerID == a.GetID(), nil
+	return u.IsBotOwnedBy(caller), nil
 }

pkg/models/label_permissions.go

@@ -50,41 +50,32 @@ func (l *Label) CanCreate(_ *xorm.Session, a web.Auth) (bool, error) {
 
 func (l *Label) isLabelOwner(s *xorm.Session, a web.Auth) (bool, error) {
 
-	if _, is := a.(*LinkSharing); is {
+	// Link shares legitimately reach here through hasAccessToLabel, and are not
+	// users: a plain denial, not an error.
+	caller, err := user.GetFromAuth(a)
+	if user.IsErrMustNotBeLinkShare(err) {
 		return false, nil
 	}
+	if err != nil {
+		return false, err
+	}
 
 	lorig, err := getLabelByIDSimple(s, l.ID)
 	if err != nil {
 		return false, err
 	}
-	if lorig.CreatedByID == a.GetID() {
+	if lorig.CreatedByID == caller.ID {
 		return true, nil
 	}
 
-	// A bot owner inherits write/delete access to labels their bots created.
 	creator, err := user.GetUserByID(s, lorig.CreatedByID)
 	if err != nil {
 		if user.IsErrUserDoesNotExist(err) {
 			return false, nil
 		}
 		return false, err
 	}
-	return creator.IsBot() && creator.BotOwnerID == a.GetID(), nil
-}
-
-// Matches labels created by a bot the caller owns, or - when the caller is a
-// bot - by the human who owns it (#3592).
-func labelCreatedByBotIdentityCond(a web.Auth) builder.Cond {
-	return builder.Or(
-		builder.In("labels.created_by_id",
-			builder.Select("id").From("users").Where(builder.Eq{"bot_owner_id": a.GetID()}),
-		),
-		builder.In("labels.created_by_id",
-			builder.Select("bot_owner_id").From("users").
-				Where(builder.And(builder.Eq{"id": a.GetID()}, builder.Gt{"bot_owner_id": 0})),
-		),
-	)
+	return creator.IsBotOwnedBy(caller), nil
 }
 
 // hasAccessToLabel reports whether the caller can read a label and, if so,
@@ -112,10 +103,11 @@ func (l *Label) hasAccessToLabel(s *xorm.Session, a web.Auth) (has bool, maxPerm
 
 	accessBranches := []builder.Cond{labelAttachedToAccessibleTask}
 	if !isLinkShare {
-		accessBranches = append(accessBranches,
-			builder.Eq{"labels.created_by_id": a.GetID()},
-			labelCreatedByBotIdentityCond(a),
-		)
+		caller, err := user.GetFromAuth(a)
+		if err != nil {
+			return false, 0, err
+		}
+		accessBranches = append(accessBranches, user.SameBotIdentityCond(caller, "labels.created_by_id"))
 	}
 
 	cond := builder.And(

pkg/models/label_task.go

@@ -230,10 +230,11 @@ func GetLabelsByTaskIDs(s *xorm.Session, opts *LabelByTaskIDsOptions) (ls []*Lab
 		), cond)
 	}
 	if opts.GetUnusedLabels && !isLinkShareAuth {
-		cond = builder.Or(cond,
-			builder.Eq{"labels.created_by_id": opts.User.GetID()},
-			labelCreatedByBotIdentityCond(opts.User),
-		)
+		caller, err := user.GetFromAuth(opts.User)
+		if err != nil {
+			return nil, 0, 0, err
+		}
+		cond = builder.Or(cond, user.SameBotIdentityCond(caller, "labels.created_by_id"))
 	}
 
 	ids := []int64{}

pkg/models/label_task_test.go

@@ -259,7 +259,7 @@ func TestLabelTask_Create(t *testing.T) {
 				LabelID: 11,
 			},
 			args: args{
-				a: &user.User{ID: 23, BotOwnerID: 21},
+				a: &user.User{ID: 23},
 			},
 		},
 		{
@@ -271,7 +271,7 @@ func TestLabelTask_Create(t *testing.T) {
 				LabelID: 6,
 			},
 			args: args{
-				a: &user.User{ID: 23, BotOwnerID: 21},
+				a: &user.User{ID: 23},
 			},
 			wantForbidden: true,
 		},
@@ -284,10 +284,20 @@ func TestLabelTask_Create(t *testing.T) {
 				LabelID: 11,
 			},
 			args: args{
-				a: &user.User{ID: 23, BotOwnerID: 21},
+				a: &user.User{ID: 23},
 			},
 			wantForbidden: true,
 		},
+		{
+			name: "bot can attach a label created by a sibling bot",
+			fields: fields{
+				TaskID:  52,
+				LabelID: 12,
+			},
+			args: args{
+				a: &user.User{ID: 23},
+			},
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {