Method to check header token if valid

Posted on

Problem

I’ve written the following function which reads an authorization header, compares it and returns whether it is valid or not.

The function works correctly:

func Auth(req *http.Request) bool{
    sHeader := strings.SplitN(req.Header.Get("authorization"), " ", 2)
    if sHeader[0] != "" {
        fmt.Printf("sHeader: ", sHeader[1])
        devEnv := os.Getenv("authorization")
        if (devEnv == sHeader[1]) {
            fmt.Println("valid")
            return true
        } else {
            fmt.Println("Not valid: " + sHeader[1])
            return false
        }
    } else {
        log.Fatal("Missing Authorization Header")
        return false
    }
}

What can I improve?

Solution

Here are my notes that I would bring to the code review.

package main

import (
    "fmt"
    "log"
    "net/http"
    "os"
    "strings"
)

func AuthCodeReview(req *http.Request) bool { // why bool? why not error
    sHeader := strings.SplitN(req.Header.Get("authorization"), " ", 3) // 3 for fields after 0 & 1?
    if len(sHeader) == 0 || sHeader[0] == "" {                         // check index in range
        log.Print("Missing Authorization Header") // never Fatal!
        return false
    }
    if len(sHeader) > 1 { // why 1? why ignore 0? check index in range
        fmt.Printf("sHeader: ", sHeader[1])  // debug?
        devEnv := os.Getenv("authorization") // LookupEnv?
        if devEnv == sHeader[1] {
            fmt.Println("valid") // debug?
            return true
        }
    }
    fmt.Println("Not valid: " + sHeader[1]) // debug?
    return false
}

func Auth(req *http.Request) bool {
    sHeader := strings.SplitN(req.Header.Get("authorization"), " ", 2)
    if sHeader[0] != "" {
        fmt.Printf("sHeader: ", sHeader[1])
        devEnv := os.Getenv("authorization")
        if devEnv == sHeader[1] {
            fmt.Println("valid")
            return true
        } else {
            fmt.Println("Not valid: " + sHeader[1])
            return false
        }
    } else {
        log.Fatal("Missing Authorization Header")
        return false
    }
}

func main() {}

Playground: https://play.golang.org/p/_MwBV80vWx



log.Fatal is equivalent to log.Print() followed by a call to
os.Exit(1).

os.Exit causes the current program to exit with the given status code.
The program terminates immediately; deferred functions are not run.

Your program is running 100,000 client connections using goroutines. One connection has a problem, so you crash the program. We have 100,000 very upset customers. Handle errors and fail gracefully; return errors; don’t panic or Exit.

See Errors.


fmt.Println("valid") // debug?

Your sysout is a massive clutter of messages. That may make sense for debugging, but it doesn’t make sense for production. What is the purpose of this and similar statements.


fmt.Println("Not valid: " + sHeader[1]) // debug?

If this message is not for debugging, it should be logged (log not fmt).


strings.SplitN The count determines the number of substrings to
return: n > 0: at most n substrings; the last substring will be the
unsplit remainder.

// 3 for fields after 0 & 1?
sHeader := strings.SplitN(req.Header.Get("authorization"), " ", 3) 

If you are expecting two fields SplitN(2) will return the first field in index 0 and any remainder in index 1. SplitN(3) will return the first field in index 0, the second field in index 1 and any remainder in index 2. Having a remainder may or may not be valid.

Playground: https://play.golang.org/p/IQA8iqIdN5


sHeader[1] // why 1? why ignore 0? 

You don’t say what the format of req.Header.Get("authorization") is. What does sHeader[1] represent. Add a comment to define sHeader and sHeader[1].


I reorganized your code.

Don’t write code as a heavily indented stream of consciousness. First, write the normal flow. The first step in a code review is to read and verify the normal flow, it should be obvious. Second, add exception handling. Third, add error handling. Sensibly minimize indentation.

See Indent Error Flow.

For example, a simple first draft,

func AuthFirstDraft(req *http.Request) error {
    // TODO: improve error detection and handling
    sHeader := strings.SplitN(req.Header.Get("authorization"), " ", 3)
    devEnv := os.Getenv("authorization")
    if len(sHeader) > 1 && devEnv == sHeader[1] {
        return nil
    }
    return fmt.Errorf("error")
}

Code review is an iterative process. If there are significant changes, open a new question with the revised code.

Leave a Reply

Your email address will not be published. Required fields are marked *