From 34477f5aa3866032c0ac3c795476f856e8f2c81b Mon Sep 17 00:00:00 2001 From: Jay Hemnani <193022578+jayhemnani9910@users.noreply.github.com> Date: Sun, 11 Oct 2026 18:49:40 +0530 Subject: [PATCH] fix: push users disabled by the LDAP sync to SCIM (#1834) --- backend/internal/ldapsync/service_test.go | 42 +++++++++++++++++++++++ backend/internal/service/user_service.go | 8 +++-- 2 files changed, 48 insertions(+), 2 deletions(-) diff --git a/backend/internal/ldapsync/service_test.go b/backend/internal/ldapsync/service_test.go index ed1a36b0..638faed5 100644 --- a/backend/internal/ldapsync/service_test.go +++ b/backend/internal/ldapsync/service_test.go @@ -5,6 +5,7 @@ import ( "net/http" "net/http/httptest" "testing" + "time" "github.com/go-ldap/ldap/v3" "github.com/stretchr/testify/assert" @@ -14,6 +15,7 @@ import ( "github.com/pocket-id/pocket-id/backend/internal/appconfig" "github.com/pocket-id/pocket-id/backend/internal/apperror" "github.com/pocket-id/pocket-id/backend/internal/model" + datatype "github.com/pocket-id/pocket-id/backend/internal/model/types" "github.com/pocket-id/pocket-id/backend/internal/service" "github.com/pocket-id/pocket-id/backend/internal/storage" testutils "github.com/pocket-id/pocket-id/backend/internal/utils/testing" @@ -359,6 +361,46 @@ func TestLdapServiceSyncAllHandlesDuplicateLDAPIDsInSingleRun(t *testing.T) { assert.ElementsMatch(t, []string{"alice"}, usernames(groups[0].Users)) } +func TestLdapServiceSyncAllBumpsUpdatedAtOfSoftDeletedUsers(t *testing.T) { + service, db := newTestLdapService(t, newFakeLDAPClient(ldapSearchResult(), ldapSearchResult())) + + lastSync := datatype.DateTime(time.Now().Add(-time.Hour)) + goneLdapID := "u-gone" + disabledLdapID := "u-disabled" + + require.NoError(t, db.Create(&model.User{ + Username: "gone", + Email: new("gone@example.com"), + LdapID: &goneLdapID, + UpdatedAt: &lastSync, + }).Error) + + require.NoError(t, db.Create(&model.User{ + Username: "disabled", + Email: new("disabled@example.com"), + LdapID: &disabledLdapID, + Disabled: true, + UpdatedAt: &lastSync, + }).Error) + + err := service.SyncAll(t.Context(), defaultTestLDAPAppConfig()) + require.NoError(t, err) + + // The SCIM sync only pushes users whose UpdatedAt is newer than the remote copy, so disabling a user must bump it + var gone model.User + require.NoError(t, db.First(&gone, "ldap_id = ?", goneLdapID).Error) + assert.True(t, gone.Disabled) + require.NotNil(t, gone.UpdatedAt) + assert.True(t, gone.UpdatedAt.ToTime().After(lastSync.ToTime())) + + // A user that an earlier sync already disabled is unchanged and keeps its timestamp + var disabled model.User + require.NoError(t, db.First(&disabled, "ldap_id = ?", disabledLdapID).Error) + assert.True(t, disabled.Disabled) + require.NotNil(t, disabled.UpdatedAt) + assert.Equal(t, lastSync.ToTime().Unix(), disabled.UpdatedAt.ToTime().Unix()) +} + func TestLdapServiceSyncAllSetsAdminFromGroupMembership(t *testing.T) { tests := []struct { name string diff --git a/backend/internal/service/user_service.go b/backend/internal/service/user_service.go index 2b0c1f97..6222f2e6 100644 --- a/backend/internal/service/user_service.go +++ b/backend/internal/service/user_service.go @@ -723,11 +723,15 @@ func (s *UserService) ResetProfilePicture(ctx context.Context, userID string) er // DisableUserInternal disables a user within an existing transaction // It's exported for the LDAP sync, which soft-deletes users that are no longer in the directory func (s *UserService) DisableUserInternal(ctx context.Context, tx *gorm.DB, userID string) error { + // Bump UpdatedAt so the SCIM sync pushes the disabled state, but only when the user was still enabled err := tx. WithContext(ctx). Model(&model.User{}). - Where("id = ?", userID). - Update("disabled", true). + Where("id = ? AND disabled = ?", userID, false). + Updates(map[string]any{ + "disabled": true, + "updated_at": datatype.DateTime(time.Now()), + }). Error if err != nil {