From cc06ab6c18acbf8d2d1b33d8af15d77b7b31f216 Mon Sep 17 00:00:00 2001 From: Amir Zarrinkafsh Date: Wed, 6 May 2020 10:52:06 +1000 Subject: [PATCH] [CI] Add gocritic linter (#977) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [CI] Add gocritic linter * Implement gocritic recommendations The outstanding recommendations are due to be addressed in #959 and #971 respectively. * Fix implementation tests * Fix remaining linting issues. * Fix tests. Co-authored-by: Clément Michaud --- .golangci.yml | 1 + cmd/authelia-scripts/cmd_docker.go | 22 ++++++++-------- cmd/authelia/main.go | 25 +++++++++++-------- internal/authentication/const.go | 3 ++- internal/authentication/file_user_provider.go | 8 +++--- internal/authentication/password_hash.go | 18 ++++++++----- internal/authentication/password_hash_test.go | 2 +- internal/authorization/authorizer.go | 5 ++-- .../configuration/schema/authentication.go | 4 +-- internal/configuration/schema/const.go | 2 ++ .../configuration/validator/authentication.go | 13 +++++----- internal/configuration/validator/storage.go | 7 +++--- internal/middlewares/identity_verification.go | 7 +++--- internal/notification/smtp_notifier.go | 15 ++++++----- internal/utils/time.go | 9 ++++--- 15 files changed, 84 insertions(+), 57 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 49b0d229..25132395 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -16,6 +16,7 @@ linters: enable: - asciicheck - goconst + - gocritic - gocyclo - godot - gofmt diff --git a/cmd/authelia-scripts/cmd_docker.go b/cmd/authelia-scripts/cmd_docker.go index 1d9e9942..a9a4c561 100644 --- a/cmd/authelia-scripts/cmd_docker.go +++ b/cmd/authelia-scripts/cmd_docker.go @@ -202,7 +202,8 @@ func deployManifest(docker *Docker, tag string, amd64tag string, arm32v7tag stri func publishDockerImage(arch string) { docker := &Docker{} - if ciTag != "" { + switch { + case ciTag != "": if len(tags) == 4 { log.Infof("Detected tags: '%s' | '%s' | '%s'", tags[1], tags[2], tags[3]) login(docker) @@ -216,16 +217,16 @@ func publishDockerImage(arch string) { } else { log.Fatal("Docker image will not be published, the specified tag does not conform to the standard") } - } else if ciBranch != masterTag && !publicRepo.MatchString(ciBranch) { + case ciBranch != masterTag && !publicRepo.MatchString(ciBranch): login(docker) deploy(docker, ciBranch+"-"+arch) - } else if ciBranch != masterTag && publicRepo.MatchString(ciBranch) { + case ciBranch != masterTag && publicRepo.MatchString(ciBranch): login(docker) deploy(docker, "PR"+ciPullRequest+"-"+arch) - } else if ciBranch == masterTag && ciPullRequest == stringFalse { + case ciBranch == masterTag && ciPullRequest == stringFalse: login(docker) deploy(docker, "master-"+arch) - } else { + default: log.Info("Docker image will not be published") } } @@ -233,7 +234,8 @@ func publishDockerImage(arch string) { func publishDockerManifest() { docker := &Docker{} - if ciTag != "" { + switch { + case ciTag != "": if len(tags) == 4 { log.Infof("Detected tags: '%s' | '%s' | '%s'", tags[1], tags[2], tags[3]) login(docker) @@ -250,17 +252,17 @@ func publishDockerManifest() { } else { log.Fatal("Docker manifest will not be published, the specified tag does not conform to the standard") } - } else if ciBranch != masterTag && !publicRepo.MatchString(ciBranch) { + case ciBranch != masterTag && !publicRepo.MatchString(ciBranch): login(docker) deployManifest(docker, ciBranch, ciBranch+"-amd64", ciBranch+"-arm32v7", ciBranch+"-arm64v8") - } else if ciBranch != masterTag && publicRepo.MatchString(ciBranch) { + case ciBranch != masterTag && publicRepo.MatchString(ciBranch): login(docker) deployManifest(docker, "PR"+ciPullRequest, "PR"+ciPullRequest+"-amd64", "PR"+ciPullRequest+"-arm32v7", "PR"+ciPullRequest+"-arm64v8") - } else if ciBranch == masterTag && ciPullRequest == stringFalse { + case ciBranch == masterTag && ciPullRequest == stringFalse: login(docker) deployManifest(docker, "master", "master-amd64", "master-arm32v7", "master-arm64v8") publishDockerReadme(docker) - } else { + default: log.Info("Docker manifest will not be published") } } diff --git a/cmd/authelia/main.go b/cmd/authelia/main.go index 0b49a892..e234feed 100644 --- a/cmd/authelia/main.go +++ b/cmd/authelia/main.go @@ -63,31 +63,36 @@ func startServer() { var userProvider authentication.UserProvider - if config.AuthenticationBackend.File != nil { + switch { + case config.AuthenticationBackend.File != nil: userProvider = authentication.NewFileUserProvider(config.AuthenticationBackend.File) - } else if config.AuthenticationBackend.Ldap != nil { + case config.AuthenticationBackend.Ldap != nil: userProvider = authentication.NewLDAPUserProvider(*config.AuthenticationBackend.Ldap) - } else { + default: log.Fatalf("Unrecognized authentication backend") } var storageProvider storage.Provider - if config.Storage.PostgreSQL != nil { + + switch { + case config.Storage.PostgreSQL != nil: storageProvider = storage.NewPostgreSQLProvider(*config.Storage.PostgreSQL) - } else if config.Storage.MySQL != nil { + case config.Storage.MySQL != nil: storageProvider = storage.NewMySQLProvider(*config.Storage.MySQL) - } else if config.Storage.Local != nil { + case config.Storage.Local != nil: storageProvider = storage.NewSQLiteProvider(config.Storage.Local.Path) - } else { + default: log.Fatalf("Unrecognized storage backend") } var notifier notification.Notifier - if config.Notifier.SMTP != nil { + + switch { + case config.Notifier.SMTP != nil: notifier = notification.NewSMTPNotifier(*config.Notifier.SMTP) - } else if config.Notifier.FileSystem != nil { + case config.Notifier.FileSystem != nil: notifier = notification.NewFileNotifier(*config.Notifier.FileSystem) - } else { + default: log.Fatalf("Unrecognized notifier") } diff --git a/internal/authentication/const.go b/internal/authentication/const.go index 46ac1a72..b929736a 100644 --- a/internal/authentication/const.go +++ b/internal/authentication/const.go @@ -33,7 +33,7 @@ type CryptAlgo string const ( // HashingAlgorithmArgon2id Argon2id hash identifier. - HashingAlgorithmArgon2id CryptAlgo = "argon2id" + HashingAlgorithmArgon2id CryptAlgo = argon2id // HashingAlgorithmSHA512 SHA512 hash identifier. HashingAlgorithmSHA512 CryptAlgo = "6" ) @@ -54,6 +54,7 @@ var HashingPossibleSaltCharacters = []rune("abcdefghijklmnopqrstuvwxyzABCDEFGHIJ // ErrUserNotFound indicates the user wasn't found in the authentication backend. var ErrUserNotFound = errors.New("user not found") +const argon2id = "argon2id" const sha512 = "sha512" const testPassword = "my;secure*password" diff --git a/internal/authentication/file_user_provider.go b/internal/authentication/file_user_provider.go index bbfa9da9..90b2083d 100644 --- a/internal/authentication/file_user_provider.go +++ b/internal/authentication/file_user_provider.go @@ -149,11 +149,13 @@ func (p *FileUserProvider) UpdatePassword(username string, newPassword string) e } var algorithm CryptAlgo - if p.configuration.Password.Algorithm == "argon2id" { + + switch p.configuration.Password.Algorithm { + case argon2id: algorithm = HashingAlgorithmArgon2id - } else if p.configuration.Password.Algorithm == sha512 { + case sha512: algorithm = HashingAlgorithmSHA512 - } else { + default: return errors.New("Invalid algorithm in configuration. It should be `argon2id` or `sha512`") } diff --git a/internal/authentication/password_hash.go b/internal/authentication/password_hash.go index c2e87aea..a966b657 100644 --- a/internal/authentication/password_hash.go +++ b/internal/authentication/password_hash.go @@ -48,23 +48,26 @@ func ParseHash(hash string) (passwordHash *PasswordHash, err error) { return nil, errors.New("Salt contains invalid base64 characters") } - if code == HashingAlgorithmSHA512 { + switch code { + case HashingAlgorithmSHA512: h.Iterations = parameters.GetInt("rounds", HashingDefaultSHA512Iterations) h.Algorithm = HashingAlgorithmSHA512 if parameters["rounds"] != "" && parameters["rounds"] != strconv.Itoa(h.Iterations) { return nil, fmt.Errorf("SHA512 iterations is not numeric (%s)", parameters["rounds"]) } - } else if code == HashingAlgorithmArgon2id { + case HashingAlgorithmArgon2id: version := parameters.GetInt("v", 0) if version < 19 { if version == 0 { return nil, fmt.Errorf("Argon2id version parameter not found (%s)", hash) } + return nil, fmt.Errorf("Argon2id versions less than v19 are not supported (hash is version %d)", version) } else if version > 19 { return nil, fmt.Errorf("Argon2id versions greater than v19 are not supported (hash is version %d)", version) } + h.Algorithm = HashingAlgorithmArgon2id h.Memory = parameters.GetInt("m", HashingDefaultArgon2idMemory) h.Iterations = parameters.GetInt("t", HashingDefaultArgon2idTime) @@ -72,13 +75,15 @@ func ParseHash(hash string) (passwordHash *PasswordHash, err error) { h.KeyLength = parameters.GetInt("k", HashingDefaultArgon2idKeyLength) decodedKey, err := crypt.Base64Encoding.DecodeString(h.Key) + if err != nil { return nil, errors.New("Hash key contains invalid base64 characters") } + if len(decodedKey) != h.KeyLength { return nil, fmt.Errorf("Argon2id key length parameter (%d) does not match the actual key length (%d)", h.KeyLength, len(decodedKey)) } - } else { + default: return nil, fmt.Errorf("Authelia only supports salted SHA512 hashing ($6$) and salted argon2id ($argon2id$), not $%s$", code) } @@ -159,11 +164,12 @@ func CheckPassword(password, hash string) (ok bool, err error) { } func getCryptSettings(salt string, algorithm CryptAlgo, iterations, memory, parallelism, keyLength int) (settings string) { - if algorithm == HashingAlgorithmArgon2id { + switch algorithm { + case HashingAlgorithmArgon2id: settings, _ = crypt.Argon2idSettings(memory, iterations, parallelism, keyLength, salt) - } else if algorithm == HashingAlgorithmSHA512 { + case HashingAlgorithmSHA512: settings = fmt.Sprintf("$6$rounds=%d$%s", iterations, salt) - } else { + default: panic("invalid password hashing algorithm provided") } diff --git a/internal/authentication/password_hash_test.go b/internal/authentication/password_hash_test.go index 9017cee0..488f9380 100644 --- a/internal/authentication/password_hash_test.go +++ b/internal/authentication/password_hash_test.go @@ -36,7 +36,7 @@ func TestShouldHashArgon2idPassword(t *testing.T) { code, parameters, salt, key, err := crypt.DecodeSettings(hash) assert.NoError(t, err) - assert.Equal(t, "argon2id", code) + assert.Equal(t, argon2id, code) assert.Equal(t, "BpLnfgDsc2WD8F2q", salt) assert.Equal(t, "O126GHPeZ5fwj7OLSs7PndXsTbje76R+QW9/EGfhkJg", key) assert.Equal(t, schema.DefaultCIPasswordConfiguration.Iterations, parameters.GetInt("t", HashingDefaultArgon2idTime)) diff --git a/internal/authorization/authorizer.go b/internal/authorization/authorizer.go index 5395afdf..9f354102 100644 --- a/internal/authorization/authorizer.go +++ b/internal/authorization/authorizer.go @@ -47,13 +47,14 @@ func selectMatchingSubjectRules(rules []schema.ACLRule, subject Subject) []schem selectedRules := []schema.ACLRule{} for _, rule := range rules { - if len(rule.Subjects) > 0 { + switch { + case len(rule.Subjects) > 0: for _, subjectRule := range rule.Subjects { if isSubjectMatching(subject, subjectRule) && isIPMatching(subject.IP, rule.Networks) { selectedRules = append(selectedRules, rule) } } - } else { + default: if isIPMatching(subject.IP, rule.Networks) { selectedRules = append(selectedRules, rule) } diff --git a/internal/configuration/schema/authentication.go b/internal/configuration/schema/authentication.go index aa963523..65763a99 100644 --- a/internal/configuration/schema/authentication.go +++ b/internal/configuration/schema/authentication.go @@ -45,7 +45,7 @@ var DefaultPasswordConfiguration = PasswordConfiguration{ Iterations: 1, KeyLength: 32, SaltLength: 16, - Algorithm: "argon2id", + Algorithm: argon2id, Memory: 1024, Parallelism: 8, } @@ -55,7 +55,7 @@ var DefaultCIPasswordConfiguration = PasswordConfiguration{ Iterations: 1, KeyLength: 32, SaltLength: 16, - Algorithm: "argon2id", + Algorithm: argon2id, Memory: 128, Parallelism: 8, } diff --git a/internal/configuration/schema/const.go b/internal/configuration/schema/const.go index e8cef750..715862aa 100644 --- a/internal/configuration/schema/const.go +++ b/internal/configuration/schema/const.go @@ -6,6 +6,8 @@ import ( const denyPolicy = "deny" +const argon2id = "argon2id" + // ProfileRefreshDisabled represents a value for refresh_interval that disables the check entirely. const ProfileRefreshDisabled = "disable" diff --git a/internal/configuration/validator/authentication.go b/internal/configuration/validator/authentication.go index 6045e479..42f3d3ab 100644 --- a/internal/configuration/validator/authentication.go +++ b/internal/configuration/validator/authentication.go @@ -40,11 +40,12 @@ func validateFileAuthenticationBackend(configuration *schema.FileAuthenticationB } //Salt Length - if configuration.Password.SaltLength == 0 { + switch { + case configuration.Password.SaltLength == 0: configuration.Password.SaltLength = schema.DefaultPasswordConfiguration.SaltLength - } else if configuration.Password.SaltLength < 2 { + case configuration.Password.SaltLength < 2: validator.Push(fmt.Errorf("The salt length must be 2 or more, you configured %d", configuration.Password.SaltLength)) - } else if configuration.Password.SaltLength > 16 { + case configuration.Password.SaltLength > 16: validator.Push(fmt.Errorf("The salt length must be 16 or less, you configured %d", configuration.Password.SaltLength)) } @@ -137,10 +138,8 @@ func validateLdapAuthenticationBackend(configuration *schema.LDAPAuthenticationB if configuration.GroupsFilter == "" { validator.Push(errors.New("Please provide a groups filter with `groups_filter` attribute")) - } else { - if !strings.HasPrefix(configuration.GroupsFilter, "(") || !strings.HasSuffix(configuration.GroupsFilter, ")") { - validator.Push(errors.New("The groups filter should contain enclosing parenthesis. For instance cn={input} should be (cn={input})")) - } + } else if !strings.HasPrefix(configuration.GroupsFilter, "(") || !strings.HasSuffix(configuration.GroupsFilter, ")") { + validator.Push(errors.New("The groups filter should contain enclosing parenthesis. For instance cn={input} should be (cn={input})")) } if configuration.UsernameAttribute == "" { diff --git a/internal/configuration/validator/storage.go b/internal/configuration/validator/storage.go index 399dc915..f0033a17 100644 --- a/internal/configuration/validator/storage.go +++ b/internal/configuration/validator/storage.go @@ -12,11 +12,12 @@ func ValidateStorage(configuration schema.StorageConfiguration, validator *schem validator.Push(errors.New("A storage configuration must be provided. It could be 'local', 'mysql' or 'postgres'")) } - if configuration.MySQL != nil { + switch { + case configuration.MySQL != nil: validateSQLConfiguration(&configuration.MySQL.SQLStorageConfiguration, validator) - } else if configuration.PostgreSQL != nil { + case configuration.PostgreSQL != nil: validatePostgreSQLConfiguration(configuration.PostgreSQL, validator) - } else if configuration.Local != nil { + case configuration.Local != nil: validateLocalStorageConfiguration(configuration.Local, validator) } } diff --git a/internal/middlewares/identity_verification.go b/internal/middlewares/identity_verification.go index cebcd3bf..9bdb0094 100644 --- a/internal/middlewares/identity_verification.go +++ b/internal/middlewares/identity_verification.go @@ -130,14 +130,15 @@ func IdentityVerificationFinish(args IdentityVerificationFinishArgs, next func(c if err != nil { if ve, ok := err.(*jwt.ValidationError); ok { - if ve.Errors&jwt.ValidationErrorMalformed != 0 { + switch { + case ve.Errors&jwt.ValidationErrorMalformed != 0: ctx.Error(fmt.Errorf("Cannot parse token"), operationFailedMessage) return - } else if ve.Errors&(jwt.ValidationErrorExpired|jwt.ValidationErrorNotValidYet) != 0 { + case ve.Errors&(jwt.ValidationErrorExpired|jwt.ValidationErrorNotValidYet) != 0: // Token is either expired or not active yet ctx.Error(fmt.Errorf("Token expired"), identityVerificationTokenHasExpiredMessage) return - } else { + default: ctx.Error(fmt.Errorf("Cannot handle this token: %s", ve), operationFailedMessage) return } diff --git a/internal/notification/smtp_notifier.go b/internal/notification/smtp_notifier.go index e6f22e58..a0c15f3c 100644 --- a/internal/notification/smtp_notifier.go +++ b/internal/notification/smtp_notifier.go @@ -101,8 +101,8 @@ func (n *SMTPNotifier) startTLS() error { return nil } - ok, _ := n.client.Extension("STARTTLS") - if ok { + switch ok, _ := n.client.Extension("STARTTLS"); ok { + case true: log.Debugf("Notifier SMTP server supports STARTTLS (disableVerifyCert: %t, ServerName: %s), attempting", n.tlsConfig.InsecureSkipVerify, n.tlsConfig.ServerName) if err := n.client.StartTLS(n.tlsConfig); err != nil { @@ -110,10 +110,13 @@ func (n *SMTPNotifier) startTLS() error { } log.Debug("Notifier SMTP STARTTLS completed without error") - } else if n.disableRequireTLS { - log.Warn("Notifier SMTP server does not support STARTTLS and SMTP configuration is set to disable the TLS requirement (only useful for unauthenticated emails over plain text)") - } else { - return errors.New("Notifier SMTP server does not support TLS and it is required by default (see documentation if you want to disable this highly recommended requirement)") + default: + switch n.disableRequireTLS { + case true: + log.Warn("Notifier SMTP server does not support STARTTLS and SMTP configuration is set to disable the TLS requirement (only useful for unauthenticated emails over plain text)") + default: + return errors.New("Notifier SMTP server does not support TLS and it is required by default (see documentation if you want to disable this highly recommended requirement)") + } } return nil diff --git a/internal/utils/time.go b/internal/utils/time.go index b1f0c124..8c05dbc3 100644 --- a/internal/utils/time.go +++ b/internal/utils/time.go @@ -14,7 +14,9 @@ func ParseDurationString(input string) (time.Duration, error) { var duration time.Duration matches := parseDurationRegexp.FindStringSubmatch(input) - if len(matches) == 3 && matches[2] != "" { + + switch { + case len(matches) == 3 && matches[2] != "": d, _ := strconv.Atoi(matches[1]) switch matches[2] { @@ -33,13 +35,14 @@ func ParseDurationString(input string) (time.Duration, error) { case "s": duration = time.Duration(d) * time.Second } - } else if input == "0" || len(matches) == 3 { + case input == "0" || len(matches) == 3: seconds, err := strconv.Atoi(input) if err != nil { return 0, fmt.Errorf("Could not convert the input string of %s into a duration: %s", input, err) } + duration = time.Duration(seconds) * time.Second - } else if input != "" { + case input != "": // Throw this error if input is anything other than a blank string, blank string will default to a duration of nothing return 0, fmt.Errorf("Could not convert the input string of %s into a duration", input) }