[CI] Add gocritic linter (#977)

* [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 <clement.michaud34@gmail.com>

.golangci.yml

@@ -16,6 +16,7 @@ linters:
   enable:
     - asciicheck
     - goconst
+    - gocritic
     - gocyclo
     - godot
     - gofmt

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")
 	}
 }

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")
 	}
 

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"

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`")
 	}
 

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")
 	}
 

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))

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)
 			}

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,
 }

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"
 

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 == "" {

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)
 	}
 }

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
 				}

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

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)
 	}