diff --git a/server/router/api/v1/idp_service.go b/server/router/api/v1/idp_service.go index b6b65b283..52535733e 100644 --- a/server/router/api/v1/idp_service.go +++ b/server/router/api/v1/idp_service.go @@ -49,17 +49,8 @@ func (s *APIV1Service) ListIdentityProviders(ctx context.Context, _ *v1pb.ListId response := &v1pb.ListIdentityProvidersResponse{ IdentityProviders: []*v1pb.IdentityProvider{}, } - - // Default to lowest-privilege role, update later based on real role - currentUserRole := store.RoleUser - currentUser, err := s.fetchCurrentUser(ctx) - if err == nil && currentUser != nil { - currentUserRole = currentUser.Role - } - for _, identityProvider := range identityProviders { - identityProviderConverted := convertIdentityProviderFromStore(identityProvider) - response.IdentityProviders = append(response.IdentityProviders, redactIdentityProviderResponse(identityProviderConverted, currentUserRole)) + response.IdentityProviders = append(response.IdentityProviders, convertIdentityProviderFromStore(identityProvider)) } return response, nil } @@ -79,15 +70,7 @@ func (s *APIV1Service) GetIdentityProvider(ctx context.Context, request *v1pb.Ge return nil, status.Errorf(codes.NotFound, "identity provider not found") } - // Default to lowest-privilege role, update later based on real role - currentUserRole := store.RoleUser - currentUser, err := s.fetchCurrentUser(ctx) - if err == nil && currentUser != nil { - currentUserRole = currentUser.Role - } - - identityProviderConverted := convertIdentityProviderFromStore(identityProvider) - return redactIdentityProviderResponse(identityProviderConverted, currentUserRole), nil + return convertIdentityProviderFromStore(identityProvider), nil } func (s *APIV1Service) UpdateIdentityProvider(ctx context.Context, request *v1pb.UpdateIdentityProviderRequest) (*v1pb.IdentityProvider, error) { @@ -137,6 +120,15 @@ func (s *APIV1Service) UpdateIdentityProvider(ctx context.Context, request *v1pb } } + // Preserve write-only credential when the caller sends an empty value. + if update.Config != nil { + if oauth2Config := update.Config.GetOauth2Config(); oauth2Config != nil && oauth2Config.ClientSecret == "" { + if existingOAuth := existing.Config.GetOauth2Config(); existingOAuth != nil { + oauth2Config.ClientSecret = existingOAuth.ClientSecret + } + } + } + identityProvider, err := s.Store.UpdateIdentityProvider(ctx, update) if err != nil { return nil, status.Errorf(codes.Internal, "failed to update identity provider, error: %+v", err) @@ -188,12 +180,12 @@ func convertIdentityProviderFromStore(identityProvider *storepb.IdentityProvider temp.Config = &v1pb.IdentityProviderConfig{ Config: &v1pb.IdentityProviderConfig_Oauth2Config{ Oauth2Config: &v1pb.OAuth2Config{ - ClientId: oauth2Config.ClientId, - ClientSecret: oauth2Config.ClientSecret, - AuthUrl: oauth2Config.AuthUrl, - TokenUrl: oauth2Config.TokenUrl, - UserInfoUrl: oauth2Config.UserInfoUrl, - Scopes: oauth2Config.Scopes, + ClientId: oauth2Config.ClientId, + // ClientSecret is write-only: never returned in responses. + AuthUrl: oauth2Config.AuthUrl, + TokenUrl: oauth2Config.TokenUrl, + UserInfoUrl: oauth2Config.UserInfoUrl, + Scopes: oauth2Config.Scopes, FieldMapping: &v1pb.FieldMapping{ Identifier: oauth2Config.FieldMapping.Identifier, DisplayName: oauth2Config.FieldMapping.DisplayName, @@ -242,12 +234,3 @@ func convertIdentityProviderConfigToStore(identityProviderType v1pb.IdentityProv return nil } -func redactIdentityProviderResponse(identityProvider *v1pb.IdentityProvider, userRole store.Role) *v1pb.IdentityProvider { - if userRole != store.RoleAdmin { - if identityProvider.Type == v1pb.IdentityProvider_OAUTH2 { - identityProvider.Config.GetOauth2Config().ClientSecret = "" - } - } - - return identityProvider -} diff --git a/server/router/api/v1/instance_service.go b/server/router/api/v1/instance_service.go index 5fb3dfcf5..76166a5ba 100644 --- a/server/router/api/v1/instance_service.go +++ b/server/router/api/v1/instance_service.go @@ -71,8 +71,9 @@ func (s *APIV1Service) GetInstanceSetting(ctx context.Context, request *v1pb.Get return nil, status.Errorf(codes.NotFound, "instance setting not found") } - // For storage setting, only admin can get it. - if instanceSetting.Key == storepb.InstanceSettingKey_STORAGE { + // Storage and notification settings contain credentials; restrict to admins only. + if instanceSetting.Key == storepb.InstanceSettingKey_STORAGE || + instanceSetting.Key == storepb.InstanceSettingKey_NOTIFICATION { user, err := s.fetchCurrentUser(ctx) if err != nil { return nil, status.Errorf(codes.Internal, "failed to get current user: %v", err) @@ -108,6 +109,26 @@ func (s *APIV1Service) UpdateInstanceSetting(ctx context.Context, request *v1pb. } updateSetting := convertInstanceSettingToStore(request.Setting) + + // Preserve write-only credential fields when the caller sends an empty value. + // An empty string means "no change", not "clear the credential". + switch updateSetting.Key { + case storepb.InstanceSettingKey_NOTIFICATION: + if notif := updateSetting.GetNotificationSetting(); notif != nil && notif.Email != nil && notif.Email.SmtpPassword == "" { + existing, err := s.Store.GetInstanceNotificationSetting(ctx) + if err == nil && existing != nil && existing.Email != nil { + notif.Email.SmtpPassword = existing.Email.SmtpPassword + } + } + case storepb.InstanceSettingKey_STORAGE: + if storage := updateSetting.GetStorageSetting(); storage != nil && storage.S3Config != nil && storage.S3Config.AccessKeySecret == "" { + existing, err := s.Store.GetInstanceStorageSetting(ctx) + if err == nil && existing != nil && existing.S3Config != nil { + storage.S3Config.AccessKeySecret = existing.S3Config.AccessKeySecret + } + } + } + instanceSetting, err := s.Store.UpsertInstanceSetting(ctx, updateSetting) if err != nil { return nil, status.Errorf(codes.Internal, "failed to upsert instance setting: %v", err) @@ -240,12 +261,12 @@ func convertInstanceStorageSettingFromStore(settingpb *storepb.InstanceStorageSe } if settingpb.S3Config != nil { setting.S3Config = &v1pb.InstanceSetting_StorageSetting_S3Config{ - AccessKeyId: settingpb.S3Config.AccessKeyId, - AccessKeySecret: settingpb.S3Config.AccessKeySecret, - Endpoint: settingpb.S3Config.Endpoint, - Region: settingpb.S3Config.Region, - Bucket: settingpb.S3Config.Bucket, - UsePathStyle: settingpb.S3Config.UsePathStyle, + AccessKeyId: settingpb.S3Config.AccessKeyId, + // AccessKeySecret is write-only: never returned in responses. + Endpoint: settingpb.S3Config.Endpoint, + Region: settingpb.S3Config.Region, + Bucket: settingpb.S3Config.Bucket, + UsePathStyle: settingpb.S3Config.UsePathStyle, } } return setting @@ -341,12 +362,12 @@ func convertInstanceNotificationSettingFromStore(setting *storepb.InstanceNotifi SmtpHost: setting.Email.SmtpHost, SmtpPort: setting.Email.SmtpPort, SmtpUsername: setting.Email.SmtpUsername, - SmtpPassword: setting.Email.SmtpPassword, - FromEmail: setting.Email.FromEmail, - FromName: setting.Email.FromName, - ReplyTo: setting.Email.ReplyTo, - UseTls: setting.Email.UseTls, - UseSsl: setting.Email.UseSsl, + // SmtpPassword is write-only: never returned in responses. + FromEmail: setting.Email.FromEmail, + FromName: setting.Email.FromName, + ReplyTo: setting.Email.ReplyTo, + UseTls: setting.Email.UseTls, + UseSsl: setting.Email.UseSsl, } } return notificationSetting diff --git a/server/router/api/v1/test/idp_service_test.go b/server/router/api/v1/test/idp_service_test.go index 302a2737e..a2e88a1f4 100644 --- a/server/router/api/v1/test/idp_service_test.go +++ b/server/router/api/v1/test/idp_service_test.go @@ -8,6 +8,8 @@ import ( "google.golang.org/protobuf/types/known/fieldmaskpb" v1pb "github.com/usememos/memos/proto/gen/api/v1" + apiv1 "github.com/usememos/memos/server/router/api/v1" + "github.com/usememos/memos/store" ) func TestCreateIdentityProvider(t *testing.T) { @@ -233,7 +235,7 @@ func TestGetIdentityProvider(t *testing.T) { Name: created.Name, } - // Test unauthenticated, should not contain client secret + // ClientSecret is write-only: never returned in responses, even to admins. resp, err := ts.Service.GetIdentityProvider(ctx, getReq) require.NoError(t, err) require.NotNil(t, resp) @@ -242,18 +244,16 @@ func TestGetIdentityProvider(t *testing.T) { require.Equal(t, v1pb.IdentityProvider_OAUTH2, resp.Type) require.NotNil(t, resp.Config.GetOauth2Config()) require.Equal(t, "test-client", resp.Config.GetOauth2Config().ClientId) - require.Equal(t, "", resp.Config.GetOauth2Config().ClientSecret) + require.Empty(t, resp.Config.GetOauth2Config().ClientSecret, + "ClientSecret must never be returned in responses") - // Test as host user, should contain client secret - respHostUser, err := ts.Service.GetIdentityProvider(userCtx, getReq) + // Same for admin: secret is still write-only. + respAdmin, err := ts.Service.GetIdentityProvider(userCtx, getReq) require.NoError(t, err) - require.NotNil(t, respHostUser) - require.Equal(t, created.Name, respHostUser.Name) - require.Equal(t, "Test Provider", respHostUser.Title) - require.Equal(t, v1pb.IdentityProvider_OAUTH2, respHostUser.Type) - require.NotNil(t, respHostUser.Config.GetOauth2Config()) - require.Equal(t, "test-client", respHostUser.Config.GetOauth2Config().ClientId) - require.Equal(t, "test-secret", respHostUser.Config.GetOauth2Config().ClientSecret) + require.NotNil(t, respAdmin) + require.Equal(t, "test-client", respAdmin.Config.GetOauth2Config().ClientId) + require.Empty(t, respAdmin.Config.GetOauth2Config().ClientSecret, + "ClientSecret must never be returned in responses, even to admins") }) t.Run("GetIdentityProvider not found", func(t *testing.T) { @@ -361,6 +361,67 @@ func TestUpdateIdentityProvider(t *testing.T) { require.Equal(t, "updated-client", updated.Config.GetOauth2Config().ClientId) }) + t.Run("UpdateIdentityProvider empty ClientSecret preserves existing credential", func(t *testing.T) { + ts := NewTestService(t) + defer ts.Cleanup() + + hostUser, err := ts.CreateHostUser(ctx, "admin") + require.NoError(t, err) + userCtx := ts.CreateUserContext(ctx, hostUser.ID) + + // Create IDP with a real secret. + created, err := ts.Service.CreateIdentityProvider(userCtx, &v1pb.CreateIdentityProviderRequest{ + IdentityProvider: &v1pb.IdentityProvider{ + Title: "Preserve Secret Test", + Type: v1pb.IdentityProvider_OAUTH2, + Config: &v1pb.IdentityProviderConfig{ + Config: &v1pb.IdentityProviderConfig_Oauth2Config{ + Oauth2Config: &v1pb.OAuth2Config{ + ClientId: "cid", + ClientSecret: "original-secret", + AuthUrl: "https://ex.com/auth", + TokenUrl: "https://ex.com/token", + UserInfoUrl: "https://ex.com/user", + FieldMapping: &v1pb.FieldMapping{Identifier: "id"}, + }, + }, + }, + }, + }) + require.NoError(t, err) + + // Update with empty ClientSecret (simulating UI that doesn't resend the secret). + _, err = ts.Service.UpdateIdentityProvider(userCtx, &v1pb.UpdateIdentityProviderRequest{ + IdentityProvider: &v1pb.IdentityProvider{ + Name: created.Name, + Title: "Updated Title", + Type: v1pb.IdentityProvider_OAUTH2, + Config: &v1pb.IdentityProviderConfig{ + Config: &v1pb.IdentityProviderConfig_Oauth2Config{ + Oauth2Config: &v1pb.OAuth2Config{ + ClientId: "cid", + ClientSecret: "", // empty = preserve existing + AuthUrl: "https://ex.com/auth", + TokenUrl: "https://ex.com/token", + UserInfoUrl: "https://ex.com/user", + FieldMapping: &v1pb.FieldMapping{Identifier: "id"}, + }, + }, + }, + }, + UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"title", "config"}}, + }) + require.NoError(t, err) + + // Verify the stored secret was preserved by reading from store directly. + uid, _ := apiv1.ExtractIdentityProviderUIDFromName(created.Name) + stored, err := ts.Store.GetIdentityProvider(ctx, &store.FindIdentityProvider{UID: &uid}) + require.NoError(t, err) + require.Equal(t, "original-secret", stored.Config.GetOauth2Config().ClientSecret, + "existing ClientSecret must be preserved when an empty value is sent") + require.Equal(t, "Updated Title", stored.Name) + }) + t.Run("UpdateIdentityProvider missing update mask", func(t *testing.T) { ts := NewTestService(t) defer ts.Cleanup() diff --git a/server/router/api/v1/test/instance_service_test.go b/server/router/api/v1/test/instance_service_test.go index 609989dab..bd7e3f288 100644 --- a/server/router/api/v1/test/instance_service_test.go +++ b/server/router/api/v1/test/instance_service_test.go @@ -204,21 +204,38 @@ func TestGetInstanceSetting(t *testing.T) { require.Empty(t, resp.GetTagsSetting().GetTags()) }) - t.Run("GetInstanceSetting - notification setting", func(t *testing.T) { + t.Run("GetInstanceSetting - notification setting requires admin", func(t *testing.T) { ts := NewTestService(t) defer ts.Cleanup() - req := &v1pb.GetInstanceSettingRequest{ - Name: "instance/settings/NOTIFICATION", - } - resp, err := ts.Service.GetInstanceSetting(ctx, req) + admin, err := ts.CreateHostUser(ctx, "admin") + require.NoError(t, err) + adminCtx := ts.CreateUserContext(ctx, admin.ID) + regularUser, err := ts.CreateRegularUser(ctx, "user") + require.NoError(t, err) + userCtx := ts.CreateUserContext(ctx, regularUser.ID) + + req := &v1pb.GetInstanceSettingRequest{Name: "instance/settings/NOTIFICATION"} + + // Unauthenticated request must be rejected. + _, err = ts.Service.GetInstanceSetting(ctx, req) + require.Error(t, err) + require.Contains(t, err.Error(), "not authenticated") + + // Non-admin request must be rejected. + _, err = ts.Service.GetInstanceSetting(userCtx, req) + require.Error(t, err) + require.Contains(t, err.Error(), "permission denied") + + // Admin request succeeds and does NOT expose SmtpPassword. + resp, err := ts.Service.GetInstanceSetting(adminCtx, req) require.NoError(t, err) require.NotNil(t, resp) require.Equal(t, "instance/settings/NOTIFICATION", resp.Name) require.NotNil(t, resp.GetNotificationSetting()) - require.NotNil(t, resp.GetNotificationSetting().GetEmail()) - require.False(t, resp.GetNotificationSetting().GetEmail().GetEnabled()) + require.Empty(t, resp.GetNotificationSetting().GetEmail().GetSmtpPassword(), + "SmtpPassword must never be returned in responses") }) t.Run("GetInstanceSetting - invalid setting name", func(t *testing.T) { @@ -301,14 +318,16 @@ func TestUpdateInstanceSetting(t *testing.T) { require.Contains(t, err.Error(), "invalid instance setting") }) - t.Run("UpdateInstanceSetting - notification setting", func(t *testing.T) { + t.Run("UpdateInstanceSetting - notification setting password is write-only", func(t *testing.T) { ts := NewTestService(t) defer ts.Cleanup() hostUser, err := ts.CreateHostUser(ctx, "admin") require.NoError(t, err) + adminCtx := ts.CreateUserContext(ctx, hostUser.ID) - resp, err := ts.Service.UpdateInstanceSetting(ts.CreateUserContext(ctx, hostUser.ID), &v1pb.UpdateInstanceSettingRequest{ + // Save notification setting with a password. + resp, err := ts.Service.UpdateInstanceSetting(adminCtx, &v1pb.UpdateInstanceSettingRequest{ Setting: &v1pb.InstanceSetting{ Name: "instance/settings/NOTIFICATION", Value: &v1pb.InstanceSetting_NotificationSetting_{ @@ -330,9 +349,117 @@ func TestUpdateInstanceSetting(t *testing.T) { UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"notification_setting"}}, }) require.NoError(t, err) - require.NotNil(t, resp.GetNotificationSetting()) - require.NotNil(t, resp.GetNotificationSetting().GetEmail()) require.True(t, resp.GetNotificationSetting().GetEmail().GetEnabled()) require.Equal(t, "smtp.example.com", resp.GetNotificationSetting().GetEmail().GetSmtpHost()) + // Password must not be returned even in the update response. + require.Empty(t, resp.GetNotificationSetting().GetEmail().GetSmtpPassword(), + "SmtpPassword must never be returned in responses") + }) + + t.Run("UpdateInstanceSetting - empty password preserves existing credential", func(t *testing.T) { + ts := NewTestService(t) + defer ts.Cleanup() + + hostUser, err := ts.CreateHostUser(ctx, "admin") + require.NoError(t, err) + adminCtx := ts.CreateUserContext(ctx, hostUser.ID) + + notificationSetting := &v1pb.InstanceSetting{ + Name: "instance/settings/NOTIFICATION", + Value: &v1pb.InstanceSetting_NotificationSetting_{ + NotificationSetting: &v1pb.InstanceSetting_NotificationSetting{ + Email: &v1pb.InstanceSetting_NotificationSetting_EmailSetting{ + Enabled: true, + SmtpHost: "smtp.example.com", + SmtpPort: 587, + SmtpUsername: "bot@example.com", + SmtpPassword: "original-password", + FromEmail: "bot@example.com", + }, + }, + }, + } + + // First save with a real password. + _, err = ts.Service.UpdateInstanceSetting(adminCtx, &v1pb.UpdateInstanceSettingRequest{ + Setting: notificationSetting, + }) + require.NoError(t, err) + + // Second update with an empty password (simulating a UI that doesn't re-send the secret). + notificationSetting.GetNotificationSetting().GetEmail().SmtpPassword = "" + notificationSetting.GetNotificationSetting().GetEmail().SmtpHost = "smtp2.example.com" + _, err = ts.Service.UpdateInstanceSetting(adminCtx, &v1pb.UpdateInstanceSettingRequest{ + Setting: notificationSetting, + }) + require.NoError(t, err) + + // The stored setting should have preserved the original password. + stored, err := ts.Store.GetInstanceNotificationSetting(ctx) + require.NoError(t, err) + require.Equal(t, "original-password", stored.GetEmail().GetSmtpPassword(), + "existing SmtpPassword must be preserved when an empty value is sent") + require.Equal(t, "smtp2.example.com", stored.GetEmail().GetSmtpHost()) + }) + + t.Run("UpdateInstanceSetting - S3 secret is write-only and preserved on empty", func(t *testing.T) { + ts := NewTestService(t) + defer ts.Cleanup() + + hostUser, err := ts.CreateHostUser(ctx, "admin") + require.NoError(t, err) + adminCtx := ts.CreateUserContext(ctx, hostUser.ID) + + // Save storage setting with a real secret. + _, err = ts.Service.UpdateInstanceSetting(adminCtx, &v1pb.UpdateInstanceSettingRequest{ + Setting: &v1pb.InstanceSetting{ + Name: "instance/settings/STORAGE", + Value: &v1pb.InstanceSetting_StorageSetting_{ + StorageSetting: &v1pb.InstanceSetting_StorageSetting{ + S3Config: &v1pb.InstanceSetting_StorageSetting_S3Config{ + AccessKeyId: "AKID", + AccessKeySecret: "super-secret", + Endpoint: "s3.example.com", + Region: "us-east-1", + Bucket: "memos", + }, + }, + }, + }, + }) + require.NoError(t, err) + + // Read back: secret must not be returned. + resp, err := ts.Service.GetInstanceSetting(adminCtx, &v1pb.GetInstanceSettingRequest{ + Name: "instance/settings/STORAGE", + }) + require.NoError(t, err) + require.Empty(t, resp.GetStorageSetting().GetS3Config().GetAccessKeySecret(), + "AccessKeySecret must never be returned in responses") + + // Update with empty secret; original must be preserved in the store. + _, err = ts.Service.UpdateInstanceSetting(adminCtx, &v1pb.UpdateInstanceSettingRequest{ + Setting: &v1pb.InstanceSetting{ + Name: "instance/settings/STORAGE", + Value: &v1pb.InstanceSetting_StorageSetting_{ + StorageSetting: &v1pb.InstanceSetting_StorageSetting{ + S3Config: &v1pb.InstanceSetting_StorageSetting_S3Config{ + AccessKeyId: "AKID", + AccessKeySecret: "", // omitted / not changed + Endpoint: "s3-v2.example.com", + Region: "us-east-1", + Bucket: "memos", + }, + }, + }, + }, + }) + require.NoError(t, err) + + stored, err := ts.Store.GetInstanceStorageSetting(ctx) + require.NoError(t, err) + require.Equal(t, "super-secret", stored.GetS3Config().GetAccessKeySecret(), + "existing AccessKeySecret must be preserved when an empty value is sent") + require.Equal(t, "s3-v2.example.com", stored.GetS3Config().GetEndpoint()) }) }