From 2ff5960b39b9a00f5a1ddcae2c33bbb84c3f4c0c Mon Sep 17 00:00:00 2001 From: Andrew Valleteau Date: Mon, 2 Dec 2024 06:59:07 +0100 Subject: [PATCH] fix(config): syncing for postgres hook without secrets (#2931) * fix(config): syncing for postgres hook without secrets * chore: apply pr comments * chore: stricter hook secrets validation * chore: update unit tests and error handling --------- Co-authored-by: Qiao Han --- pkg/config/auth.go | 55 ++++++++---- pkg/config/auth_test.go | 88 ++++++++++++++----- pkg/config/config.go | 32 ++++--- pkg/config/config_test.go | 87 +++++++++++------- .../local_enabled_and_disabled.diff | 25 ++++-- 5 files changed, 197 insertions(+), 90 deletions(-) diff --git a/pkg/config/auth.go b/pkg/config/auth.go index 572b88950..10611c23b 100644 --- a/pkg/config/auth.go +++ b/pkg/config/auth.go @@ -262,53 +262,76 @@ func (a *auth) FromRemoteAuthConfig(remoteConfig v1API.AuthConfigResponse) { func (h hook) toAuthConfigBody(body *v1API.UpdateAuthConfigBody) { if body.HookCustomAccessTokenEnabled = &h.CustomAccessToken.Enabled; *body.HookCustomAccessTokenEnabled { body.HookCustomAccessTokenUri = &h.CustomAccessToken.URI - body.HookCustomAccessTokenSecrets = &h.CustomAccessToken.Secrets + if len(h.CustomAccessToken.Secrets) > 0 { + body.HookCustomAccessTokenSecrets = &h.CustomAccessToken.Secrets + } } if body.HookSendEmailEnabled = &h.SendEmail.Enabled; *body.HookSendEmailEnabled { body.HookSendEmailUri = &h.SendEmail.URI - body.HookSendEmailSecrets = &h.SendEmail.Secrets + if len(h.SendEmail.Secrets) > 0 { + body.HookSendEmailSecrets = &h.SendEmail.Secrets + } } if body.HookSendSmsEnabled = &h.SendSMS.Enabled; *body.HookSendSmsEnabled { body.HookSendSmsUri = &h.SendSMS.URI - body.HookSendSmsSecrets = &h.SendSMS.Secrets + if len(h.SendSMS.Secrets) > 0 { + body.HookSendSmsSecrets = &h.SendSMS.Secrets + } } // Enterprise and team only features if body.HookMfaVerificationAttemptEnabled = &h.MFAVerificationAttempt.Enabled; *body.HookMfaVerificationAttemptEnabled { body.HookMfaVerificationAttemptUri = &h.MFAVerificationAttempt.URI - body.HookMfaVerificationAttemptSecrets = &h.MFAVerificationAttempt.Secrets + if len(h.MFAVerificationAttempt.Secrets) > 0 { + body.HookMfaVerificationAttemptSecrets = &h.MFAVerificationAttempt.Secrets + } } if body.HookPasswordVerificationAttemptEnabled = &h.PasswordVerificationAttempt.Enabled; *body.HookPasswordVerificationAttemptEnabled { body.HookPasswordVerificationAttemptUri = &h.PasswordVerificationAttempt.URI - body.HookPasswordVerificationAttemptSecrets = &h.PasswordVerificationAttempt.Secrets + if len(h.PasswordVerificationAttempt.Secrets) > 0 { + body.HookPasswordVerificationAttemptSecrets = &h.PasswordVerificationAttempt.Secrets + } } } - func (h *hook) fromAuthConfig(remoteConfig v1API.AuthConfigResponse) { // Ignore disabled hooks because their envs are not loaded if h.CustomAccessToken.Enabled { h.CustomAccessToken.URI = cast.Val(remoteConfig.HookCustomAccessTokenUri, "") - h.CustomAccessToken.Secrets = hashPrefix + cast.Val(remoteConfig.HookCustomAccessTokenSecrets, "") + if remoteConfig.HookCustomAccessTokenSecrets != nil { + h.CustomAccessToken.Secrets = hashPrefix + cast.Val(remoteConfig.HookCustomAccessTokenSecrets, "") + } } h.CustomAccessToken.Enabled = cast.Val(remoteConfig.HookCustomAccessTokenEnabled, false) + if h.SendEmail.Enabled { h.SendEmail.URI = cast.Val(remoteConfig.HookSendEmailUri, "") - h.SendEmail.Secrets = hashPrefix + cast.Val(remoteConfig.HookSendEmailSecrets, "") + if remoteConfig.HookSendEmailSecrets != nil { + h.SendEmail.Secrets = hashPrefix + cast.Val(remoteConfig.HookSendEmailSecrets, "") + } } h.SendEmail.Enabled = cast.Val(remoteConfig.HookSendEmailEnabled, false) + if h.SendSMS.Enabled { h.SendSMS.URI = cast.Val(remoteConfig.HookSendSmsUri, "") - h.SendSMS.Secrets = hashPrefix + cast.Val(remoteConfig.HookSendSmsSecrets, "") + if remoteConfig.HookSendSmsSecrets != nil { + h.SendSMS.Secrets = hashPrefix + cast.Val(remoteConfig.HookSendSmsSecrets, "") + } } h.SendSMS.Enabled = cast.Val(remoteConfig.HookSendSmsEnabled, false) + // Enterprise and team only features if h.MFAVerificationAttempt.Enabled { h.MFAVerificationAttempt.URI = cast.Val(remoteConfig.HookMfaVerificationAttemptUri, "") - h.MFAVerificationAttempt.Secrets = hashPrefix + cast.Val(remoteConfig.HookMfaVerificationAttemptSecrets, "") + if remoteConfig.HookMfaVerificationAttemptSecrets != nil { + h.MFAVerificationAttempt.Secrets = hashPrefix + cast.Val(remoteConfig.HookMfaVerificationAttemptSecrets, "") + } } h.MFAVerificationAttempt.Enabled = cast.Val(remoteConfig.HookMfaVerificationAttemptEnabled, false) + if h.PasswordVerificationAttempt.Enabled { h.PasswordVerificationAttempt.URI = cast.Val(remoteConfig.HookPasswordVerificationAttemptUri, "") - h.PasswordVerificationAttempt.Secrets = hashPrefix + cast.Val(remoteConfig.HookPasswordVerificationAttemptSecrets, "") + if remoteConfig.HookPasswordVerificationAttemptSecrets != nil { + h.PasswordVerificationAttempt.Secrets = hashPrefix + cast.Val(remoteConfig.HookPasswordVerificationAttemptSecrets, "") + } } h.PasswordVerificationAttempt.Enabled = cast.Val(remoteConfig.HookPasswordVerificationAttemptEnabled, false) } @@ -851,19 +874,19 @@ func (a *auth) HashSecrets(key string) { case a.Sms.Vonage.Enabled: a.Sms.Vonage.ApiSecret = hash(a.Sms.Vonage.ApiSecret) } - if a.Hook.MFAVerificationAttempt.Enabled { + if a.Hook.MFAVerificationAttempt.Enabled && len(a.Hook.MFAVerificationAttempt.Secrets) > 0 { a.Hook.MFAVerificationAttempt.Secrets = hash(a.Hook.MFAVerificationAttempt.Secrets) } - if a.Hook.PasswordVerificationAttempt.Enabled { + if a.Hook.PasswordVerificationAttempt.Enabled && len(a.Hook.PasswordVerificationAttempt.Secrets) > 0 { a.Hook.PasswordVerificationAttempt.Secrets = hash(a.Hook.PasswordVerificationAttempt.Secrets) } - if a.Hook.CustomAccessToken.Enabled { + if a.Hook.CustomAccessToken.Enabled && len(a.Hook.CustomAccessToken.Secrets) > 0 { a.Hook.CustomAccessToken.Secrets = hash(a.Hook.CustomAccessToken.Secrets) } - if a.Hook.SendSMS.Enabled { + if a.Hook.SendSMS.Enabled && len(a.Hook.SendSMS.Secrets) > 0 { a.Hook.SendSMS.Secrets = hash(a.Hook.SendSMS.Secrets) } - if a.Hook.SendEmail.Enabled { + if a.Hook.SendEmail.Enabled && len(a.Hook.SendEmail.Secrets) > 0 { a.Hook.SendEmail.Secrets = hash(a.Hook.SendEmail.Secrets) } for name, provider := range a.External { diff --git a/pkg/config/auth_test.go b/pkg/config/auth_test.go index cba5cd9a1..057c5ccdd 100644 --- a/pkg/config/auth_test.go +++ b/pkg/config/auth_test.go @@ -121,29 +121,47 @@ func TestHookDiff(t *testing.T) { t.Run("local and remote enabled", func(t *testing.T) { c := newWithDefaults() c.Hook = hook{ - CustomAccessToken: hookConfig{Enabled: true}, - SendSMS: hookConfig{Enabled: true}, - SendEmail: hookConfig{Enabled: true}, - MFAVerificationAttempt: hookConfig{Enabled: true}, - PasswordVerificationAttempt: hookConfig{Enabled: true}, + CustomAccessToken: hookConfig{ + Enabled: true, + URI: "http://example.com", + Secrets: "test-secret", + }, + SendSMS: hookConfig{ + Enabled: true, + URI: "http://example.com", + Secrets: "test-secret", + }, + SendEmail: hookConfig{ + Enabled: true, + URI: "https://example.com", + Secrets: "test-secret", + }, + MFAVerificationAttempt: hookConfig{ + Enabled: true, + URI: "https://example.com", + Secrets: "test-secret", + }, + PasswordVerificationAttempt: hookConfig{ + Enabled: true, + URI: "pg-functions://verifyPassword", + }, } // Run test diff, err := c.DiffWithRemote("", v1API.AuthConfigResponse{ HookCustomAccessTokenEnabled: cast.Ptr(true), - HookCustomAccessTokenUri: cast.Ptr(""), - HookCustomAccessTokenSecrets: cast.Ptr("b613679a0814d9ec772f95d778c35fc5ff1697c493715653c6c712144292c5ad"), + HookCustomAccessTokenUri: cast.Ptr("http://example.com"), + HookCustomAccessTokenSecrets: cast.Ptr("ce62bb9bcced294fd4afe668f8ab3b50a89cf433093c526fffa3d0e46bf55252"), HookSendEmailEnabled: cast.Ptr(true), - HookSendEmailUri: cast.Ptr(""), - HookSendEmailSecrets: cast.Ptr("b613679a0814d9ec772f95d778c35fc5ff1697c493715653c6c712144292c5ad"), + HookSendEmailUri: cast.Ptr("https://example.com"), + HookSendEmailSecrets: cast.Ptr("ce62bb9bcced294fd4afe668f8ab3b50a89cf433093c526fffa3d0e46bf55252"), HookSendSmsEnabled: cast.Ptr(true), - HookSendSmsUri: cast.Ptr(""), - HookSendSmsSecrets: cast.Ptr("b613679a0814d9ec772f95d778c35fc5ff1697c493715653c6c712144292c5ad"), + HookSendSmsUri: cast.Ptr("http://example.com"), + HookSendSmsSecrets: cast.Ptr("ce62bb9bcced294fd4afe668f8ab3b50a89cf433093c526fffa3d0e46bf55252"), HookMfaVerificationAttemptEnabled: cast.Ptr(true), - HookMfaVerificationAttemptUri: cast.Ptr(""), - HookMfaVerificationAttemptSecrets: cast.Ptr("b613679a0814d9ec772f95d778c35fc5ff1697c493715653c6c712144292c5ad"), + HookMfaVerificationAttemptUri: cast.Ptr("https://example.com"), + HookMfaVerificationAttemptSecrets: cast.Ptr("ce62bb9bcced294fd4afe668f8ab3b50a89cf433093c526fffa3d0e46bf55252"), HookPasswordVerificationAttemptEnabled: cast.Ptr(true), - HookPasswordVerificationAttemptUri: cast.Ptr(""), - HookPasswordVerificationAttemptSecrets: cast.Ptr("b613679a0814d9ec772f95d778c35fc5ff1697c493715653c6c712144292c5ad"), + HookPasswordVerificationAttemptUri: cast.Ptr("pg-functions://verifyPassword"), }) // Check error assert.NoError(t, err) @@ -153,17 +171,41 @@ func TestHookDiff(t *testing.T) { t.Run("local enabled and disabled", func(t *testing.T) { c := newWithDefaults() c.Hook = hook{ - CustomAccessToken: hookConfig{Enabled: true}, - MFAVerificationAttempt: hookConfig{Enabled: false}, + CustomAccessToken: hookConfig{ + Enabled: true, + URI: "http://example.com", + Secrets: "test-secret", + }, + SendSMS: hookConfig{ + Enabled: false, + URI: "https://example.com", + Secrets: "test-secret", + }, + SendEmail: hookConfig{ + Enabled: true, + URI: "pg-functions://sendEmail", + }, + MFAVerificationAttempt: hookConfig{ + Enabled: false, + URI: "pg-functions://verifyMFA", + }, + PasswordVerificationAttempt: hookConfig{Enabled: false}, } // Run test diff, err := c.DiffWithRemote("", v1API.AuthConfigResponse{ - HookCustomAccessTokenEnabled: cast.Ptr(false), - HookCustomAccessTokenUri: cast.Ptr(""), - HookCustomAccessTokenSecrets: cast.Ptr("b613679a0814d9ec772f95d778c35fc5ff1697c493715653c6c712144292c5ad"), - HookMfaVerificationAttemptEnabled: cast.Ptr(true), - HookMfaVerificationAttemptUri: cast.Ptr(""), - HookMfaVerificationAttemptSecrets: cast.Ptr("b613679a0814d9ec772f95d778c35fc5ff1697c493715653c6c712144292c5ad"), + HookCustomAccessTokenEnabled: cast.Ptr(false), + HookCustomAccessTokenUri: cast.Ptr(""), + HookCustomAccessTokenSecrets: cast.Ptr("b613679a0814d9ec772f95d778c35fc5ff1697c493715653c6c712144292c5ad"), + HookSendEmailEnabled: cast.Ptr(false), + HookSendEmailUri: cast.Ptr(""), + HookSendSmsEnabled: cast.Ptr(true), + HookSendSmsUri: cast.Ptr("http://example.com"), + HookSendSmsSecrets: cast.Ptr("ce62bb9bcced294fd4afe668f8ab3b50a89cf433093c526fffa3d0e46bf55252"), + HookMfaVerificationAttemptEnabled: cast.Ptr(true), + HookMfaVerificationAttemptUri: cast.Ptr("pg-functions://verifyMFA"), + HookPasswordVerificationAttemptEnabled: cast.Ptr(true), + HookPasswordVerificationAttemptUri: cast.Ptr("https://example.com"), + HookPasswordVerificationAttemptSecrets: cast.Ptr("ce62bb9bcced294fd4afe668f8ab3b50a89cf433093c526fffa3d0e46bf55252"), }) // Check error assert.NoError(t, err) diff --git a/pkg/config/config.go b/pkg/config/config.go index 8d809c595..f5a56dfc0 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -993,14 +993,25 @@ func (h *hookConfig) validate(hookType string) (err error) { return nil } if h.URI == "" { - return errors.Errorf("missing required field in config: auth.hook.%s.uri", hookType) - } else if parsed, err := url.Parse(h.URI); err != nil { + return errors.Errorf("Missing required field in config: auth.hook.%s.uri", hookType) + } + parsed, err := url.Parse(h.URI) + if err != nil { return errors.Errorf("failed to parse template url: %w", err) - } else if !(parsed.Scheme == "http" || parsed.Scheme == "https" || parsed.Scheme == "pg-functions") { - return errors.Errorf("Invalid HTTP hook config: auth.hook.%v should be a Postgres function URI, or a HTTP or HTTPS URL", hookType) } - if h.Secrets, err = maybeLoadEnv(h.Secrets); err != nil { - return errors.Errorf("missing required field in config: auth.hook.%s.secrets", hookType) + switch strings.ToLower(parsed.Scheme) { + case "http", "https": + if len(h.Secrets) == 0 { + return errors.Errorf("Missing required field in config: auth.hook.%s.secrets", hookType) + } else if h.Secrets, err = maybeLoadEnv(h.Secrets); err != nil { + return err + } + case "pg-functions": + if len(h.Secrets) > 0 { + return errors.Errorf("Invalid hook config: auth.hook.%s.secrets is unsupported for pg-functions URI", hookType) + } + default: + return errors.Errorf("Invalid hook config: auth.hook.%v should be a HTTP, HTTPS, or pg-functions URI", hookType) } return nil } @@ -1070,19 +1081,16 @@ func (c *tpaCognito) issuerURL() string { return fmt.Sprintf("https://cognito-idp.%s.amazonaws.com/%s", c.UserPoolRegion, c.UserPoolID) } -func (c *tpaCognito) validate() error { +func (c *tpaCognito) validate() (err error) { if c.UserPoolID == "" { return errors.New("Invalid config: auth.third_party.cognito is enabled but without a user_pool_id.") - } - var err error - if c.UserPoolID, err = maybeLoadEnv(c.UserPoolID); err != nil { + } else if c.UserPoolID, err = maybeLoadEnv(c.UserPoolID); err != nil { return err } if c.UserPoolRegion == "" { return errors.New("Invalid config: auth.third_party.cognito is enabled but without a user_pool_region.") - } - if c.UserPoolRegion, err = maybeLoadEnv(c.UserPoolRegion); err != nil { + } else if c.UserPoolRegion, err = maybeLoadEnv(c.UserPoolRegion); err != nil { return err } diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 8f1169d5e..4aa7ce243 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -205,55 +205,74 @@ func TestSigningJWT(t *testing.T) { func TestValidateHookURI(t *testing.T) { tests := []struct { - name string - uri string - hookName string - shouldErr bool - errorMsg string + hookConfig + name string + errorMsg string }{ { - name: "valid http URL", - uri: "http://example.com", - hookName: "testHook", - shouldErr: false, + name: "valid http URL", + hookConfig: hookConfig{ + Enabled: true, + URI: "http://example.com", + Secrets: "test-secret", + }, }, { - name: "valid https URL", - uri: "https://example.com", - hookName: "testHook", - shouldErr: false, + name: "valid https URL", + hookConfig: hookConfig{ + Enabled: true, + URI: "https://example.com", + Secrets: "test-secret", + }, + }, + { + name: "valid pg-functions URI", + hookConfig: hookConfig{ + Enabled: true, + URI: "pg-functions://functionName", + }, + }, + { + name: "invalid URI with unsupported scheme", + hookConfig: hookConfig{ + Enabled: true, + URI: "ftp://example.com", + Secrets: "test-secret", + }, + errorMsg: "Invalid hook config: auth.hook.invalid URI with unsupported scheme should be a HTTP, HTTPS, or pg-functions URI", }, { - name: "valid pg-functions URI", - uri: "pg-functions://functionName", - hookName: "pgHook", - shouldErr: false, + name: "invalid URI with parsing error", + hookConfig: hookConfig{ + Enabled: true, + URI: "http://a b.com", + Secrets: "test-secret", + }, + errorMsg: "failed to parse template url: parse \"http://a b.com\": invalid character \" \" in host name", }, { - name: "invalid URI with unsupported scheme", - uri: "ftp://example.com", - hookName: "malformedHook", - shouldErr: true, - errorMsg: "Invalid HTTP hook config: auth.hook.malformedHook should be a Postgres function URI, or a HTTP or HTTPS URL", + name: "valid http URL with missing secrets", + hookConfig: hookConfig{ + Enabled: true, + URI: "http://example.com", + }, + errorMsg: "Missing required field in config: auth.hook.valid http URL with missing secrets.secrets", }, { - name: "invalid URI with parsing error", - uri: "http://a b.com", - hookName: "errorHook", - shouldErr: true, - errorMsg: "failed to parse template url: parse \"http://a b.com\": invalid character \" \" in host name", + name: "valid pg-functions URI with unsupported secrets", + hookConfig: hookConfig{ + Enabled: true, + URI: "pg-functions://functionName", + Secrets: "test-secret", + }, + errorMsg: "Invalid hook config: auth.hook.valid pg-functions URI with unsupported secrets.secrets is unsupported for pg-functions URI", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - h := hookConfig{ - Enabled: true, - URI: tt.uri, - Secrets: "test-secret", - } - err := h.validate(tt.hookName) - if tt.shouldErr { + err := tt.hookConfig.validate(tt.name) + if len(tt.errorMsg) > 0 { assert.Error(t, err, "Expected an error for %v", tt.name) assert.EqualError(t, err, tt.errorMsg, "Expected error message does not match for %v", tt.name) } else { diff --git a/pkg/config/testdata/TestHookDiff/local_enabled_and_disabled.diff b/pkg/config/testdata/TestHookDiff/local_enabled_and_disabled.diff index b21cd4073..e3afeb491 100644 --- a/pkg/config/testdata/TestHookDiff/local_enabled_and_disabled.diff +++ b/pkg/config/testdata/TestHookDiff/local_enabled_and_disabled.diff @@ -1,21 +1,36 @@ diff remote[auth] local[auth] --- remote[auth] +++ local[auth] -@@ -11,7 +11,7 @@ +@@ -11,24 +11,24 @@ [hook] [hook.mfa_verification_attempt] -enabled = true +enabled = false - uri = "" + uri = "pg-functions://verifyMFA" secrets = "" [hook.password_verification_attempt] -@@ -19,7 +19,7 @@ +-enabled = true ++enabled = false uri = "" secrets = "" [hook.custom_access_token] -enabled = false +-uri = "" +-secrets = "hash:b613679a0814d9ec772f95d778c35fc5ff1697c493715653c6c712144292c5ad" +enabled = true - uri = "" - secrets = "hash:b613679a0814d9ec772f95d778c35fc5ff1697c493715653c6c712144292c5ad" ++uri = "http://example.com" ++secrets = "hash:ce62bb9bcced294fd4afe668f8ab3b50a89cf433093c526fffa3d0e46bf55252" [hook.send_sms] +-enabled = true ++enabled = false + uri = "https://example.com" + secrets = "test-secret" + [hook.send_email] +-enabled = false +-uri = "" ++enabled = true ++uri = "pg-functions://sendEmail" + secrets = "" + + [mfa]