BugChomper utility - rewrite in Go

tools/bug_chomper/src/server/server.go

+// Copyright (c) 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+/*
+        Serves a webpage for easy management of Skia bugs.
+
+        WARNING: This server is NOT secure and should not be made publicly
+        accessible.
+*/
+
+package main
+
+import (
+        "crypto/rand"
+        "encoding/base64"
+        "encoding/json"
+        "errors"
+        "html/template"
+        "issue_tracker"
+        "log"
+        "net/http"
+        "net/url"
+        "path"
+        "path/filepath"
+        "strconv"
+        "strings"
+        "time"
+)
+
+const certFile = "certs/cert.pem"
+const keyFile = "certs/key.pem"
+const issueComment = "Edited by BugChomper"
+const oauthConfigFile = "oauth_client_secret.json"
+const defaultPort = 8000
+const maxSessionLen = 3600
+const priorityPrefix = "Priority-"
+const project = "skia"
+const scheme = "http"
+const sidBytes = 64
+const sidCookieName = "BugChomperSID"
+
+var contentTypesByExtension = map[string]string{

[jcgregorio, 2014/05/07 19:32:25]

Is there a reason you didn't use http://golang.org/pkg/mime/#TypeByExtension ?

[borenet, 2014/05/07 22:27:37]

Didn't know about it!

+        "css":  "text/css",
+        "js":   "application/javascript",
+        "ico":  "image/x-icon",
+        "htm":  "text/html",
+        "html": "text/html",
+}
+var staticPath, _ = filepath.Abs("static")
+var templatePath, _ = filepath.Abs("templates")
+var templates = template.Must(template.ParseFiles(
+        path.Join(templatePath, "bug_chomper.html"),
+        path.Join(templatePath, "submitted.html")))
+
+// SessionState contains data for a given session.
+type SessionState struct {
+        issueTracker   *issue_tracker.IssueTracker
+        origRequestURL string
+        sessionStart   time.Time
+}
+
+// sessionStates contains the state for all sessions. It's leaky because

[jcgregorio, 2014/05/07 19:32:25]

It also goes away when the server restarts. This will actually cause problems in
the long run because you only get a refresh token the first time you do the
OAuth dance, after that you only get an access token, which means everyone will
have to go through the OAuth dance every time they visit the site, but more
problematic, the site will only work for 1 hour, after which the access token
expires and because there's no refresh token your calls will either fail, or if
you detect the situation, they have to do the OAuth dance again.

[borenet, 2014/05/07 22:27:37]

So:
- (de)serialize session states on startup/shutdown
- Add offline access to OAuth2 config (done) OR delete all expired sessions
- Detect and delete long-expired sessions
?

[jcgregorio, 2014/05/08 15:11:56]

Again, the choices depend on if this is going to run publicly or just locally.
For a local run, just keep the info in memory, don't worry about saving it, and
use access_type online.

If this was going to be a public service another way to run would be to encrypt
and store the session state in a cookie, thus avoiding having any persistent
storage.

Actually encrypt and store the session state in a cookie might radically
simplify your code, since you now have session management in server.go and auth
stuff in issue_tracker.go, that change could consolidate it all into session
cookie handling. I've done this once before, I think I used
http://www.gorillatoolkit.org/pkg/securecookie to handle the details.


On 2014/05/07 22:27:37, borenet wrote:

+// nothing ever gets removed!
+var sessionStates = make(map[string]*SessionState)
+
+// makeSid returns a randomly-generated session identifier.
+func makeSid() string {
+        sid := make([]byte, sidBytes)
+        rand.Read(sid)
+        encodedSid := make([]byte, base64.StdEncoding.EncodedLen(len(sid)))
+        base64.StdEncoding.Encode(encodedSid, sid)
+        return string(encodedSid)
+}
+
+// getAbsoluteURL returns the absolute URL of the given Request.
+func getAbsoluteURL(req *http.Request) string {
+        return scheme + "://" + req.Host + req.URL.Path
+}
+
+// makeSession creates a new session for the Request.
+func makeSession(
+        resp http.ResponseWriter, req *http.Request) (*SessionState, error) {
+        sid := makeSid()
+        issueTracker, err := issue_tracker.MakeIssueTracker(oauthConfigFile)
+        if err != nil {
+                return nil, err
+        }
+        session := SessionState{
+                issueTracker:   issueTracker,
+                origRequestURL: getAbsoluteURL(req),
+                sessionStart:   time.Now(),
+        }
+        log.Println("Started session with SID: ", sid)
+        sessionStates[sid] = &session
+        cookie := makeCookieForSession(sid)
+        http.SetCookie(resp, cookie)
+        return &session, nil
+}
+
+// makeCookieForSession creates a Cookie containing the session ID for this
+// session.
+func makeCookieForSession(sid string) *http.Cookie {
+        expires := time.Now().Add(time.Duration(maxSessionLen * time.Second))
+        cookie := http.Cookie{
+                Name:       sidCookieName,
+                Value:      sid,
+                Path:       "",
+                Domain:     "",
+                Expires:    expires,
+                RawExpires: expires.String(),
+                MaxAge:     maxSessionLen,
+                Secure:     false,
+                HttpOnly:   true,
+                Raw:        "",
+                Unparsed:   nil,
+        }
+        return &cookie
+}
+
+// getSession retrieves the active SessionState or creates and returns a new
+// SessionState.
+func getSession(
+        resp http.ResponseWriter, req *http.Request) (*SessionState, error) {
+        cookie, err := req.Cookie(sidCookieName)
+        if err != nil {
+                // This is a new session. Create a SessionState.
+                return makeSession(resp, req)
+        }
+        sid := cookie.Value
+        session, ok := sessionStates[sid]
+        if !ok {
+                // Unknown session. Start a new one.
+                return makeSession(resp, req)
+        }
+        return session, nil
+}
+
+// makeBugChomperPage builds and serves the BugChomper page.
+func makeBugChomperPage(resp http.ResponseWriter, req *http.Request) error {
+        session, err := getSession(resp, req)
+        if err != nil {
+                return err
+        }
+        issueTracker := session.issueTracker
+        user := "borenet@google.com"
+        log.Println("Loading bugs for " + user)
+        bugList, err := issueTracker.GetBugs(project, user)
+        if err != nil {
+                return err
+        }
+        bugsById := make(map[string]*issue_tracker.Issue)
+        bugsByPriority := make(map[string][]*issue_tracker.Issue)
+        for _, bug := range bugList.Items {
+                bugsById[strconv.Itoa(bug.Id)] = bug
+                var bugPriority string
+                for _, label := range bug.Labels {
+                        if strings.HasPrefix(label, priorityPrefix) {
+                                bugPriority = label[len(priorityPrefix):]
+                        }
+                }
+                if _, ok := bugsByPriority[bugPriority]; !ok {
+                        bugsByPriority[bugPriority] = make(
+                                []*issue_tracker.Issue, 0)
+                }
+                bugsByPriority[bugPriority] = append(
+                        bugsByPriority[bugPriority], bug)
+        }
+        bugsJson, err := json.Marshal(bugsById)
+        if err != nil {
+                return err
+        }
+        pageData := struct {
+                Title          string
+                User           string
+                BugsJson       template.JS
+                BugsByPriority *map[string][]*issue_tracker.Issue
+                Priorities     []string
+                PriorityPrefix string
+        }{
+                Title:          "BugChomper",
+                User:           "borenet@google.com",
+                BugsJson:       template.JS(string(bugsJson)),
+                BugsByPriority: &bugsByPriority,
+                Priorities:     issue_tracker.BugPriorities,
+                PriorityPrefix: priorityPrefix,
+        }
+        err = templates.ExecuteTemplate(resp, "bug_chomper.html", pageData)
+        if err != nil {
+                return err
+        }
+        return nil
+}
+
+// authIfNeeded determines whether the current user is logged in. If not, it
+// redirects to a login page. If the user is just returning from the login,
+// it upgrades the single-use authorization code obtained from the login page
+// and redirects to the original request URL. Returns true if the user is
+// redirected and false otherwise.
+func authIfNeeded(resp http.ResponseWriter, req *http.Request) (bool, error) {
+        session, err := getSession(resp, req)
+        if err != nil {
+                return false, err
+        }
+        issueTracker := session.issueTracker
+        if !issueTracker.IsAuthenticated() {
+                params, err := url.ParseQuery(req.URL.RawQuery)
+                if err != nil {
+                        return false, err
+                }
+                if code, ok := params["code"]; ok {
+                        // We're returning from the login page.
+                        log.Println("Upgrading auth token:", code[0])
+                        err = issueTracker.UpgradeCode(code[0])

[jcgregorio, 2014/05/07 19:32:25]

This shouldn't be part of a regular page, i.e. the page that you redirect to for
the code exchange should be its own landing page that you bounce through on your
way back. See /oauth2callback on https://gist.github.com/border/3579615. There
are security issues with not using a separate page.

[borenet, 2014/05/07 22:27:37]

Done.

+                        if err != nil {
+                                return false, err
+                        }
+                        http.Redirect(resp, req, session.origRequestURL,
+                                http.StatusTemporaryRedirect)
+                        return true, nil
+                } else {
+                        loginURL := issueTracker.MakeAuthRequestURL(
+                                session.origRequestURL)
+                        log.Println("Redirecting for login:", loginURL)
+                        http.Redirect(resp, req, loginURL,
+                                http.StatusTemporaryRedirect)
+                        return true, nil
+                }
+        }
+        return false, nil
+}
+
+// submitData attempts to submit data from a POST request to the IssueTracker.
+func submitData(resp http.ResponseWriter, req *http.Request) error {
+        session, err := getSession(resp, req)
+        if err != nil {
+                return err
+        }
+        issueTracker := session.issueTracker
+        edits := req.FormValue("all_edits")
+        var editsMap map[string]*issue_tracker.Issue
+        err = json.Unmarshal([]byte(edits), &editsMap)
+        if err != nil {
+                return errors.New(
+                        "Could not parse edits from form response: " +
+                                err.Error())
+        }
+        pageData := struct {
+                Title    string
+                Message  string
+                BackLink string
+        }{}
+        if len(editsMap) == 0 {
+                pageData.Title = "No Changes Submitted"
+                pageData.Message = "You didn't change anything!"
+                pageData.BackLink = ""
+                err = templates.ExecuteTemplate(
+                        resp, "submitted.html", pageData)
+                if err != nil {
+                        return err
+                }
+                return nil
+        }
+        errorList := make([]error, 0)
+        for issueId, newIssue := range editsMap {
+                log.Println("Editing issue " + issueId)
+                err = issueTracker.SubmitIssueChanges(newIssue, issueComment)
+                if err != nil {
+                        errorList = append(errorList, err)
+                }
+        }
+        if len(errorList) > 0 {
+                errorStrings := ""
+                for _, err := range errorList {
+                        errorStrings += err.Error() + "\n"
+                }
+                return errors.New(
+                        "Not all changes could be submitted: \n" +
+                                errorStrings)
+        }
+        pageData.Title = "Submitted Changes"
+        pageData.Message = "Your changes were submitted to the issue tracker."
+        pageData.BackLink = ""
+        err = templates.ExecuteTemplate(resp, "submitted.html", pageData)
+        if err != nil {
+                return err
+        }
+        return nil
+}
+
+// handleBugChomper handles HTTP requests for the bug_chomper page.
+func handleBugChomper(resp http.ResponseWriter, req *http.Request) error {
+        redirected, err := authIfNeeded(resp, req)
+        if err != nil {
+                return err
+        }
+        if redirected {
+                return nil
+        }
+        switch req.Method {
+        case "GET":
+                err = makeBugChomperPage(resp, req)
+                if err != nil {
+                        return err
+                }
+        case "POST":
+                err = submitData(resp, req)
+                if err != nil {
+                        return err
+                }
+        }
+        return nil
+}
+
+// handle is the top-level handler function for all HTTP requests coming to
+// this server.
+func handle(resp http.ResponseWriter, req *http.Request) {

[jcgregorio, 2014/05/07 19:32:25]

Use w and r as the variable names here:

  func handle(w http.ResponseWriter, r *http.Request) {

Here and in all funcs that take http.ResponseWriter, *http.Request.

[borenet, 2014/05/07 22:27:37]

Done.  Is this just convention?  To me it seems easier to tell what "resp" and
"req" are than "w" and "r".

[jcgregorio, 2014/05/08 15:11:56]

Yes, it's a convention.

On 2014/05/07 22:27:37, borenet wrote:

+        log.Println("Fetching " + req.URL.Path)
+        if req.URL.Path == "/" || req.URL.Path == "/index.html" {
+                err := handleBugChomper(resp, req)
+                if err != nil {
+                        http.Error(resp, err.Error(),
+                                http.StatusInternalServerError)
+                        log.Println(err.Error())
+                }
+                return
+        }

[jcgregorio, 2014/05/07 19:32:25]

Just serve this off a subdir, i.e. do this in main():


   http.Handle("/res/", http.FileServer(http.Dir("./static")))

[borenet, 2014/05/07 22:27:37]

Done.

+        for ext := range contentTypesByExtension {
+                if strings.HasSuffix(req.URL.Path, ext) {
+                        http.ServeFile(
+                                resp, req, path.Join(staticPath, req.URL.Path))
+                        return
+                }
+        }
+        http.NotFound(resp, req)
+}
+
+// Run the BugChomper server.
+func main() {
+        http.HandleFunc("/", handle)
+        port := ":" + strconv.Itoa(defaultPort)
+        log.Println("Server is running at " + port)
+        var err error
+        if scheme == "https" {
+                err = http.ListenAndServeTLS(port, certFile, keyFile, nil)
+        } else {
+                err = http.ListenAndServe(port, nil)
+        }
+        if err != nil {
+                log.Println(err.Error())
+        }
+}