From 8689b11e7c3a446273fea0a0d59af9744a8d7393 Mon Sep 17 00:00:00 2001 From: Dmitry Verkhoturov Date: Sun, 30 Jan 2022 14:27:51 +0300 Subject: [PATCH] log when deprecated and new args are set at the same time For example, when notify.telegram.token and telegram.token are both set but to different values, user might see "access denied" error in log on attempt to send telegram notification, thinking that notify.telegram.token value is used, when in fact it is ignored and only telegram.token is used. New behavior is the same, ignoring the old param when new one is set, but issuing the error log message which explicitly tells the user about that. Resolves #1218. --- backend/app/cmd/cmd.go | 5 +++ backend/app/cmd/server.go | 52 +++++++++++++++++++++++++--- backend/app/cmd/server_test.go | 62 ++++++++++++++++++++++++++++++++++ backend/app/main.go | 3 ++ 4 files changed, 118 insertions(+), 4 deletions(-) diff --git a/backend/app/cmd/cmd.go b/backend/app/cmd/cmd.go index 4d257192..534c1788 100644 --- a/backend/app/cmd/cmd.go +++ b/backend/app/cmd/cmd.go @@ -22,6 +22,7 @@ type CommonOptionsCommander interface { SetCommon(commonOpts CommonOpts) Execute(args []string) error HandleDeprecatedFlags() []DeprecatedFlag + FindDeprecatedFlagsCollisions() []DeprecatedFlag } // CommonOpts sets externally from main, shared across all commands @@ -49,6 +50,10 @@ func (c *CommonOpts) SetCommon(commonOpts CommonOpts) { // HandleDeprecatedFlags sets new flags from deprecated and returns their list func (c *CommonOpts) HandleDeprecatedFlags() []DeprecatedFlag { return nil } +// FindDeprecatedFlagsCollisions returns list of flags collisions, e.g. when both deprecated +// and new flags are set to non-default values, and different from each other. +func (c *CommonOpts) FindDeprecatedFlagsCollisions() []DeprecatedFlag { return nil } + // fileParser used to convert template strings like blah-{{.SITE}}-{{.YYYYMMDD}} the final format type fileParser struct { site string diff --git a/backend/app/cmd/server.go b/backend/app/cmd/server.go index 89b2b919..06ebcee1 100644 --- a/backend/app/cmd/server.go +++ b/backend/app/cmd/server.go @@ -321,7 +321,9 @@ func (s *ServerCommand) Execute(_ []string) error { return nil } -// HandleDeprecatedFlags sets new flags from deprecated returns their list +// HandleDeprecatedFlags sets new flags from deprecated returns their list. +// Returned list has DeprecatedFlag.Old and DeprecatedFlag.Version set, and DeprecatedFlag.New is optional +// (as some entries are removed without substitute). func (s *ServerCommand) HandleDeprecatedFlags() (result []DeprecatedFlag) { if s.Auth.Email.Host != "" && s.SMTP.Host == "" { s.SMTP.Host = s.Auth.Email.Host @@ -343,7 +345,8 @@ func (s *ServerCommand) HandleDeprecatedFlags() (result []DeprecatedFlag) { s.SMTP.Password = s.Auth.Email.SMTPPassword result = append(result, DeprecatedFlag{Old: "auth.email.passwd", New: "smtp.password", Version: "1.5"}) } - if s.Auth.Email.TimeOut != 10*time.Second && s.SMTP.TimeOut == 10*time.Second { + const emailDefaultTimout = 10 * time.Second + if s.Auth.Email.TimeOut != emailDefaultTimout && s.SMTP.TimeOut == emailDefaultTimout { s.SMTP.TimeOut = s.Auth.Email.TimeOut result = append(result, DeprecatedFlag{Old: "auth.email.timeout", New: "smtp.timeout", Version: "1.5"}) } @@ -368,8 +371,8 @@ func (s *ServerCommand) HandleDeprecatedFlags() (result []DeprecatedFlag) { s.Telegram.Token = s.Notify.Telegram.Token result = append(result, DeprecatedFlag{Old: "notify.telegram.token", New: "telegram.token", Version: "1.9"}) } - const telegramDefaultDuration = time.Second * 5 - if s.Notify.Telegram.Timeout != telegramDefaultDuration && s.Telegram.Timeout == telegramDefaultDuration { + const telegramDefaultTimeout = time.Second * 5 + if s.Notify.Telegram.Timeout != telegramDefaultTimeout && s.Telegram.Timeout == telegramDefaultTimeout { s.Telegram.Timeout = s.Notify.Telegram.Timeout result = append(result, DeprecatedFlag{Old: "notify.telegram.timeout", New: "telegram.timeout", Version: "1.9"}) } @@ -379,6 +382,40 @@ func (s *ServerCommand) HandleDeprecatedFlags() (result []DeprecatedFlag) { return result } +// FindDeprecatedFlagsCollisions returns flags which are set both old (deprecated) and new way, +// which means new ones are used and old ones are ignored by deprecated flag handler. +// It returns DeprecatedFlag list which always has only DeprecatedFlag.Old and DeprecatedFlag.New set. +func (s *ServerCommand) FindDeprecatedFlagsCollisions() (result []DeprecatedFlag) { + if stringsSetAndDifferent(s.Auth.Email.Host, s.SMTP.Host) { + result = append(result, DeprecatedFlag{Old: "auth.email.host", New: "smtp.host"}) + } + if s.Auth.Email.Port != 0 && s.SMTP.Port != 0 && s.Auth.Email.Port != s.SMTP.Port { + result = append(result, DeprecatedFlag{Old: "auth.email.port", New: "smtp.port"}) + } + if stringsSetAndDifferent(s.Auth.Email.SMTPUserName, s.SMTP.Username) { + result = append(result, DeprecatedFlag{Old: "auth.email.user", New: "smtp.username"}) + } + if stringsSetAndDifferent(s.Auth.Email.SMTPPassword, s.SMTP.Password) { + result = append(result, DeprecatedFlag{Old: "auth.email.passwd", New: "smtp.password"}) + } + const emailDefaultTimout = 10 * time.Second + if s.Auth.Email.TimeOut != emailDefaultTimout && s.SMTP.TimeOut != emailDefaultTimout && s.Auth.Email.TimeOut != s.SMTP.TimeOut { + result = append(result, DeprecatedFlag{Old: "auth.email.timeout", New: "smtp.timeout"}) + } + if !(len(s.Notify.Type) == 1 && contains("none", s.Notify.Type)) && // default, "none" notify type + (len(s.Notify.Users) != 0 || len(s.Notify.Admins) != 0) { // new notify param(s) are used, old ones will be ignored + result = append(result, DeprecatedFlag{Old: "notify.type", New: "notify.(users|admins)"}) + } + if stringsSetAndDifferent(s.Notify.Telegram.Token, s.Telegram.Token) { + result = append(result, DeprecatedFlag{Old: "notify.telegram.token", New: "telegram.token"}) + } + const telegramDefaultTimeout = time.Second * 5 + if s.Notify.Telegram.Timeout != telegramDefaultTimeout && s.Telegram.Timeout != telegramDefaultTimeout && s.Notify.Telegram.Timeout != s.Telegram.Timeout { + result = append(result, DeprecatedFlag{Old: "notify.telegram.timeout", New: "telegram.timeout"}) + } + return result +} + func (s *ServerCommand) handleDeprecatedNotifications() { for _, t := range s.Notify.Type { if t == "email" && !contains(t, s.Notify.Users) { @@ -390,6 +427,13 @@ func (s *ServerCommand) handleDeprecatedNotifications() { } } +func stringsSetAndDifferent(s1, s2 string) bool { + if s1 != "" && s2 != "" && s1 != s2 { + return true + } + return false +} + func contains(s string, a []string) bool { for _, t := range a { if t == s { diff --git a/backend/app/cmd/server_test.go b/backend/app/cmd/server_test.go index 074dce1b..6437263f 100644 --- a/backend/app/cmd/server_test.go +++ b/backend/app/cmd/server_test.go @@ -410,7 +410,10 @@ func TestServerApp_DeprecatedArgs(t *testing.T) { p := flags.NewParser(&s, flags.Default) args := []string{ "test", + "--notify.type=email", "--notify.type=telegram", + "--img-proxy", + "--notify.email.notify_admin", "--auth.email.host=smtp.example.org", "--auth.email.port=666", "--auth.email.tls", @@ -420,6 +423,7 @@ func TestServerApp_DeprecatedArgs(t *testing.T) { "--auth.email.template=file.tmpl", "--notify.telegram.token=abcd", "--notify.telegram.timeout=3m", + "--notify.telegram.api=http://example.org", } assert.Empty(t, s.SMTP.Host) assert.Empty(t, s.SMTP.Port) @@ -439,9 +443,12 @@ func TestServerApp_DeprecatedArgs(t *testing.T) { {Old: "auth.email.passwd", New: "smtp.password", Version: "1.5"}, {Old: "auth.email.timeout", New: "smtp.timeout", Version: "1.5"}, {Old: "auth.email.template", Version: "1.5"}, + {Old: "img-proxy", New: "image-proxy.http2https", Version: "1.5"}, + {Old: "notify.email.notify_admin", New: "notify.admins=email", Version: "1.9"}, {Old: "notify.type", New: "notify.(users|admins)", Version: "1.9"}, {Old: "notify.telegram.token", New: "telegram.token", Version: "1.9"}, {Old: "notify.telegram.timeout", New: "telegram.timeout", Version: "1.9"}, + {Old: "notify.telegram.api", Version: "1.9"}, }, deprecatedFlags) assert.Equal(t, "smtp.example.org", s.SMTP.Host) @@ -452,6 +459,61 @@ func TestServerApp_DeprecatedArgs(t *testing.T) { assert.Equal(t, 15*time.Second, s.SMTP.TimeOut) } +func TestServerApp_DeprecatedArgsCollisions(t *testing.T) { + s := ServerCommand{} + s.SetCommon(CommonOpts{RemarkURL: "https://demo.remark42.com", SharedSecret: "123456"}) + + p := flags.NewParser(&s, flags.Default) + args := []string{ + "test", + "--auth.email.host=smtp-old.example.org", + "--smtp.host=smtp-new.example.org", + "--auth.email.port=666", + "--smtp.port=999", + "--auth.email.user=test_user", + "--smtp.username=new_test_user", + "--auth.email.passwd=test_password", + "--smtp.password=new_test_password", + "--auth.email.timeout=15s", + "--smtp.timeout=20s", + "--notify.type=telegram", + "--notify.users=telegram", + "--notify.telegram.token=abcd", + "--telegram.token=dcba", + "--notify.telegram.timeout=3m", + "--telegram.timeout=5m", + } + _, err := p.ParseArgs(args) + require.NoError(t, err) + deprecatedFlagsCollisions := s.FindDeprecatedFlagsCollisions() + assert.ElementsMatch(t, + []DeprecatedFlag{ + {Old: "notify.type", New: "notify.(users|admins)"}, + {Old: "auth.email.host", New: "smtp.host"}, + {Old: "auth.email.port", New: "smtp.port"}, + {Old: "auth.email.user", New: "smtp.username"}, + {Old: "auth.email.passwd", New: "smtp.password"}, + {Old: "auth.email.timeout", New: "smtp.timeout"}, + {Old: "notify.telegram.token", New: "telegram.token"}, + {Old: "notify.telegram.timeout", New: "telegram.timeout"}, + }, + deprecatedFlagsCollisions) + + // case which should return nothing + s = ServerCommand{} + s.SetCommon(CommonOpts{RemarkURL: "https://demo.remark42.com", SharedSecret: "123456"}) + p = flags.NewParser(&s, flags.Default) + args = []string{ + "test", + "--auth.email.host=smtp-old.example.org", + "--smtp.host=''", + } + _, err = p.ParseArgs(args) + require.NoError(t, err) + deprecatedFlagsCollisions = s.FindDeprecatedFlagsCollisions() + assert.Empty(t, []DeprecatedFlag{}, deprecatedFlagsCollisions) +} + func Test_ACMEEmail(t *testing.T) { cmd := ServerCommand{} cmd.SetCommon(CommonOpts{RemarkURL: "https://remark.com:443", SharedSecret: "123456"}) diff --git a/backend/app/main.go b/backend/app/main.go index 2371349c..77eeac9f 100644 --- a/backend/app/main.go +++ b/backend/app/main.go @@ -45,6 +45,9 @@ func main() { SharedSecret: opts.SharedSecret, Revision: revision, }) + for _, entry := range c.FindDeprecatedFlagsCollisions() { + log.Print(fmt.Sprintf("[ERROR] deprecated --%s and new --%s options are set to different values, old one is ignored: please remove it", entry.Old, entry.New)) + } for _, entry := range c.HandleDeprecatedFlags() { deprecationNote := fmt.Sprintf("[WARN] --%s is deprecated since v%s and will be removed in the future", entry.Old, entry.Version) if entry.New != "" {