From 563c0f93a6d6473f0ff315cfd0bedbf57ba78f05 Mon Sep 17 00:00:00 2001 From: "Alessandro (Ale) Segala" <43508+ItalyPaleAle@users.noreply.github.com> Date: Fri, 7 Aug 2026 09:18:30 -0700 Subject: [PATCH] refactor: migrate LDAP sync to an actor (#1651) Co-authored-by: Kyle Mendell --- backend/internal/appconfig/resolver.go | 10 + .../bootstrap/e2etest_router_bootstrap.go | 2 +- .../internal/bootstrap/router_bootstrap.go | 3 +- .../internal/bootstrap/scheduler_bootstrap.go | 6 +- .../internal/bootstrap/services_bootstrap.go | 19 +- .../controller/app_config_controller.go | 26 --- backend/internal/devicelogin/handler.go | 5 +- backend/internal/devicelogin/module.go | 6 +- backend/internal/emailverification/handler.go | 5 +- backend/internal/emailverification/module.go | 7 +- backend/internal/job/ldap_job.go | 35 --- backend/internal/ldapsync/actor.go | 130 +++++++++++ backend/internal/ldapsync/actor_test.go | 217 ++++++++++++++++++ backend/internal/ldapsync/handler.go | 41 ++++ backend/internal/ldapsync/module.go | 84 +++++++ .../ldap_service.go => ldapsync/service.go} | 72 +++--- .../service_test.go} | 40 ++-- backend/internal/onetimeaccess/handler.go | 5 +- backend/internal/onetimeaccess/module.go | 7 +- backend/internal/service/e2etest_service.go | 14 +- .../internal/service/user_group_service.go | 24 +- backend/internal/service/user_service.go | 17 +- backend/internal/usersignup/handler.go | 5 +- backend/internal/usersignup/module.go | 7 +- backend/internal/utils/testing/actors.go | 4 +- backend/internal/webauthn/handler.go | 10 +- backend/internal/webauthn/module.go | 7 +- 27 files changed, 627 insertions(+), 181 deletions(-) create mode 100644 backend/internal/appconfig/resolver.go delete mode 100644 backend/internal/job/ldap_job.go create mode 100644 backend/internal/ldapsync/actor.go create mode 100644 backend/internal/ldapsync/actor_test.go create mode 100644 backend/internal/ldapsync/handler.go create mode 100644 backend/internal/ldapsync/module.go rename backend/internal/{service/ldap_service.go => ldapsync/service.go} (86%) rename backend/internal/{service/ldap_service_test.go => ldapsync/service_test.go} (94%) diff --git a/backend/internal/appconfig/resolver.go b/backend/internal/appconfig/resolver.go new file mode 100644 index 00000000..6ac5d83b --- /dev/null +++ b/backend/internal/appconfig/resolver.go @@ -0,0 +1,10 @@ +package appconfig + +import ( + "context" +) + +// AppConfigResolver loads the current application configuration, so handlers can pass it explicitly to the service methods that need it +type AppConfigResolver interface { + GetConfig(ctx context.Context) (*AppConfigModel, error) +} diff --git a/backend/internal/bootstrap/e2etest_router_bootstrap.go b/backend/internal/bootstrap/e2etest_router_bootstrap.go index d5c99d1c..df6d3558 100644 --- a/backend/internal/bootstrap/e2etest_router_bootstrap.go +++ b/backend/internal/bootstrap/e2etest_router_bootstrap.go @@ -17,7 +17,7 @@ import ( func init() { registerTestControllers = []func(apiGroup *gin.RouterGroup, db *gorm.DB, svc *services){ func(apiGroup *gin.RouterGroup, db *gorm.DB, svc *services) { - testService, err := service.NewTestService(db, svc.actors, svc.appConfigService, svc.jwtService, svc.ldapService, svc.fileStorage) + testService, err := service.NewTestService(db, svc.actors, svc.appConfigService, svc.jwtService, svc.ldapSyncModule, svc.fileStorage) if err != nil { slog.Error("Failed to initialize test service", slog.Any("error", err)) os.Exit(1) diff --git a/backend/internal/bootstrap/router_bootstrap.go b/backend/internal/bootstrap/router_bootstrap.go index 8e938b12..bd9ee767 100644 --- a/backend/internal/bootstrap/router_bootstrap.go +++ b/backend/internal/bootstrap/router_bootstrap.go @@ -169,7 +169,8 @@ func registerRoutes(r *gin.Engine, db *gorm.DB, svc *services, rateLimitServices ) controller.NewOidcController(apiGroup, authMiddleware, fileSizeLimitMiddleware, svc.oidcService) controller.NewUserController(apiGroup, authMiddleware, svc.appConfigService, svc.userService, svc.webauthnModule) - controller.NewAppConfigController(apiGroup, authMiddleware, svc.appConfigService, svc.emailModule, svc.ldapService) + controller.NewAppConfigController(apiGroup, authMiddleware, svc.appConfigService, svc.emailModule) + svc.ldapSyncModule.RegisterRoutes(apiGroup, authMiddleware.Add()) controller.NewAppImagesController(apiGroup, authMiddleware, svc.appImagesService) controller.NewAuditLogController(apiGroup, svc.auditLogService, authMiddleware) controller.NewUserGroupController(apiGroup, authMiddleware, svc.appConfigService, svc.userGroupService) diff --git a/backend/internal/bootstrap/scheduler_bootstrap.go b/backend/internal/bootstrap/scheduler_bootstrap.go index d3f3b0ae..81b6bf88 100644 --- a/backend/internal/bootstrap/scheduler_bootstrap.go +++ b/backend/internal/bootstrap/scheduler_bootstrap.go @@ -10,11 +10,7 @@ import ( ) func registerScheduledJobs(ctx context.Context, db *gorm.DB, svc *services, scheduler *job.Scheduler) error { - err := scheduler.RegisterLdapJobs(ctx, svc.ldapService, svc.appConfigService) - if err != nil { - return fmt.Errorf("failed to register LDAP jobs in scheduler: %w", err) - } - err = scheduler.RegisterDbCleanupJobs(ctx, db) + err := scheduler.RegisterDbCleanupJobs(ctx, db) if err != nil { return fmt.Errorf("failed to register DB cleanup jobs in scheduler: %w", err) } diff --git a/backend/internal/bootstrap/services_bootstrap.go b/backend/internal/bootstrap/services_bootstrap.go index 6a990154..cc60d297 100644 --- a/backend/internal/bootstrap/services_bootstrap.go +++ b/backend/internal/bootstrap/services_bootstrap.go @@ -15,6 +15,7 @@ import ( "github.com/pocket-id/pocket-id/backend/internal/emailverification" "github.com/pocket-id/pocket-id/backend/internal/geolite" "github.com/pocket-id/pocket-id/backend/internal/job" + "github.com/pocket-id/pocket-id/backend/internal/ldapsync" "github.com/pocket-id/pocket-id/backend/internal/oidc" "github.com/pocket-id/pocket-id/backend/internal/onetimeaccess" "github.com/pocket-id/pocket-id/backend/internal/service" @@ -36,12 +37,12 @@ type services struct { customClaimService *service.CustomClaimService oidcService *service.OidcService userGroupService *service.UserGroupService - ldapService *service.LdapService versionService *service.VersionService fileStorage storage.FileStorage apiKeyModule *apikey.Module deviceLoginModule *devicelogin.Module + ldapSyncModule *ldapsync.Module oidcModule *oidc.Module webauthnModule *webauthn.Module userSignUpModule *usersignup.Module @@ -152,7 +153,21 @@ func initServices( svc.userGroupService = service.NewUserGroupService(db, svc.scimService) svc.userService = service.NewUserService(db, svc.jwtService, svc.auditLogService, svc.customClaimService, svc.appImagesService, svc.scimService, fileStorage) - svc.ldapService = service.NewLdapService(db, httpClient, svc.userService, svc.userGroupService, fileStorage) + + svc.ldapSyncModule, err = ldapsync.New(ldapsync.Dependencies{ + DB: db, + Actors: actors, + HTTPClient: httpClient, + FileStorage: fileStorage, + Users: svc.userService, + Groups: svc.userGroupService, + AppConfig: svc.appConfigService, + // Disable in test environment + ScheduleDisabled: common.EnvConfig.AppEnv.IsTest(), + }) + if err != nil { + return nil, fmt.Errorf("failed to create LDAP sync module: %w", err) + } svc.apiKeyModule, err = apikey.New(ctx, apikey.Dependencies{ DB: db, diff --git a/backend/internal/controller/app_config_controller.go b/backend/internal/controller/app_config_controller.go index 1ca995e5..07c675e9 100644 --- a/backend/internal/controller/app_config_controller.go +++ b/backend/internal/controller/app_config_controller.go @@ -11,7 +11,6 @@ import ( "github.com/pocket-id/pocket-id/backend/internal/dto" "github.com/pocket-id/pocket-id/backend/internal/httpserver" "github.com/pocket-id/pocket-id/backend/internal/middleware" - "github.com/pocket-id/pocket-id/backend/internal/service" "github.com/pocket-id/pocket-id/backend/internal/tracing" ) @@ -28,26 +27,22 @@ func NewAppConfigController( authMiddleware *middleware.AuthMiddleware, appConfigService *appconfig.AppConfigService, emailSender TestEmailSender, - ldapService *service.LdapService, ) { acc := &AppConfigController{ appConfigService: appConfigService, emailSender: emailSender, - ldapService: ldapService, } group.GET("/application-configuration", httpserver.Handle(acc.listAppConfigHandler)) group.GET("/application-configuration/all", authMiddleware.Add(), httpserver.Handle(acc.listAllAppConfigHandler)) group.PUT("/application-configuration", authMiddleware.Add(), httpserver.Handle(acc.updateAppConfigHandler)) group.POST("/application-configuration/test-email", authMiddleware.Add(), httpserver.Handle(acc.testEmailHandler)) - group.POST("/application-configuration/sync-ldap", authMiddleware.Add(), httpserver.Handle(acc.syncLdapHandler)) } type AppConfigController struct { appConfigService *appconfig.AppConfigService emailSender TestEmailSender - ldapService *service.LdapService } // listAppConfigHandler godoc @@ -140,27 +135,6 @@ func (acc *AppConfigController) updateAppConfigHandler(c *gin.Context) error { return nil } -// syncLdapHandler godoc -// @Summary Synchronize LDAP -// @Description Manually trigger LDAP synchronization -// @Tags Application Configuration -// @Success 204 "No Content" -// @Router /api/application-configuration/sync-ldap [post] -func (acc *AppConfigController) syncLdapHandler(c *gin.Context) error { - dbConfig, err := acc.appConfigService.GetConfig(c.Request.Context()) - if err != nil { - return err - } - - err = acc.ldapService.SyncAll(c.Request.Context(), dbConfig) - if err != nil { - return err - } - - c.Status(http.StatusNoContent) - return nil -} - // testEmailHandler godoc // @Summary Send test email // @Description Send a test email to verify email configuration diff --git a/backend/internal/devicelogin/handler.go b/backend/internal/devicelogin/handler.go index d6c70d0b..4cc677a2 100644 --- a/backend/internal/devicelogin/handler.go +++ b/backend/internal/devicelogin/handler.go @@ -7,6 +7,7 @@ import ( "github.com/gin-gonic/gin" + "github.com/pocket-id/pocket-id/backend/internal/appconfig" "github.com/pocket-id/pocket-id/backend/internal/dto" "github.com/pocket-id/pocket-id/backend/internal/httpserver" "github.com/pocket-id/pocket-id/backend/internal/utils/cookie" @@ -15,10 +16,10 @@ import ( type handler struct { service *Service baseURL string - appConfig AppConfigProvider + appConfig appconfig.AppConfigResolver } -func newHandler(service *Service, baseURL string, appConfig AppConfigProvider) *handler { +func newHandler(service *Service, baseURL string, appConfig appconfig.AppConfigResolver) *handler { return &handler{ service: service, baseURL: baseURL, diff --git a/backend/internal/devicelogin/module.go b/backend/internal/devicelogin/module.go index 5ffc6ad4..6ef3cea9 100644 --- a/backend/internal/devicelogin/module.go +++ b/backend/internal/devicelogin/module.go @@ -31,10 +31,6 @@ type IPLocationResolver interface { GetLocationByIP(ctx context.Context, ipAddress string) (country string, city string, err error) } -type AppConfigProvider interface { - GetConfig(ctx context.Context) (*appconfig.AppConfigModel, error) -} - type Dependencies struct { DB *gorm.DB Actors *local.Host @@ -44,7 +40,7 @@ type Dependencies struct { Reauth ReauthenticationTokenConsumer AuditLog AuditLogger IPLocator IPLocationResolver - AppConfig AppConfigProvider + AppConfig appconfig.AppConfigResolver } type Module struct { diff --git a/backend/internal/emailverification/handler.go b/backend/internal/emailverification/handler.go index d7573e3d..d30de55e 100644 --- a/backend/internal/emailverification/handler.go +++ b/backend/internal/emailverification/handler.go @@ -6,16 +6,17 @@ import ( "github.com/gin-gonic/gin" + "github.com/pocket-id/pocket-id/backend/internal/appconfig" "github.com/pocket-id/pocket-id/backend/internal/dto" "github.com/pocket-id/pocket-id/backend/internal/httpserver" ) type handler struct { service *Service - appConfig AppConfigResolver + appConfig appconfig.AppConfigResolver } -func newHandler(service *Service, appConfig AppConfigResolver) *handler { +func newHandler(service *Service, appConfig appconfig.AppConfigResolver) *handler { return &handler{service: service, appConfig: appConfig} } diff --git a/backend/internal/emailverification/module.go b/backend/internal/emailverification/module.go index 19abbd38..e2502dcf 100644 --- a/backend/internal/emailverification/module.go +++ b/backend/internal/emailverification/module.go @@ -1,7 +1,6 @@ package emailverification import ( - "context" "fmt" "github.com/gin-gonic/gin" @@ -12,17 +11,13 @@ import ( "github.com/pocket-id/pocket-id/backend/internal/httpserver" ) -type AppConfigResolver interface { - GetConfig(ctx context.Context) (*appconfig.AppConfigModel, error) -} - type Dependencies struct { DB *gorm.DB Actors *local.Host Users UserProvider EmailSender EmailSender - AppConfig AppConfigResolver + AppConfig appconfig.AppConfigResolver AppURL string } diff --git a/backend/internal/job/ldap_job.go b/backend/internal/job/ldap_job.go deleted file mode 100644 index 4c450bcb..00000000 --- a/backend/internal/job/ldap_job.go +++ /dev/null @@ -1,35 +0,0 @@ -package job - -import ( - "context" - "fmt" - "time" - - "github.com/pocket-id/pocket-id/backend/internal/appconfig" - "github.com/pocket-id/pocket-id/backend/internal/service" -) - -type LdapJobs struct { - ldapService *service.LdapService - appConfigService *appconfig.AppConfigService -} - -func (s *Scheduler) RegisterLdapJobs(ctx context.Context, ldapService *service.LdapService, appConfigService *appconfig.AppConfigService) error { - jobs := &LdapJobs{ldapService: ldapService, appConfigService: appConfigService} - - // Register the job to run every hour (with some jitter) - return s.RegisterJob(ctx, "SyncLdap", jobDefWithJitter(time.Hour), jobs.syncLdap, service.RegisterJobOpts{RunImmediately: true}) -} - -func (j *LdapJobs) syncLdap(ctx context.Context) error { - dbConfig, err := j.appConfigService.GetConfig(ctx) - if err != nil { - return fmt.Errorf("error load app config: %w", err) - } - - if !dbConfig.LdapEnabled.IsTrue() { - return nil - } - - return j.ldapService.SyncAll(ctx, dbConfig) -} diff --git a/backend/internal/ldapsync/actor.go b/backend/internal/ldapsync/actor.go new file mode 100644 index 00000000..1de774a0 --- /dev/null +++ b/backend/internal/ldapsync/actor.go @@ -0,0 +1,130 @@ +package ldapsync + +import ( + "context" + "errors" + "fmt" + "log/slog" + "time" + + "github.com/italypaleale/francis/actor" + + "github.com/pocket-id/pocket-id/backend/internal/appconfig" +) + +// The LdapSync singleton actor decides when the recurring LDAP synchronization runs. + +// SyncActorType is the actor type for the LDAP sync actor +const SyncActorType = "LdapSync" + +const ( + // alarmSync is the name of the repeating alarm that runs the synchronization + alarmSync = "sync" + + // syncInterval is how often the synchronization runs, as the ISO8601 duration the alarm repetition expects + // There's no jitter: the alarm is cluster-wide, so there are no replicas to spread apart + syncInterval = "PT1H" + + // Delay the initial sync by 5s + initialSyncDelay = 5 * time.Second + + // alarmTimeout bounds the alarm operations performed by the actor + alarmTimeout = 10 * time.Second +) + +// syncActor is the cluster-wide singleton that triggers the recurring LDAP synchronization +type syncActor struct { + log *slog.Logger + service *Service + appConfig appconfig.AppConfigResolver + // scheduleDisabled removes the alarm instead of arming it, for environments that drive syncs explicitly + scheduleDisabled bool + client actor.Client[struct{}] +} + +// NewSyncActor returns the factory that allocates the LDAP sync actor +func NewSyncActor(service *Service, appConfig appconfig.AppConfigResolver, scheduleDisabled bool) actor.Factory { + return func(actorID string, actorService *actor.Service) actor.Actor { + return &syncActor{ + log: slog.With( + slog.String("scope", "actor"), + slog.String("actorType", SyncActorType), + ), + service: service, + appConfig: appConfig, + scheduleDisabled: scheduleDisabled, + // The actor keeps no state of its own: the client is only used to manage the alarm + client: actor.NewActorClient[struct{}](SyncActorType, actorID, actorService), + } + } +} + +// Bootstrap implements actor.ActorBootstrapper +// The host drives it on every startup, routed to the single owning host, so it must stay idempotent +func (a *syncActor) Bootstrap(parentCtx context.Context, _ actor.Envelope) error { + ctx, cancel := context.WithTimeout(parentCtx, alarmTimeout) + defer cancel() + + // The schedule may have been enabled in a previous run, so make sure a leftover alarm doesn't keep firing + if a.scheduleDisabled { + err := a.client.DeleteAlarm(ctx, alarmSync) + if err != nil && !errors.Is(err, actor.ErrAlarmNotFound) { + return fmt.Errorf("error deleting the LDAP sync alarm: %w", err) + } + + return nil + } + + // Setting the alarm replaces whatever is registered, which both restores an alarm that was lost and picks up a change to the interval + // It's due right away (with a small delay) so the directory is synchronized as soon as the cluster starts, matching what the pre-actor scheduled job did + err := a.client.SetAlarm(ctx, alarmSync, actor.AlarmProperties{ + DueTime: time.Now().Add(initialSyncDelay), + Interval: syncInterval, + }) + if err != nil { + return fmt.Errorf("error setting the LDAP sync alarm: %w", err) + } + + a.log.DebugContext(parentCtx, "Registered the recurring LDAP sync alarm", slog.String("interval", syncInterval)) + + return nil +} + +// Alarm implements actor.ActorAlarm +func (a *syncActor) Alarm(ctx context.Context, name string, _ actor.Envelope) error { + if name != alarmSync { + return fmt.Errorf("unsupported alarm '%s' for the %s actor", name, SyncActorType) + } + + a.sync(ctx) + + // A failed sync never surfaces as an error: the framework would retry the occurrence and then delete the alarm once the attempts run out, which would stop the synchronization altogether + // The next occurrence comes around on its own, exactly like the pre-actor scheduled job + return nil +} + +// sync runs one synchronization, unless LDAP is disabled +// It logs failures rather than returning them, since the alarm has nowhere useful to send the error +func (a *syncActor) sync(ctx context.Context) { + dbConfig, err := a.appConfig.GetConfig(ctx) + if err != nil { + a.log.ErrorContext(ctx, "Failed to load the app configuration, skipping the LDAP sync", slog.Any("error", err)) + return + } + + if !dbConfig.LdapEnabled.IsTrue() { + a.log.DebugContext(ctx, "LDAP is disabled, skipping the sync") + return + } + + a.log.InfoContext(ctx, "Starting the LDAP sync") + start := time.Now() + + err = a.service.SyncAll(ctx, dbConfig) + if err != nil { + a.log.ErrorContext(ctx, "LDAP sync failed, will try again on the next run", slog.Duration("duration", time.Since(start)), slog.Any("error", err)) + return + } + + a.log.InfoContext(ctx, "LDAP sync completed", slog.Duration("duration", time.Since(start))) +} diff --git a/backend/internal/ldapsync/actor_test.go b/backend/internal/ldapsync/actor_test.go new file mode 100644 index 00000000..e15eb83d --- /dev/null +++ b/backend/internal/ldapsync/actor_test.go @@ -0,0 +1,217 @@ +package ldapsync + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/italypaleale/francis/actor" + "github.com/italypaleale/francis/host/local" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/pocket-id/pocket-id/backend/internal/appconfig" + "github.com/pocket-id/pocket-id/backend/internal/model" + testutils "github.com/pocket-id/pocket-id/backend/internal/utils/testing" +) + +// fakeAppConfigResolver returns a fixed application configuration +type fakeAppConfigResolver struct { + config *appconfig.AppConfigModel + err error +} + +func (f fakeAppConfigResolver) GetConfig(_ context.Context) (*appconfig.AppConfigModel, error) { + if f.err != nil { + return nil, f.err + } + + return f.config, nil +} + +func TestSyncActorBootstrapArmsRecurringAlarm(t *testing.T) { + host, act := newSyncActorForTest(t, nil, fakeAppConfigResolver{config: defaultTestLDAPAppConfig()}, false) + + require.NoError(t, act.Bootstrap(t.Context(), nil)) + + props, err := host.GetAlarm(t.Context(), SyncActorType, actor.SingletonActorID, alarmSync) + require.NoError(t, err) + assert.Equal(t, syncInterval, props.Interval) + // The first occurrence is due after the initial delay, so a restart re-syncs the directory shortly after startup + // The tolerance is tight enough to catch the delay being dropped, which would put the due time at "now" + assert.WithinDuration(t, time.Now().Add(initialSyncDelay), props.DueTime, time.Second) +} + +func TestSyncActorBootstrapIsIdempotent(t *testing.T) { + host, act := newSyncActorForTest(t, nil, fakeAppConfigResolver{config: defaultTestLDAPAppConfig()}, false) + + // Every host bootstraps the singleton, so repeating it must leave a single alarm behind rather than failing + require.NoError(t, act.Bootstrap(t.Context(), nil)) + require.NoError(t, act.Bootstrap(t.Context(), nil)) + + props, err := host.GetAlarm(t.Context(), SyncActorType, actor.SingletonActorID, alarmSync) + require.NoError(t, err) + assert.Equal(t, syncInterval, props.Interval) +} + +func TestSyncActorBootstrapRemovesAlarmWhenScheduleDisabled(t *testing.T) { + host, act := newSyncActorForTest(t, nil, fakeAppConfigResolver{config: defaultTestLDAPAppConfig()}, true) + + // Simulate an alarm left behind by a run where the schedule was still enabled + require.NoError(t, host.SetAlarm(t.Context(), SyncActorType, actor.SingletonActorID, alarmSync, actor.AlarmProperties{ + DueTime: time.Now(), + Interval: syncInterval, + })) + + require.NoError(t, act.Bootstrap(t.Context(), nil)) + + _, err := host.GetAlarm(t.Context(), SyncActorType, actor.SingletonActorID, alarmSync) + require.ErrorIs(t, err, actor.ErrAlarmNotFound) +} + +func TestSyncActorBootstrapWithScheduleDisabledAndNoAlarm(t *testing.T) { + host, act := newSyncActorForTest(t, nil, fakeAppConfigResolver{config: defaultTestLDAPAppConfig()}, true) + + // There's nothing to remove, which must not be reported as a failure + require.NoError(t, act.Bootstrap(t.Context(), nil)) + + _, err := host.GetAlarm(t.Context(), SyncActorType, actor.SingletonActorID, alarmSync) + require.ErrorIs(t, err, actor.ErrAlarmNotFound) +} + +func TestSyncActorAlarmRunsSync(t *testing.T) { + appCfg := defaultTestLDAPAppConfig() + service, db := newTestLdapService(t, newFakeLDAPClient( + ldapSearchResult( + ldapEntry("uid=alice,ou=people,dc=example,dc=com", map[string][]string{ + "entryUUID": {"u-alice"}, + "uid": {"alice"}, + "mail": {"alice@example.com"}, + "givenName": {"Alice"}, + "sn": {"Jones"}, + "displayName": {""}, + }), + ), + ldapSearchResult(), + )) + + _, act := newSyncActorForTest(t, service, fakeAppConfigResolver{config: appCfg}, false) + + require.NoError(t, act.Alarm(t.Context(), alarmSync, nil)) + + var alice model.User + require.NoError(t, db.First(&alice, "ldap_id = ?", "u-alice").Error) + assert.Equal(t, "alice", alice.Username) +} + +func TestSyncActorAlarmSkipsSyncWhenLdapIsDisabled(t *testing.T) { + service, db := newTestLdapService(t, newFakeLDAPClient( + ldapSearchResult( + ldapEntry("uid=alice,ou=people,dc=example,dc=com", map[string][]string{ + "entryUUID": {"u-alice"}, + "uid": {"alice"}, + "mail": {"alice@example.com"}, + }), + ), + ldapSearchResult(), + )) + + disabledCfg := defaultTestLDAPAppConfig() + disabledCfg.LdapEnabled = "false" + _, act := newSyncActorForTest(t, service, fakeAppConfigResolver{config: disabledCfg}, false) + + require.NoError(t, act.Alarm(t.Context(), alarmSync, nil)) + + var count int64 + require.NoError(t, db.Model(&model.User{}).Count(&count).Error) + assert.Zero(t, count) +} + +func TestSyncActorAlarmSwallowsSyncFailures(t *testing.T) { + // A failed sync must not surface as an error, or the framework would eventually delete the alarm and stop synchronizing altogether + service, _ := newTestLdapService(t, newFakeLDAPClient(ldapSearchResult(), ldapSearchResult())) + service.clientFactory = func(_ *appconfig.AppConfigModel) (ldapClient, error) { + return nil, errors.New("connection refused") + } + + _, act := newSyncActorForTest(t, service, fakeAppConfigResolver{config: defaultTestLDAPAppConfig()}, false) + + require.NoError(t, act.Alarm(t.Context(), alarmSync, nil)) +} + +func TestSyncActorAlarmSwallowsAppConfigFailures(t *testing.T) { + service, _ := newTestLdapService(t, newFakeLDAPClient(ldapSearchResult(), ldapSearchResult())) + + _, act := newSyncActorForTest(t, service, fakeAppConfigResolver{err: errors.New("config unavailable")}, false) + + require.NoError(t, act.Alarm(t.Context(), alarmSync, nil)) +} + +func TestSyncActorAlarmRejectsUnknownAlarm(t *testing.T) { + _, act := newSyncActorForTest(t, nil, fakeAppConfigResolver{config: defaultTestLDAPAppConfig()}, false) + + err := act.Alarm(t.Context(), "unknown", nil) + require.Error(t, err) + assert.ErrorContains(t, err, "unsupported alarm") +} + +func TestSyncActorRegisteredSingletonBootstrapsAndFires(t *testing.T) { + // This exercises the wiring the unit tests above bypass: the host bootstraps the singleton on its own, and the alarm it arms is delivered back to the actor + appCfg := defaultTestLDAPAppConfig() + service, db := newTestLdapService(t, newFakeLDAPClient( + ldapSearchResult( + ldapEntry("uid=alice,ou=people,dc=example,dc=com", map[string][]string{ + "entryUUID": {"u-alice"}, + "uid": {"alice"}, + "mail": {"alice@example.com"}, + "givenName": {"Alice"}, + "sn": {"Jones"}, + "displayName": {""}, + }), + ), + ldapSearchResult(), + )) + + // The host uses the same relaxed alarm intervals the application configures when HA is disabled, since those are what decide how soon the first occurrence is picked up + // Francis only performs an early first fetch when the poll interval is long, so with the default (short) test interval this test would pass even if that behavior regressed + host := testutils.NewActorHostForTest(t, + func(t *testing.T, h *local.Host) { + err := h.RegisterSingletonActor(SyncActorType, NewSyncActor(service, fakeAppConfigResolver{config: appCfg}, false)) + require.NoError(t, err) + }, + local.WithAlarmsPollInterval(5*time.Minute), + local.WithAlarmsFetchAheadInterval(5*time.Minute), + ) + + // The host bootstraps singletons in the background once it's ready, so wait for the alarm to show up + require.Eventually(t, func() bool { + _, err := host.GetAlarm(t.Context(), SyncActorType, actor.SingletonActorID, alarmSync) + return err == nil + }, 10*time.Second, 20*time.Millisecond, "the sync alarm was never armed") + + // The first occurrence runs shortly after startup rather than waiting out the poll interval, so the deadline here is far below it + require.Eventually(t, + func() bool { + var count int64 + require.NoError(t, db.Model(&model.User{}).Where("ldap_id = ?", "u-alice").Count(&count).Error) + return count == 1 + }, + initialSyncDelay+30*time.Second, + 50*time.Millisecond, + "the sync alarm never ran", + ) +} + +// newSyncActorForTest starts a test actor host and allocates the sync actor against it +// The actor is not registered on the host, so the host never bootstraps or fires it on its own and the test drives it explicitly +func newSyncActorForTest(t *testing.T, service *Service, appConfig appconfig.AppConfigResolver, scheduleDisabled bool) (*local.Host, *syncActor) { + t.Helper() + + host := testutils.NewActorHostForTest(t, nil) + + act, ok := NewSyncActor(service, appConfig, scheduleDisabled)(actor.SingletonActorID, host.Service()).(*syncActor) + require.True(t, ok) + + return host, act +} diff --git a/backend/internal/ldapsync/handler.go b/backend/internal/ldapsync/handler.go new file mode 100644 index 00000000..639978a7 --- /dev/null +++ b/backend/internal/ldapsync/handler.go @@ -0,0 +1,41 @@ +package ldapsync + +import ( + "fmt" + "net/http" + + "github.com/gin-gonic/gin" + + "github.com/pocket-id/pocket-id/backend/internal/appconfig" +) + +type handler struct { + service *Service + appConfig appconfig.AppConfigResolver +} + +func newHandler(service *Service, appConfig appconfig.AppConfigResolver) *handler { + return &handler{service: service, appConfig: appConfig} +} + +// syncLdap godoc +// @Summary Synchronize LDAP +// @Description Manually trigger LDAP synchronization +// @Tags Application Configuration +// @Success 204 "No Content" +// @Router /api/application-configuration/sync-ldap [post] +func (h *handler) syncLdap(c *gin.Context) error { + dbConfig, err := h.appConfig.GetConfig(c.Request.Context()) + if err != nil { + return fmt.Errorf("error loading app configuration: %w", err) + } + + // The sync runs inline rather than through the actor, so the response reports whether it succeeded + err = h.service.SyncAll(c.Request.Context(), dbConfig) + if err != nil { + return err + } + + c.Status(http.StatusNoContent) + return nil +} diff --git a/backend/internal/ldapsync/module.go b/backend/internal/ldapsync/module.go new file mode 100644 index 00000000..81aafb66 --- /dev/null +++ b/backend/internal/ldapsync/module.go @@ -0,0 +1,84 @@ +package ldapsync + +import ( + "context" + "fmt" + "io" + "net/http" + + "github.com/gin-gonic/gin" + "github.com/italypaleale/francis/host/local" + "gorm.io/gorm" + + "github.com/pocket-id/pocket-id/backend/internal/appconfig" + "github.com/pocket-id/pocket-id/backend/internal/dto" + "github.com/pocket-id/pocket-id/backend/internal/httpserver" + "github.com/pocket-id/pocket-id/backend/internal/model" + "github.com/pocket-id/pocket-id/backend/internal/storage" +) + +// UserSyncer applies the desired LDAP state to the users in the database +// Every method takes the transaction the sync runs in, since users, groups, and memberships are reconciled atomically +type UserSyncer interface { + CreateUserInternal(ctx context.Context, dbConfig *appconfig.AppConfigModel, input dto.UserCreateDto, isLdapSync bool, tx *gorm.DB) (model.User, error) + UpdateUserInternal(ctx context.Context, dbConfig *appconfig.AppConfigModel, userID string, input dto.UserCreateDto, updateOwnUser bool, isLdapSync bool, tx *gorm.DB) (model.User, error) + DisableUserInternal(ctx context.Context, tx *gorm.DB, userID string) error + DeleteUserInternal(ctx context.Context, dbConfig *appconfig.AppConfigModel, tx *gorm.DB, userID string, allowLdapDelete bool) error + + // UpdateProfilePicture stores a user's profile picture, which happens after the transaction has been committed since it touches the storage layer + UpdateProfilePicture(ctx context.Context, userID string, file io.ReadSeeker) error +} + +// GroupSyncer applies the desired LDAP state to the user groups in the database +type GroupSyncer interface { + CreateInternal(ctx context.Context, input dto.UserGroupCreateDto, tx *gorm.DB) (model.UserGroup, error) + UpdateInternal(ctx context.Context, dbConfig *appconfig.AppConfigModel, id string, input dto.UserGroupCreateDto, isLdapSync bool, tx *gorm.DB) (model.UserGroup, error) + UpdateUsersInternal(ctx context.Context, id string, userIDs []string, tx *gorm.DB) (model.UserGroup, error) +} + +type Dependencies struct { + DB *gorm.DB + Actors *local.Host + HTTPClient *http.Client + FileStorage storage.FileStorage + + Users UserSyncer + Groups GroupSyncer + AppConfig appconfig.AppConfigResolver + + // ScheduleDisabled keeps the recurring sync from being armed + // It's set in the test environment, where syncs are driven explicitly by the end-to-end tests + ScheduleDisabled bool +} + +type Module struct { + service *Service + handler *handler +} + +func New(deps Dependencies) (*Module, error) { + service := newService(deps) + + // Register the actor that drives the recurring sync + // It's a singleton, so the host bootstraps it at startup and the alarm fires once per cluster rather than once per replica + err := deps.Actors.RegisterSingletonActor(SyncActorType, NewSyncActor(service, deps.AppConfig, deps.ScheduleDisabled)) + if err != nil { + return nil, fmt.Errorf("error registering the %s actor: %w", SyncActorType, err) + } + + return &Module{ + service: service, + handler: newHandler(service, deps.AppConfig), + }, nil +} + +// RegisterRoutes mounts the manual LDAP synchronization endpoint +// auth guards it, as it's an admin-only operation +func (m *Module) RegisterRoutes(apiGroup *gin.RouterGroup, auth gin.HandlerFunc) { + apiGroup.POST("/application-configuration/sync-ldap", auth, httpserver.Handle(m.handler.syncLdap)) +} + +// SyncAll runs a full LDAP synchronization with the provided application configuration +func (m *Module) SyncAll(ctx context.Context, dbConfig *appconfig.AppConfigModel) error { + return m.service.SyncAll(ctx, dbConfig) +} diff --git a/backend/internal/service/ldap_service.go b/backend/internal/ldapsync/service.go similarity index 86% rename from backend/internal/service/ldap_service.go rename to backend/internal/ldapsync/service.go index b874b30e..511c087a 100644 --- a/backend/internal/service/ldap_service.go +++ b/backend/internal/ldapsync/service.go @@ -1,4 +1,4 @@ -package service +package ldapsync import ( "bytes" @@ -17,22 +17,24 @@ import ( "github.com/go-ldap/ldap/v3" "github.com/google/uuid" - "github.com/pocket-id/pocket-id/backend/internal/appconfig" - "github.com/pocket-id/pocket-id/backend/internal/storage" - "github.com/pocket-id/pocket-id/backend/internal/utils" "golang.org/x/text/unicode/norm" "gorm.io/gorm" + "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/dto" "github.com/pocket-id/pocket-id/backend/internal/model" + "github.com/pocket-id/pocket-id/backend/internal/storage" + "github.com/pocket-id/pocket-id/backend/internal/utils" ) -type LdapService struct { +// Service performs the actual LDAP synchronization +// It is deliberately free of any actor concern: the sync actor only decides when a sync runs, while the reconciliation logic lives here and is called directly by the manual "sync now" endpoint too +type Service struct { db *gorm.DB httpClient *http.Client - userService *UserService - groupService *UserGroupService + users UserSyncer + groups GroupSyncer fileStorage storage.FileStorage clientFactory func(dbConfig *appconfig.AppConfigModel) (ldapClient, error) } @@ -68,20 +70,20 @@ type ldapClient interface { Close() error } -func NewLdapService(db *gorm.DB, httpClient *http.Client, userService *UserService, groupService *UserGroupService, fileStorage storage.FileStorage) *LdapService { - service := &LdapService{ - db: db, - httpClient: httpClient, - userService: userService, - groupService: groupService, - fileStorage: fileStorage, +func newService(deps Dependencies) *Service { + service := &Service{ + db: deps.DB, + httpClient: deps.HTTPClient, + users: deps.Users, + groups: deps.Groups, + fileStorage: deps.FileStorage, } service.clientFactory = service.createClient return service } -func (s *LdapService) createClient(dbConfig *appconfig.AppConfigModel) (ldapClient, error) { +func (s *Service) createClient(dbConfig *appconfig.AppConfigModel) (ldapClient, error) { if !dbConfig.LdapEnabled.IsTrue() { return nil, apperror.LdapDisabled() } @@ -103,7 +105,7 @@ func (s *LdapService) createClient(dbConfig *appconfig.AppConfigModel) (ldapClie } // SyncAll synchronizes LDAP using the provided application configuration -func (s *LdapService) SyncAll(ctx context.Context, dbConfig *appconfig.AppConfigModel) error { +func (s *Service) SyncAll(ctx context.Context, dbConfig *appconfig.AppConfigModel) error { // Setup LDAP connection client, err := s.clientFactory(dbConfig) if err != nil { @@ -164,7 +166,7 @@ func (s *LdapService) SyncAll(ctx context.Context, dbConfig *appconfig.AppConfig return nil } -func (s *LdapService) fetchDesiredState(ctx context.Context, client ldapClient, dbConfig *appconfig.AppConfigModel) (ldapDesiredState, error) { +func (s *Service) fetchDesiredState(ctx context.Context, client ldapClient, dbConfig *appconfig.AppConfigModel) (ldapDesiredState, error) { // Fetch users first so we can use their DNs when resolving group members users, userIDs, usernamesByDN, err := s.fetchUsersFromLDAP(ctx, client, dbConfig) if err != nil { @@ -190,7 +192,7 @@ func (s *LdapService) fetchDesiredState(ctx context.Context, client ldapClient, }, nil } -func (s *LdapService) applyAdminGroupMembership(desiredUsers []ldapDesiredUser, desiredGroups []ldapDesiredGroup, dbConfig *appconfig.AppConfigModel) { +func (s *Service) applyAdminGroupMembership(desiredUsers []ldapDesiredUser, desiredGroups []ldapDesiredGroup, dbConfig *appconfig.AppConfigModel) { if dbConfig.LdapAdminGroupName == "" { return } @@ -212,7 +214,7 @@ func (s *LdapService) applyAdminGroupMembership(desiredUsers []ldapDesiredUser, } } -func (s *LdapService) fetchGroupsFromLDAP(ctx context.Context, client ldapClient, usernamesByDN map[string]string, dbConfig *appconfig.AppConfigModel) (desiredGroups []ldapDesiredGroup, ldapGroupIDs map[string]struct{}, err error) { +func (s *Service) fetchGroupsFromLDAP(ctx context.Context, client ldapClient, usernamesByDN map[string]string, dbConfig *appconfig.AppConfigModel) (desiredGroups []ldapDesiredGroup, ldapGroupIDs map[string]struct{}, err error) { // Query LDAP for all groups we want to manage searchAttrs := []string{ dbConfig.LdapAttributeGroupName.String(), @@ -283,7 +285,7 @@ func (s *LdapService) fetchGroupsFromLDAP(ctx context.Context, client ldapClient return desiredGroups, ldapGroupIDs, nil } -func (s *LdapService) fetchUsersFromLDAP(ctx context.Context, client ldapClient, dbConfig *appconfig.AppConfigModel) (desiredUsers []ldapDesiredUser, ldapUserIDs map[string]struct{}, usernamesByDN map[string]string, err error) { +func (s *Service) fetchUsersFromLDAP(ctx context.Context, client ldapClient, dbConfig *appconfig.AppConfigModel) (desiredUsers []ldapDesiredUser, ldapUserIDs map[string]struct{}, usernamesByDN map[string]string, err error) { // Query LDAP for all users we want to manage searchAttrs := []string{ "sn", @@ -368,7 +370,7 @@ func (s *LdapService) fetchUsersFromLDAP(ctx context.Context, client ldapClient, return desiredUsers, ldapUserIDs, usernamesByDN, nil } -func (s *LdapService) resolveGroupMemberUsername(ctx context.Context, client ldapClient, member string, usernamesByDN map[string]string, usernameAttr string) string { +func (s *Service) resolveGroupMemberUsername(ctx context.Context, client ldapClient, member string, usernamesByDN map[string]string, usernameAttr string) string { // First try the DN cache we built while loading users username, exists := usernamesByDN[normalizeLDAPDN(member)] if exists && username != "" { @@ -413,7 +415,7 @@ func (s *LdapService) resolveGroupMemberUsername(ctx context.Context, client lda return norm.NFC.String(username) } -func (s *LdapService) reconcileGroups(ctx context.Context, tx *gorm.DB, desiredGroups []ldapDesiredGroup, ldapGroupIDs map[string]struct{}, dbConfig *appconfig.AppConfigModel) error { +func (s *Service) reconcileGroups(ctx context.Context, tx *gorm.DB, desiredGroups []ldapDesiredGroup, ldapGroupIDs map[string]struct{}, dbConfig *appconfig.AppConfigModel) error { // Load the current LDAP-managed state from the database ldapGroupsInDB, ldapGroupsByID, err := s.loadLDAPGroupsInDB(ctx, tx) if err != nil { @@ -440,25 +442,25 @@ func (s *LdapService) reconcileGroups(ctx context.Context, tx *gorm.DB, desiredG databaseGroup := ldapGroupsByID[desiredGroup.ldapID] if databaseGroup.ID == "" { - newGroup, err := s.groupService.createInternal(ctx, desiredGroup.input, tx) + newGroup, err := s.groups.CreateInternal(ctx, desiredGroup.input, tx) if err != nil { return fmt.Errorf("failed to create group '%s': %w", desiredGroup.input.Name, err) } ldapGroupsByID[desiredGroup.ldapID] = newGroup - _, err = s.groupService.updateUsersInternal(ctx, newGroup.ID, memberUserIDs, tx) + _, err = s.groups.UpdateUsersInternal(ctx, newGroup.ID, memberUserIDs, tx) if err != nil { return fmt.Errorf("failed to sync users for group '%s': %w", desiredGroup.input.Name, err) } continue } - _, err = s.groupService.updateInternal(ctx, databaseGroup.ID, desiredGroup.input, true, tx, dbConfig) + _, err = s.groups.UpdateInternal(ctx, dbConfig, databaseGroup.ID, desiredGroup.input, true, tx) if err != nil { return fmt.Errorf("failed to update group '%s': %w", desiredGroup.input.Name, err) } - _, err = s.groupService.updateUsersInternal(ctx, databaseGroup.ID, memberUserIDs, tx) + _, err = s.groups.UpdateUsersInternal(ctx, databaseGroup.ID, memberUserIDs, tx) if err != nil { return fmt.Errorf("failed to sync users for group '%s': %w", desiredGroup.input.Name, err) } @@ -489,7 +491,7 @@ func (s *LdapService) reconcileGroups(ctx context.Context, tx *gorm.DB, desiredG } //nolint:gocognit -func (s *LdapService) reconcileUsers(ctx context.Context, tx *gorm.DB, desiredUsers []ldapDesiredUser, ldapUserIDs map[string]struct{}, dbConfig *appconfig.AppConfigModel) (savePictures []savePicture, deleteFiles []string, err error) { +func (s *Service) reconcileUsers(ctx context.Context, tx *gorm.DB, desiredUsers []ldapDesiredUser, ldapUserIDs map[string]struct{}, dbConfig *appconfig.AppConfigModel) (savePictures []savePicture, deleteFiles []string, err error) { // Load the current LDAP-managed state from the database ldapUsersInDB, ldapUsersByID, _, err := s.loadLDAPUsersInDB(ctx, tx) if err != nil { @@ -520,7 +522,7 @@ func (s *LdapService) reconcileUsers(ctx context.Context, tx *gorm.DB, desiredUs userID := databaseUser.ID if databaseUser.ID == "" { - createdUser, err := s.userService.createUserInternal(ctx, desiredUser.input, true, tx, dbConfig) + createdUser, err := s.users.CreateUserInternal(ctx, dbConfig, desiredUser.input, true, tx) if apperror.IsCode(err, apperror.CodeAlreadyInUse) { slog.Warn("Skipping creating LDAP user", slog.String("username", desiredUser.input.Username), slog.Any("error", err)) continue @@ -531,7 +533,7 @@ func (s *LdapService) reconcileUsers(ctx context.Context, tx *gorm.DB, desiredUs userID = createdUser.ID ldapUsersByID[desiredUser.ldapID] = createdUser } else { - _, err = s.userService.updateUserInternal(ctx, databaseUser.ID, desiredUser.input, false, true, tx, dbConfig) + _, err = s.users.UpdateUserInternal(ctx, dbConfig, databaseUser.ID, desiredUser.input, false, true, tx) if apperror.IsCode(err, apperror.CodeAlreadyInUse) { slog.Warn("Skipping updating LDAP user", slog.String("username", desiredUser.input.Username), slog.Any("error", err)) continue @@ -561,7 +563,7 @@ func (s *LdapService) reconcileUsers(ctx context.Context, tx *gorm.DB, desiredUs } if dbConfig.LdapSoftDeleteUsers.IsTrue() { - err = s.userService.disableUserInternal(ctx, tx, user.ID) + err = s.users.DisableUserInternal(ctx, tx, user.ID) if err != nil { return nil, nil, fmt.Errorf("failed to disable user %s: %w", user.Username, err) } @@ -570,7 +572,7 @@ func (s *LdapService) reconcileUsers(ctx context.Context, tx *gorm.DB, desiredUs continue } - err = s.userService.deleteUserInternal(ctx, tx, user.ID, true, dbConfig) + err = s.users.DeleteUserInternal(ctx, dbConfig, tx, user.ID, true) if err != nil { if apperror.IsCode(err, apperror.CodeLdapUserUpdate) { return nil, nil, fmt.Errorf("failed to delete user %s: LDAP user must be disabled before deletion", user.Username) @@ -585,7 +587,7 @@ func (s *LdapService) reconcileUsers(ctx context.Context, tx *gorm.DB, desiredUs return savePictures, deleteFiles, nil } -func (s *LdapService) loadLDAPUsersInDB(ctx context.Context, tx *gorm.DB) (users []model.User, byLdapID map[string]model.User, byUsername map[string]model.User, err error) { +func (s *Service) loadLDAPUsersInDB(ctx context.Context, tx *gorm.DB) (users []model.User, byLdapID map[string]model.User, byUsername map[string]model.User, err error) { // Load all LDAP-managed users and index them by LDAP ID and by username err = tx. WithContext(ctx). @@ -607,7 +609,7 @@ func (s *LdapService) loadLDAPUsersInDB(ctx context.Context, tx *gorm.DB) (users return users, byLdapID, byUsername, nil } -func (s *LdapService) loadLDAPGroupsInDB(ctx context.Context, tx *gorm.DB) ([]model.UserGroup, map[string]model.UserGroup, error) { +func (s *Service) loadLDAPGroupsInDB(ctx context.Context, tx *gorm.DB) ([]model.UserGroup, map[string]model.UserGroup, error) { var groups []model.UserGroup // Load all LDAP-managed groups and index them by LDAP ID @@ -629,7 +631,7 @@ func (s *LdapService) loadLDAPGroupsInDB(ctx context.Context, tx *gorm.DB) ([]mo return groups, groupsByID, nil } -func (s *LdapService) saveProfilePicture(parentCtx context.Context, userId string, pictureString string) error { +func (s *Service) saveProfilePicture(parentCtx context.Context, userId string, pictureString string) error { var reader io.ReadSeeker // Accept either a URL, a base64-encoded payload, or raw binary data @@ -666,7 +668,7 @@ func (s *LdapService) saveProfilePicture(parentCtx context.Context, userId strin } // Update the profile picture - err = s.userService.UpdateProfilePicture(parentCtx, userId, reader) + err = s.users.UpdateProfilePicture(parentCtx, userId, reader) if err != nil { return fmt.Errorf("failed to update profile picture: %w", err) } diff --git a/backend/internal/service/ldap_service_test.go b/backend/internal/ldapsync/service_test.go similarity index 94% rename from backend/internal/service/ldap_service_test.go rename to backend/internal/ldapsync/service_test.go index 5218d3e8..a9b6efe4 100644 --- a/backend/internal/service/ldap_service_test.go +++ b/backend/internal/ldapsync/service_test.go @@ -1,4 +1,4 @@ -package service +package ldapsync import ( "net/http" @@ -12,6 +12,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" + "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" ) @@ -21,9 +22,9 @@ type fakeLDAPClient struct { } func TestCreateLDAPClientRejectsDisabledConfiguration(t *testing.T) { - service := NewLdapService(nil, nil, nil, nil, nil) + svc := newService(Dependencies{}) - _, err := service.createClient(&appconfig.AppConfigModel{LdapEnabled: "false"}) + _, err := svc.createClient(&appconfig.AppConfigModel{LdapEnabled: "false"}) require.True(t, apperror.IsCode(err, apperror.CodeLdapDisabled)) } @@ -158,7 +159,7 @@ func TestLdapServiceSyncAllMapsPosixGroupMemberUid(t *testing.T) { appCfg.LdapUserGroupSearchFilter = "(objectClass=posixGroup)" appCfg.LdapAttributeGroupMember = "memberUid" - service, db := newTestLdapServiceWithAppConfig(t, appCfg, newFakeLDAPClient( + service, db := newTestLdapService(t, newFakeLDAPClient( ldapSearchResult( ldapEntry("uid=alice,ou=users,dc=example,dc=com", map[string][]string{ "entryUUID": {"u-alice"}, @@ -287,7 +288,7 @@ func TestLdapServiceSyncAllSetsAdminFromGroupMembership(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - service, db := newTestLdapServiceWithAppConfig(t, tt.appConfig, newFakeLDAPClient( + service, db := newTestLdapService(t, newFakeLDAPClient( ldapSearchResult( ldapEntry("uid=testadmin,ou=people,dc=example,dc=com", map[string][]string{ "entryUUID": {"u-testadmin"}, @@ -316,13 +317,7 @@ func TestLdapServiceSyncAllSetsAdminFromGroupMembership(t *testing.T) { } } -func newTestLdapService(t *testing.T, client ldapClient) (*LdapService, *gorm.DB) { - t.Helper() - - return newTestLdapServiceWithAppConfig(t, defaultTestLDAPAppConfig(), client) -} - -func newTestLdapServiceWithAppConfig(t *testing.T, appConfigModel *appconfig.AppConfigModel, client ldapClient) (*LdapService, *gorm.DB) { +func newTestLdapService(t *testing.T, client ldapClient) (*Service, *gorm.DB) { t.Helper() db := testutils.NewDatabaseForTest(t) @@ -330,23 +325,30 @@ func newTestLdapServiceWithAppConfig(t *testing.T, appConfigModel *appconfig.App fileStorage, err := storage.NewDatabaseStorage(db) require.NoError(t, err) - groupService := NewUserGroupService(db, nil) - userService := NewUserService( + // The sync is exercised against the real user and group services, so the assertions below can check what actually lands in the database + groupService := service.NewUserGroupService(db, nil) + userService := service.NewUserService( db, nil, nil, - NewCustomClaimService(db), - NewAppImagesService(map[string]string{}, fileStorage), + service.NewCustomClaimService(db), + service.NewAppImagesService(map[string]string{}, fileStorage), nil, fileStorage, ) - service := NewLdapService(db, &http.Client{}, userService, groupService, fileStorage) - service.clientFactory = func(dbConfig *appconfig.AppConfigModel) (ldapClient, error) { + svc := newService(Dependencies{ + DB: db, + HTTPClient: &http.Client{}, + FileStorage: fileStorage, + Users: userService, + Groups: groupService, + }) + svc.clientFactory = func(dbConfig *appconfig.AppConfigModel) (ldapClient, error) { return client, nil } - return service, db + return svc, db } func defaultTestLDAPAppConfig() *appconfig.AppConfigModel { diff --git a/backend/internal/onetimeaccess/handler.go b/backend/internal/onetimeaccess/handler.go index 54ecda78..d26bf2c1 100644 --- a/backend/internal/onetimeaccess/handler.go +++ b/backend/internal/onetimeaccess/handler.go @@ -7,6 +7,7 @@ import ( "github.com/gin-gonic/gin" + "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/dto" "github.com/pocket-id/pocket-id/backend/internal/httpserver" @@ -17,10 +18,10 @@ const defaultTokenDuration = 15 * time.Minute type handler struct { service *Service - appConfig AppConfigResolver + appConfig appconfig.AppConfigResolver } -func newHandler(service *Service, appConfig AppConfigResolver) *handler { +func newHandler(service *Service, appConfig appconfig.AppConfigResolver) *handler { return &handler{service: service, appConfig: appConfig} } diff --git a/backend/internal/onetimeaccess/module.go b/backend/internal/onetimeaccess/module.go index 9f7a539e..2b3e1f16 100644 --- a/backend/internal/onetimeaccess/module.go +++ b/backend/internal/onetimeaccess/module.go @@ -31,11 +31,6 @@ type UserProvider interface { GetUser(ctx context.Context, userID string) (model.User, error) } -// AppConfigResolver loads the current application configuration, so handlers can pass it explicitly to the service methods that need it -type AppConfigResolver interface { - GetConfig(ctx context.Context) (*appconfig.AppConfigModel, error) -} - type Dependencies struct { DB *gorm.DB Actors *local.Host @@ -44,7 +39,7 @@ type Dependencies struct { AuditLog AuditLogger UserProvider UserProvider EmailSender EmailSender - AppConfig AppConfigResolver + AppConfig appconfig.AppConfigResolver } type Module struct { diff --git a/backend/internal/service/e2etest_service.go b/backend/internal/service/e2etest_service.go index c647a6fe..3bc1ba91 100644 --- a/backend/internal/service/e2etest_service.go +++ b/backend/internal/service/e2etest_service.go @@ -43,12 +43,18 @@ import ( "github.com/pocket-id/pocket-id/backend/resources" ) +// LdapSyncer runs a full LDAP synchronization +// It's an interface so this package doesn't import the ldapsync package, which imports this one in its tests +type LdapSyncer interface { + SyncAll(ctx context.Context, dbConfig *appconfig.AppConfigModel) error +} + type TestService struct { db *gorm.DB actors *local.Host jwtService *JwtService appConfigService *appconfig.AppConfigService - ldapService *LdapService + ldapSyncer LdapSyncer fileStorage storage.FileStorage externalIdPKey jwk.Key } @@ -63,13 +69,13 @@ const ( e2eEmailVerificationToken = "2FZFSoupBdHyqIL65bWTsgCgHIhxlXup" ) -func NewTestService(db *gorm.DB, actors *local.Host, appConfigService *appconfig.AppConfigService, jwtService *JwtService, ldapService *LdapService, fileStorage storage.FileStorage) (*TestService, error) { +func NewTestService(db *gorm.DB, actors *local.Host, appConfigService *appconfig.AppConfigService, jwtService *JwtService, ldapSyncer LdapSyncer, fileStorage storage.FileStorage) (*TestService, error) { s := &TestService{ db: db, actors: actors, appConfigService: appConfigService, jwtService: jwtService, - ldapService: ldapService, + ldapSyncer: ldapSyncer, fileStorage: fileStorage, } err := s.initExternalIdP() @@ -751,7 +757,7 @@ func (s *TestService) SyncLdap(ctx context.Context) error { if err != nil { return fmt.Errorf("error loading app configuration: %w", err) } - return s.ldapService.SyncAll(ctx, dbConfig) + return s.ldapSyncer.SyncAll(ctx, dbConfig) } // SetLdapTestConfig updates the LDAP configuration used by the end-to-end test server diff --git a/backend/internal/service/user_group_service.go b/backend/internal/service/user_group_service.go index ecb4937d..39f850e2 100644 --- a/backend/internal/service/user_group_service.go +++ b/backend/internal/service/user_group_service.go @@ -110,11 +110,13 @@ func (s *UserGroupService) Delete(ctx context.Context, cfg *appconfig.AppConfigM } func (s *UserGroupService) Create(ctx context.Context, input dto.UserGroupCreateDto) (group model.UserGroup, err error) { - return s.createInternal(ctx, input, s.db) + return s.CreateInternal(ctx, input, s.db) } -func (s *UserGroupService) createInternal(ctx context.Context, input dto.UserGroupCreateDto, tx *gorm.DB) (group model.UserGroup, err error) { - group = model.UserGroup{ +// CreateInternal creates a user group within an existing transaction +// It's exported for the LDAP sync, which reconciles users and groups in a single transaction of its own +func (s *UserGroupService) CreateInternal(ctx context.Context, input dto.UserGroupCreateDto, tx *gorm.DB) (model.UserGroup, error) { + group := model.UserGroup{ FriendlyName: input.FriendlyName, Name: input.Name, } @@ -123,7 +125,7 @@ func (s *UserGroupService) createInternal(ctx context.Context, input dto.UserGro group.LdapID = &input.LdapID } - err = tx. + err := tx. WithContext(ctx). Preload("Users"). Create(&group). @@ -160,6 +162,12 @@ func (s *UserGroupService) Update(ctx context.Context, cfg *appconfig.AppConfigM return group, nil } +// UpdateInternal updates a user group within an existing transaction +// It's exported for the LDAP sync, which reconciles users and groups in a single transaction of its own +func (s *UserGroupService) UpdateInternal(ctx context.Context, cfg *appconfig.AppConfigModel, id string, input dto.UserGroupCreateDto, isLdapSync bool, tx *gorm.DB) (model.UserGroup, error) { + return s.updateInternal(ctx, id, input, isLdapSync, tx, cfg) +} + func (s *UserGroupService) updateInternal(ctx context.Context, id string, input dto.UserGroupCreateDto, isLdapSync bool, tx *gorm.DB, cfg *appconfig.AppConfigModel) (group model.UserGroup, err error) { group, err = s.getInternal(ctx, id, tx) if err != nil { @@ -201,7 +209,7 @@ func (s *UserGroupService) UpdateUsers(ctx context.Context, id string, userIds [ tx.Rollback() }() - group, err = s.updateUsersInternal(ctx, id, userIds, tx) + group, err = s.UpdateUsersInternal(ctx, id, userIds, tx) if err != nil { return model.UserGroup{}, err } @@ -214,8 +222,10 @@ func (s *UserGroupService) UpdateUsers(ctx context.Context, id string, userIds [ return group, nil } -func (s *UserGroupService) updateUsersInternal(ctx context.Context, id string, userIds []string, tx *gorm.DB) (group model.UserGroup, err error) { - group, err = s.getInternal(ctx, id, tx) +// UpdateUsersInternal replaces the members of a user group within an existing transaction +// It's exported for the LDAP sync, which reconciles users and groups in a single transaction of its own +func (s *UserGroupService) UpdateUsersInternal(ctx context.Context, id string, userIds []string, tx *gorm.DB) (model.UserGroup, error) { + group, err := s.getInternal(ctx, id, tx) if err != nil { return model.UserGroup{}, err } diff --git a/backend/internal/service/user_service.go b/backend/internal/service/user_service.go index 0344383f..4f5b7842 100644 --- a/backend/internal/service/user_service.go +++ b/backend/internal/service/user_service.go @@ -196,7 +196,7 @@ func (s *UserService) UpdateProfilePicture(ctx context.Context, userID string, f func (s *UserService) DeleteUser(ctx context.Context, dbConfig *appconfig.AppConfigModel, userID string, allowLdapDelete bool) error { err := s.db.Transaction(func(tx *gorm.DB) error { - return s.deleteUserInternal(ctx, tx, userID, allowLdapDelete, dbConfig) + return s.DeleteUserInternal(ctx, dbConfig, tx, userID, allowLdapDelete) }) if err != nil { return fmt.Errorf("failed to delete user '%s': %w", userID, err) @@ -212,7 +212,10 @@ func (s *UserService) DeleteUser(ctx context.Context, dbConfig *appconfig.AppCon return nil } -func (s *UserService) deleteUserInternal(ctx context.Context, tx *gorm.DB, userID string, allowLdapDelete bool, cfg *appconfig.AppConfigModel) error { +// DeleteUserInternal deletes a user within an existing transaction +// It's exported for the LDAP sync, which deletes users that are no longer in the directory +// Note that the caller is responsible for removing the user's profile picture from the storage layer, which must happen outside of the transaction +func (s *UserService) DeleteUserInternal(ctx context.Context, cfg *appconfig.AppConfigModel, tx *gorm.DB, userID string, allowLdapDelete bool) error { var user model.User err := tx. WithContext(ctx). @@ -439,7 +442,7 @@ func (s *UserService) UpdateUser(ctx context.Context, cfg *appconfig.AppConfigMo tx.Rollback() }() - user, err := s.updateUserInternal(ctx, userID, updatedUser, updateOwnUser, isLdapSync, tx, cfg) + user, err := s.UpdateUserInternal(ctx, cfg, userID, updatedUser, updateOwnUser, isLdapSync, tx) if err != nil { return model.User{}, err } @@ -452,7 +455,9 @@ func (s *UserService) UpdateUser(ctx context.Context, cfg *appconfig.AppConfigMo return user, nil } -func (s *UserService) updateUserInternal(ctx context.Context, userID string, updatedUser dto.UserCreateDto, updateOwnUser bool, isLdapSync bool, tx *gorm.DB, cfg *appconfig.AppConfigModel) (model.User, error) { +// UpdateUserInternal updates a user within an existing transaction +// It's exported for the LDAP sync, which reconciles users and groups in a single transaction of its own +func (s *UserService) UpdateUserInternal(ctx context.Context, cfg *appconfig.AppConfigModel, userID string, updatedUser dto.UserCreateDto, updateOwnUser bool, isLdapSync bool, tx *gorm.DB) (model.User, error) { if cfg.RequireUserEmail.IsTrue() && updatedUser.Email == nil { return model.User{}, apperror.MissingField("email") } @@ -643,7 +648,9 @@ func (s *UserService) ResetProfilePicture(ctx context.Context, userID string) er return nil } -func (s *UserService) disableUserInternal(ctx context.Context, tx *gorm.DB, userID string) error { +// 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 { err := tx. WithContext(ctx). Model(&model.User{}). diff --git a/backend/internal/usersignup/handler.go b/backend/internal/usersignup/handler.go index eab0e9c5..c4746137 100644 --- a/backend/internal/usersignup/handler.go +++ b/backend/internal/usersignup/handler.go @@ -7,6 +7,7 @@ import ( "github.com/gin-gonic/gin" + "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/dto" "github.com/pocket-id/pocket-id/backend/internal/httpserver" @@ -18,10 +19,10 @@ const defaultSignupTokenDuration = time.Hour type handler struct { service *Service - appConfig AppConfigResolver + appConfig appconfig.AppConfigResolver } -func newHandler(service *Service, appConfig AppConfigResolver) *handler { +func newHandler(service *Service, appConfig appconfig.AppConfigResolver) *handler { return &handler{service: service, appConfig: appConfig} } diff --git a/backend/internal/usersignup/module.go b/backend/internal/usersignup/module.go index a5169a6d..d7b174f1 100644 --- a/backend/internal/usersignup/module.go +++ b/backend/internal/usersignup/module.go @@ -27,11 +27,6 @@ type UserCreator interface { CreateUserInternal(ctx context.Context, dbConfig *appconfig.AppConfigModel, input dto.UserCreateDto, isLdapSync bool, tx *gorm.DB) (model.User, error) } -// AppConfigResolver loads the current application configuration, so handlers can pass it explicitly to the service methods that need it -type AppConfigResolver interface { - GetConfig(ctx context.Context) (*appconfig.AppConfigModel, error) -} - type Dependencies struct { DB *gorm.DB Actors *local.Host @@ -39,7 +34,7 @@ type Dependencies struct { Signer TokenService AuditLog AuditLogger UserCreator UserCreator - AppConfig AppConfigResolver + AppConfig appconfig.AppConfigResolver } type Module struct { diff --git a/backend/internal/utils/testing/actors.go b/backend/internal/utils/testing/actors.go index cb0aba95..187fd269 100644 --- a/backend/internal/utils/testing/actors.go +++ b/backend/internal/utils/testing/actors.go @@ -25,9 +25,10 @@ const testActorHostPSK = "pocket-id-test-actor-host-psk-32bytes" // NewActorHostForTest starts a single-host Francis cluster backed by the in-memory provider, runs it, and waits until it is ready to serve invocations // The register callback, if not nil, runs after the host is created but before it starts, so callers can register actors with host.RegisterActor/host.RegisterBuiltInActor (must be called before the host is running) +// Any extra options are appended last, so they override the defaults set here, which lets a test reproduce a production host setting such as the alarm poll interval // The host is stopped when the test ends // The in-memory provider keeps no state on disk, so the test never touches a real database -func NewActorHostForTest(t *testing.T, register func(t *testing.T, h *local.Host)) *local.Host { +func NewActorHostForTest(t *testing.T, register func(t *testing.T, h *local.Host), extraOpts ...local.HostOption) *local.Host { t.Helper() address := freeLoopbackUDPAddr(t) @@ -37,6 +38,7 @@ func NewActorHostForTest(t *testing.T, register func(t *testing.T, h *local.Host local.WithStandaloneMemoryProvider(standalone.StandaloneMemoryOptions{}), local.WithShutdownGracePeriod(time.Second), } + hostOpts = append(hostOpts, extraOpts...) h, err := local.NewHost(hostOpts...) require.NoError(t, err) diff --git a/backend/internal/webauthn/handler.go b/backend/internal/webauthn/handler.go index 7d905f1d..133a8e5f 100644 --- a/backend/internal/webauthn/handler.go +++ b/backend/internal/webauthn/handler.go @@ -7,6 +7,7 @@ import ( "github.com/gin-gonic/gin" "github.com/go-webauthn/webauthn/protocol" + "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/dto" "github.com/pocket-id/pocket-id/backend/internal/httpserver" @@ -15,11 +16,14 @@ import ( type handler struct { service *Service - appConfig AppConfigResolver + appConfig appconfig.AppConfigResolver } -func newHandler(service *Service, appConfig AppConfigResolver) *handler { - return &handler{service: service, appConfig: appConfig} +func newHandler(service *Service, appConfig appconfig.AppConfigResolver) *handler { + return &handler{ + service: service, + appConfig: appConfig, + } } func (h *handler) beginRegistration(c *gin.Context) error { diff --git a/backend/internal/webauthn/module.go b/backend/internal/webauthn/module.go index f5f49543..300f8ac6 100644 --- a/backend/internal/webauthn/module.go +++ b/backend/internal/webauthn/module.go @@ -24,18 +24,13 @@ type AuditLogger interface { CreateNewSignInWithEmail(ctx context.Context, ipAddress, userAgent, userID string, tx *gorm.DB, emailLoginNotificationEnabled bool) model.AuditLog } -// AppConfigResolver loads the current application configuration, so handlers can pass it explicitly to the service methods that need it -type AppConfigResolver interface { - GetConfig(ctx context.Context) (*appconfig.AppConfigModel, error) -} - type Dependencies struct { DB *gorm.DB AppURL string Signer TokenService AuditLog AuditLogger - AppConfig AppConfigResolver + AppConfig appconfig.AppConfigResolver } type Module struct {