HiveBrain v1.2.0
Get Started
← Back to all entries
patterngoMinor

Slack App OAuth flow in golang

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
flowappoauthgolangslack

Problem

I'm working on a Slack app that uses Slash Commands. I've been looking for an excuse to learn Go, but the online tour was a little slow paced so I tried jumping in with this project.

I'm looking for advice on everything I can do to make this code as idiomatic as possible (that includes using libraries that might make some of what I'm doing simpler).

I'm also having some trouble figuring out how to split this into separate source files "the go way" since the project is going to get a bit larger than this.

There are 3 endpoints exposed:

"/": Returns "Hello World"

"/setColor": Called by Slack when a user uses the command

"/setColor", currently just returns "Hello World"

"/authenticateSlackTeam": Callback given to Slack to kickoff the OAuth flow, generates token and stores in database

package main

import (
"encoding/json"
"fmt"
"log"
http "net/http"
"os"
"time"

"github.com/dghubble/sling"
"github.com/jinzhu/gorm"
_ "github.com/jinzhu/gorm/dialects/postgres"
)

//Team exported for Gorm?
type team struct {
ID string
gorm:"primary_key"
Name string
Scope string
json:"scope"
Token string
}

type slackOauthRequestParams struct {
ClientID string
url:"client_id,omitempty"
ClientSecret string
url:"client_secret,omitempty"
Code string
url:"code,omitempty"
}
type slackOauthRequestResponse struct {
AccessToken string
json:"access_token"
Scope string
json:"scope"
TeamName string
json:"team_name"
TeamID string
json:"team_id"`
}

func sayHelloWorld(writer http.ResponseWriter, request *http.Request) {
log.Print("Request Received: ", request)
fmt.Fprintf(writer, "Hello World!")
}
func setColor(writer http.ResponseWriter, request *http.Request) {
log.Print("Set Color Request Received: ", request)
fmt.Fprintf(writer, "Hello World!")
}

func generateOAuthRequest(code string) (request *http.Request, err error) {
params := slackOauthR

Solution

Well, when you decide to something as a learning exercise it's normal to tackle just one problem at a time. In your program you have databases, object-relational mapping, JSON encoding, OAuth protocols, closures, REST, and more. It almost makes me a bit sad not to see slices, channels, and go-routines in there. What happened to the good-old FizzBuzz?

Your ambition here is admirable, and I understand the motivation to accomplish something meaningful (a cool slack-bot is awesome), but I worry that you'll maybe miss some details in your haste.
General

In general, your code looks like it has been through the wringer a few times, and it has wrinkles, and a few worn spots. While it looks like you are already in the habit of running your code through the go fmt routines, you should also add go vet and golint to your tools. You have a few issues I can see off-hand:

-
there's no need to rename the import net/http to http... that's what it's called already.

-
the comments on team are out of date. It's no longer exported (no longer capital-T Team.

-
you have mis-uses of log.Fatal(...). That call really is fatal, it calls os.Exit(1) when done. No code can run after the log.Fatal() so the following lines are redundant:

errorMessage := fmt.Sprintf("Failed to create OAuth Token request, parameters: %v", request)
    log.Fatal(errorMessage)
    http.Error(writer, errorMessage, 501)
    return


The httpError(...) is never sent.

There are some places where error checking is incomplete:

database, err := gorm.Open("postgres", os.Getenv("DATABASE_URL"))
defer database.Close()


The authorizeSlackTeam method's handler function captures the body of the request, but does not use it. If you are simply trying to force the close of the body at the function end (and not the beginning), you can replace:

body := request.Body
    defer body.Close()


with:

defer request.Body.Close()


In that same area, you have the function declaration:

func authorizeSlackTeam(db *gorm.DB) (handler func(writer http.ResponseWriter, request *http.Request)) {


but there's no need to name the writer and request. It can be just:

func authorizeSlackTeam(db *gorm.DB) (handler func(http.ResponseWriter,  *http.Request)) {


but even better, you can reuse the
http.HandlerFunc` type to simplify it to:

func authorizeSlackTeam(db *gorm.DB) (handler http.HandlerFunc) {


Despite the above issues, in general, the code is good. The variable names are descriptive, the function names too. The code does flow quite well, and I don't feel like the functions are doing too much.
http errors

It's common in http-handling code to have a helper function to allow child methods to return an error, and then you catch that error and return a consistent, reused, error message system.

Consider a function like:

type UncheckedHandler func(http.ResponseWriter, *http.Request) error

func checkedHandler(handler UncheckedHandler) http.HandlerFunc {

    return func(writer http.ResponseWriter, request *http.Request) {
        if err := handler(writer, request) err != nil {
            log.Printf(err)
            http.Error(writer, err.Error(), 501)
        }
    }

}


Now you can have your own set of handlers that have an error return value, and you can simplify the error-handling code that you have scattered over the place. The setup routine changes from:

http.HandleFunc("/", sayHelloWorld)
http.HandleFunc("/setColor", setColor)
http.HandleFunc("/authorizeSlackTeam", authorizeSlackTeam(database))


to

http.HandleFunc("/", checkedHandler(sayHelloWorld))
http.HandleFunc("/setColor", checkedHandler(setColor))
http.HandleFunc("/authorizeSlackTeam", checkedHandler(authorizeSlackTeam(database)))

Code Snippets

errorMessage := fmt.Sprintf("Failed to create OAuth Token request, parameters: %v", request)
    log.Fatal(errorMessage)
    http.Error(writer, errorMessage, 501)
    return
database, err := gorm.Open("postgres", os.Getenv("DATABASE_URL"))
defer database.Close()
body := request.Body
    defer body.Close()
defer request.Body.Close()
func authorizeSlackTeam(db *gorm.DB) (handler func(writer http.ResponseWriter, request *http.Request)) {

Context

StackExchange Code Review Q#154759, answer score: 2

Revisions (0)

No revisions yet.