Skip to content
Closed
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion management/server/account.go
Original file line number Diff line number Diff line change
Expand Up @@ -1403,9 +1403,21 @@ func (am *DefaultAccountManager) SyncUserJWTGroups(ctx context.Context, userAuth
}

addNewGroups = util.Difference(updatedAutoGroups, user.AutoGroups)
for _, group := range addNewGroups {
err = transaction.AddUserToGroup(ctx, userAuth.AccountId, user.Id, group)
if err != nil {
return fmt.Errorf("error adding user to group: %w", err)
}
}
removeOldGroups = util.Difference(user.AutoGroups, updatedAutoGroups)

for _, group := range removeOldGroups {
err = transaction.RemoveUserFromGroup(ctx, user.Id, group)
if err != nil {
return fmt.Errorf("error removing user from group: %w", err)
}
}
user.AutoGroups = updatedAutoGroups

if err = transaction.SaveUser(ctx, user); err != nil {
return fmt.Errorf("error saving user: %w", err)
}
Expand Down
14 changes: 14 additions & 0 deletions management/server/account_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1723,6 +1723,13 @@ func TestAccount_Copy(t *testing.T) {
Id: "user1",
Role: types.UserRoleAdmin,
AutoGroups: []string{"group1"},
Groups: []*types.GroupUser{
{
AccountID: "account1",
UserID: "user1",
GroupID: "group1",
},
},
PATs: map[string]*types.PersonalAccessToken{
"pat1": {
ID: "pat1",
Expand All @@ -1742,6 +1749,13 @@ func TestAccount_Copy(t *testing.T) {
Peers: []string{"peer1"},
Resources: []types.Resource{},
GroupPeers: []types.GroupPeer{},
GroupUsers: []types.GroupUser{
{
AccountID: "account1",
UserID: "user1",
GroupID: "group1",
},
},
},
},
Policies: []*types.Policy{
Expand Down
25 changes: 18 additions & 7 deletions management/server/group_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -344,22 +344,22 @@
Peers: make([]string, 0),
}

routeResource := &route.Route{

Check failure on line 347 in management/server/group_test.go

View workflow job for this annotation

GitHub Actions / Darwin

assigned to routeResource, but never used afterwards (wastedassign)

Check failure on line 347 in management/server/group_test.go

View workflow job for this annotation

GitHub Actions / Linux

assigned to routeResource, but never used afterwards (wastedassign)

Check failure on line 347 in management/server/group_test.go

View workflow job for this annotation

GitHub Actions / Windows

assigned to routeResource, but never used afterwards (wastedassign)
ID: "example route",
Groups: []string{groupForRoute.ID},
}

routePeerGroupResource := &route.Route{

Check failure on line 352 in management/server/group_test.go

View workflow job for this annotation

GitHub Actions / Darwin

assigned to routePeerGroupResource, but never used afterwards (wastedassign)

Check failure on line 352 in management/server/group_test.go

View workflow job for this annotation

GitHub Actions / Linux

assigned to routePeerGroupResource, but never used afterwards (wastedassign)

Check failure on line 352 in management/server/group_test.go

View workflow job for this annotation

GitHub Actions / Windows

assigned to routePeerGroupResource, but never used afterwards (wastedassign)
ID: "example route with peer groups",
PeerGroups: []string{groupForRoute2.ID},
}

nameServerGroup := &nbdns.NameServerGroup{

Check failure on line 357 in management/server/group_test.go

View workflow job for this annotation

GitHub Actions / Darwin

assigned to nameServerGroup, but never used afterwards (wastedassign)

Check failure on line 357 in management/server/group_test.go

View workflow job for this annotation

GitHub Actions / Linux

assigned to nameServerGroup, but never used afterwards (wastedassign)

Check failure on line 357 in management/server/group_test.go

View workflow job for this annotation

GitHub Actions / Windows

assigned to nameServerGroup, but never used afterwards (wastedassign)
ID: "example name server group",
Groups: []string{groupForNameServerGroups.ID},
}

policy := &types.Policy{

Check failure on line 362 in management/server/group_test.go

View workflow job for this annotation

GitHub Actions / Darwin

ineffectual assignment to policy (ineffassign)

Check failure on line 362 in management/server/group_test.go

View workflow job for this annotation

GitHub Actions / Linux

ineffectual assignment to policy (ineffassign)

Check failure on line 362 in management/server/group_test.go

View workflow job for this annotation

GitHub Actions / Windows

ineffectual assignment to policy (ineffassign)
ID: "example policy",
Rules: []*types.PolicyRule{
{
Expand All @@ -369,24 +369,17 @@
},
}

setupKey := &types.SetupKey{

Check failure on line 372 in management/server/group_test.go

View workflow job for this annotation

GitHub Actions / Darwin

assigned to setupKey, but never used afterwards (wastedassign)

Check failure on line 372 in management/server/group_test.go

View workflow job for this annotation

GitHub Actions / Linux

assigned to setupKey, but never used afterwards (wastedassign)

Check failure on line 372 in management/server/group_test.go

View workflow job for this annotation

GitHub Actions / Windows

assigned to setupKey, but never used afterwards (wastedassign)
Id: "example setup key",
AutoGroups: []string{groupForSetupKeys.ID},
UpdatedAt: time.Now(),
}

user := &types.User{

Check failure on line 378 in management/server/group_test.go

View workflow job for this annotation

GitHub Actions / Darwin

assigned to user, but never used afterwards (wastedassign)

Check failure on line 378 in management/server/group_test.go

View workflow job for this annotation

GitHub Actions / Linux

assigned to user, but never used afterwards (wastedassign)

Check failure on line 378 in management/server/group_test.go

View workflow job for this annotation

GitHub Actions / Windows

assigned to user, but never used afterwards (wastedassign)
Id: "example user",
AutoGroups: []string{groupForUsers.ID},
}
account := newAccountWithId(context.Background(), accountID, groupAdminUserID, domain, "", "", false)
account.Routes[routeResource.ID] = routeResource
account.Routes[routePeerGroupResource.ID] = routePeerGroupResource
account.NameServerGroups[nameServerGroup.ID] = nameServerGroup
account.Policies = append(account.Policies, policy)
account.SetupKeys[setupKey.Id] = setupKey
account.Users[user.Id] = user

err := am.Store.SaveAccount(context.Background(), account)
if err != nil {
return nil, nil, err
Expand All @@ -400,6 +393,24 @@
_ = am.CreateGroup(context.Background(), accountID, groupAdminUserID, groupForUsers)
_ = am.CreateGroup(context.Background(), accountID, groupAdminUserID, groupForIntegration)

account, err = am.Store.GetAccount(context.Background(), accountID)
if err != nil {
return nil, nil, err
}
return am, account, nil

account.Routes[routeResource.ID] = routeResource

Check failure on line 402 in management/server/group_test.go

View workflow job for this annotation

GitHub Actions / Darwin

unreachable: unreachable code (govet)

Check failure on line 402 in management/server/group_test.go

View workflow job for this annotation

GitHub Actions / Linux

unreachable: unreachable code (govet)

Check failure on line 402 in management/server/group_test.go

View workflow job for this annotation

GitHub Actions / Windows

unreachable: unreachable code (govet)
account.Routes[routePeerGroupResource.ID] = routePeerGroupResource
account.NameServerGroups[nameServerGroup.ID] = nameServerGroup
account.Policies = append(account.Policies, policy)
account.SetupKeys[setupKey.Id] = setupKey
account.Users[user.Id] = user

err = am.Store.SaveAccount(context.Background(), account)
if err != nil {
return nil, nil, err
}

acc, err := am.Store.GetAccount(context.Background(), account.Id)
if err != nil {
return nil, nil, err
Expand Down
100 changes: 100 additions & 0 deletions management/server/migration/migration.go
Original file line number Diff line number Diff line change
Expand Up @@ -487,3 +487,103 @@ func MigrateJsonToTable[T any](ctx context.Context, db *gorm.DB, columnName stri
log.WithContext(ctx).Infof("Migration of JSON field %s from table %s into separate table completed", columnName, tableName)
return nil
}

// CleanupOrphanedIDs removes non-existent IDs from the JSON array column.
// T is the type of the model that contains the list.
// This migration cleans up the lists field by removing IDs that no longer exist in the target table.
func CleanupOrphanedIDs[T, S any](ctx context.Context, db *gorm.DB, columnName string) error {
var sourceModel T
var fkModel S

if !db.Migrator().HasTable(&sourceModel) {
log.WithContext(ctx).Debugf("Table for %T does not exist, no migration needed", sourceModel)
return nil
}

if !db.Migrator().HasTable(&fkModel) {
log.WithContext(ctx).Debugf("Table for %T does not exist, no migration needed", fkModel)
return nil
}

stmt := &gorm.Statement{DB: db}
err := stmt.Parse(&sourceModel)
if err != nil {
return fmt.Errorf("parse model: %w", err)
}
tableName := stmt.Schema.Table

if !db.Migrator().HasColumn(&sourceModel, columnName) {
log.WithContext(ctx).Debugf("Column %s does not exist in table %s, no migration needed", columnName, tableName)
return nil
}

if err := db.Transaction(func(tx *gorm.DB) error {
var rows []map[string]any
if err := tx.Table(tableName).Select("id", columnName).Find(&rows).Error; err != nil {
return fmt.Errorf("find rows: %w", err)
}

// Get all valid IDs from the fk table
var validIDs []string
if err := tx.Model(fkModel).Select("id").Pluck("id", &validIDs).Error; err != nil {
return fmt.Errorf("fetch valid group IDs: %w", err)
}

validIDMap := make(map[string]bool, len(validIDs))
for _, id := range validIDs {
validIDMap[id] = true
}

updatedCount := 0
for _, row := range rows {
jsonValue, ok := row[columnName].(string)
if !ok || jsonValue == "" || jsonValue == "null" {
continue
}

var list []string
if err := json.Unmarshal([]byte(jsonValue), &list); err != nil {
log.WithContext(ctx).Warnf("Failed to unmarshal %s for id %v: %v", columnName, row["id"], err)
continue
}

if len(list) == 0 {
continue
}

// Filter out non-existent IDs
cleanedList := make([]string, 0, len(list))
for _, groupID := range list {
if validIDMap[groupID] {
cleanedList = append(cleanedList, groupID)
}
}

// Only update if there were orphaned ids removed
if len(cleanedList) != len(list) {
cleanedJSON, err := json.Marshal(cleanedList)
if err != nil {
return fmt.Errorf("marshal cleaned %s: %w", columnName, err)
}

if err := tx.Table(tableName).Where("id = ?", row["id"]).Update(columnName, cleanedJSON).Error; err != nil {
return fmt.Errorf("update row with id %v: %w", row["id"], err)
}
updatedCount++
}
}

if updatedCount > 0 {
log.WithContext(ctx).Infof("Cleaned up orphaned %s in %d rows from table %s", columnName, updatedCount, tableName)
} else {
log.WithContext(ctx).Debugf("No orphaned %s found in table %s", columnName, tableName)
}

return nil
}); err != nil {
return err
}

log.WithContext(ctx).Infof("Cleanup of orphaned auto_groups from table %s completed", tableName)
return nil
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
}
Loading
Loading