From 4d63e9cd68f3426e6ac670a061ea9ecca332118d Mon Sep 17 00:00:00 2001 From: "m.nabokikh" Date: Sun, 18 Oct 2020 01:02:29 +0400 Subject: [PATCH 1/2] fix: Bump golangci-lint version and fix some linter's problems Signed-off-by: m.nabokikh --- .golangci.yml | 67 +++++++++++++--------- Makefile | 2 +- connector/atlassiancrowd/atlassiancrowd.go | 2 +- connector/google/google.go | 3 +- connector/ldap/ldap.go | 5 +- connector/saml/saml.go | 33 ++++++----- server/deviceflowhandlers_test.go | 4 +- server/handlers.go | 10 ++-- server/templates.go | 25 ++++---- storage/kubernetes/types.go | 2 +- storage/sql/crud.go | 6 +- storage/storage.go | 17 +++--- 12 files changed, 97 insertions(+), 79 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index ed271f01..64357049 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -1,34 +1,45 @@ run: - timeout: 2m + timeout: 2m linters-settings: - golint: - min-confidence: 0.1 - goimports: - local-prefixes: github.com/dexidp/dex + golint: + min-confidence: 0.1 + goimports: + local-prefixes: github.com/dexidp/dex linters: - enable-all: true - disable: - - funlen - - maligned - - wsl + disable-all: true + enable: + - bodyclose + - deadcode + - depguard + - dogsled + - gochecknoinits + - gofmt + - goimports + - golint + - gosimple + - govet + - ineffassign + - interfacer + - misspell + - nakedret + - staticcheck + - structcheck + - stylecheck + - typecheck + - unconvert + - unused + - varcheck + - whitespace - # TODO: fix me - - unparam - - golint - - goconst - - staticcheck - - nakedret - - errcheck - - gosec - - gochecknoinits - - gochecknoglobals - - prealloc - - scopelint - - lll - - dupl - - gocritic - - gocyclo - - gocognit - - godox + # TODO: fix linter errors before enabling + # - unparam + # - scopelint + # - gosec + # - gocyclo + # - lll + # - goconst + # - gocritic + # - errcheck + # - dupl diff --git a/Makefile b/Makefile index abf8655d..b665bcfc 100644 --- a/Makefile +++ b/Makefile @@ -18,7 +18,7 @@ export GOBIN=$(PWD)/bin LD_FLAGS="-w -X $(REPO_PATH)/version.Version=$(VERSION)" # Dependency versions -GOLANGCI_VERSION = 1.21.0 +GOLANGCI_VERSION = 1.31.0 build: bin/dex diff --git a/connector/atlassiancrowd/atlassiancrowd.go b/connector/atlassiancrowd/atlassiancrowd.go index 928e69a6..dbc60a74 100644 --- a/connector/atlassiancrowd/atlassiancrowd.go +++ b/connector/atlassiancrowd/atlassiancrowd.go @@ -111,7 +111,7 @@ func (c *crowdConnector) Login(ctx context.Context, s connector.Scopes, username // We want to return a different error if the user's password is incorrect vs // if there was an error. - incorrectPass := false + var incorrectPass bool var user crowdUser client := c.crowdAPIClient() diff --git a/connector/google/google.go b/connector/google/google.go index 37b89edd..d4c4e18a 100644 --- a/connector/google/google.go +++ b/connector/google/google.go @@ -13,6 +13,7 @@ import ( "golang.org/x/oauth2" "golang.org/x/oauth2/google" admin "google.golang.org/api/admin/directory/v1" + "google.golang.org/api/option" "github.com/dexidp/dex/connector" pkg_groups "github.com/dexidp/dex/pkg/groups" @@ -289,7 +290,7 @@ func createDirectoryService(serviceAccountFilePath string, email string) (*admin ctx := context.Background() client := config.Client(ctx) - srv, err := admin.New(client) + srv, err := admin.NewService(ctx, option.WithHTTPClient(client)) if err != nil { return nil, fmt.Errorf("unable to create directory service %v", err) } diff --git a/connector/ldap/ldap.go b/connector/ldap/ldap.go index 2e12a4c9..99745b58 100644 --- a/connector/ldap/ldap.go +++ b/connector/ldap/ldap.go @@ -56,6 +56,7 @@ import ( // nameAttr: name // +// UserMatcher holds information about user and group matching. type UserMatcher struct { UserAttr string `json:"userAttr"` GroupAttr string `json:"groupAttr"` @@ -189,7 +190,7 @@ func (c *ldapConnector) userMatchers() []UserMatcher { if len(c.GroupSearch.UserMatchers) > 0 && c.GroupSearch.UserMatchers[0].UserAttr != "" { return c.GroupSearch.UserMatchers[:] } - + return []UserMatcher{ { UserAttr: c.GroupSearch.UserAttr, @@ -303,7 +304,7 @@ var ( // do initializes a connection to the LDAP directory and passes it to the // provided function. It then performs appropriate teardown or reuse before // returning. -func (c *ldapConnector) do(ctx context.Context, f func(c *ldap.Conn) error) error { +func (c *ldapConnector) do(_ context.Context, f func(c *ldap.Conn) error) error { // TODO(ericchiang): support context here var ( conn *ldap.Conn diff --git a/connector/saml/saml.go b/connector/saml/saml.go index e0f3f933..29f5dd98 100644 --- a/connector/saml/saml.go +++ b/connector/saml/saml.go @@ -11,6 +11,7 @@ import ( "fmt" "io/ioutil" "strings" + "sync" "time" "github.com/beevik/etree" @@ -60,20 +61,9 @@ var ( nameIDformatTransient, } nameIDFormatLookup = make(map[string]string) -) -func init() { - suffix := func(s, sep string) string { - if i := strings.LastIndex(s, sep); i > 0 { - return s[i+1:] - } - return s - } - for _, format := range nameIDFormats { - nameIDFormatLookup[suffix(format, ":")] = format - nameIDFormatLookup[format] = format - } -} + lookupOnce sync.Once +) // Config represents configuration options for the SAML provider. type Config struct { @@ -176,6 +166,19 @@ func (c *Config) openConnector(logger log.Logger) (*provider, error) { if p.nameIDPolicyFormat == "" { p.nameIDPolicyFormat = nameIDFormatPersistent } else { + lookupOnce.Do(func() { + suffix := func(s, sep string) string { + if i := strings.LastIndex(s, sep); i > 0 { + return s[i+1:] + } + return s + } + for _, format := range nameIDFormats { + nameIDFormatLookup[suffix(format, ":")] = format + nameIDFormatLookup[format] = format + } + }) + if format, ok := nameIDFormatLookup[p.nameIDPolicyFormat]; ok { p.nameIDPolicyFormat = format } else { @@ -364,7 +367,7 @@ func (p *provider) HandlePOST(s connector.Scopes, samlResponse, inResponseTo str switch { case subject.NameID != nil: if ident.UserID = subject.NameID.Value; ident.UserID == "" { - return ident, fmt.Errorf("NameID element does not contain a value") + return ident, fmt.Errorf("element NameID does not contain a value") } default: return ident, fmt.Errorf("subject does not contain an NameID element") @@ -488,7 +491,7 @@ func (p *provider) validateSubject(subject *subject, inResponseTo string) error data := c.SubjectConfirmationData if data == nil { - return fmt.Errorf("SubjectConfirmation contained no SubjectConfirmationData") + return fmt.Errorf("no SubjectConfirmationData field found in SubjectConfirmation") } if data.InResponseTo != inResponseTo { return fmt.Errorf("expected SubjectConfirmationData InResponseTo value %q, got %q", inResponseTo, data.InResponseTo) diff --git a/server/deviceflowhandlers_test.go b/server/deviceflowhandlers_test.go index 5ab3ddb6..196fc54c 100644 --- a/server/deviceflowhandlers_test.go +++ b/server/deviceflowhandlers_test.go @@ -546,7 +546,7 @@ func TestDeviceTokenResponse(t *testing.T) { t.Errorf("Could read token response %v", err) } if tc.expectedResponseCode == http.StatusBadRequest || tc.expectedResponseCode == http.StatusUnauthorized { - expectJsonErrorResponse(tc.testName, body, tc.expectedServerResponse, t) + expectJSONErrorResponse(tc.testName, body, tc.expectedServerResponse, t) } else if string(body) != tc.expectedServerResponse { t.Errorf("Unexpected Server Response. Expected %v got %v", tc.expectedServerResponse, string(body)) } @@ -554,7 +554,7 @@ func TestDeviceTokenResponse(t *testing.T) { } } -func expectJsonErrorResponse(testCase string, body []byte, expectedError string, t *testing.T) { +func expectJSONErrorResponse(testCase string, body []byte, expectedError string, t *testing.T) { jsonMap := make(map[string]interface{}) err := json.Unmarshal(body, &jsonMap) if err != nil { diff --git a/server/handlers.go b/server/handlers.go index 9c02524f..0bc6dcb4 100644 --- a/server/handlers.go +++ b/server/handlers.go @@ -274,7 +274,7 @@ func (s *Server) handleAuthorization(w http.ResponseWriter, r *http.Request) { } } - if err := s.templates.login(r, w, connectorInfos, r.URL.Path); err != nil { + if err := s.templates.login(r, w, connectorInfos); err != nil { s.logger.Errorf("Server template error: %v", err) } } @@ -335,7 +335,7 @@ func (s *Server) handleConnectorLogin(w http.ResponseWriter, r *http.Request) { } http.Redirect(w, r, callbackURL, http.StatusFound) case connector.PasswordConnector: - if err := s.templates.password(r, w, r.URL.String(), "", usernamePrompt(conn), false, showBacklink, r.URL.Path); err != nil { + if err := s.templates.password(r, w, r.URL.String(), "", usernamePrompt(conn), false, showBacklink); err != nil { s.logger.Errorf("Server template error: %v", err) } case connector.SAMLConnector: @@ -383,7 +383,7 @@ func (s *Server) handleConnectorLogin(w http.ResponseWriter, r *http.Request) { return } if !ok { - if err := s.templates.password(r, w, r.URL.String(), username, usernamePrompt(passwordConnector), true, showBacklink, r.URL.Path); err != nil { + if err := s.templates.password(r, w, r.URL.String(), username, usernamePrompt(passwordConnector), true, showBacklink); err != nil { s.logger.Errorf("Server template error: %v", err) } return @@ -577,7 +577,7 @@ func (s *Server) handleApproval(w http.ResponseWriter, r *http.Request) { s.renderError(r, w, http.StatusInternalServerError, "Failed to retrieve client.") return } - if err := s.templates.approval(r, w, authReq.ID, authReq.Claims.Username, client.Name, authReq.Scopes, r.URL.Path); err != nil { + if err := s.templates.approval(r, w, authReq.ID, authReq.Claims.Username, client.Name, authReq.Scopes); err != nil { s.logger.Errorf("Server template error: %v", err) } case http.MethodPost: @@ -650,7 +650,7 @@ func (s *Server) sendCodeResponse(w http.ResponseWriter, r *http.Request, authRe // Implicit and hybrid flows that try to use the OOB redirect URI are // rejected earlier. If we got here we're using the code flow. if authReq.RedirectURI == redirectURIOOB { - if err := s.templates.oob(r, w, code.ID, r.URL.Path); err != nil { + if err := s.templates.oob(r, w, code.ID); err != nil { s.logger.Errorf("Server template error: %v", err) } return diff --git a/server/templates.go b/server/templates.go index c5b62532..601aaa08 100644 --- a/server/templates.go +++ b/server/templates.go @@ -78,7 +78,7 @@ func dirExists(dir string) error { // | |- (theme name) // |- templates // -func loadWebConfig(c webConfig) (static, theme http.Handler, templates *templates, err error) { +func loadWebConfig(c webConfig) (http.Handler, http.Handler, *templates, error) { if c.theme == "" { c.theme = "coreos" } @@ -106,11 +106,11 @@ func loadWebConfig(c webConfig) (static, theme http.Handler, templates *template } } - static = http.FileServer(http.Dir(staticDir)) - theme = http.FileServer(http.Dir(themeDir)) + static := http.FileServer(http.Dir(staticDir)) + theme := http.FileServer(http.Dir(themeDir)) - templates, err = loadTemplates(c, templatesDir) - return + templates, err := loadTemplates(c, templatesDir) + return static, theme, templates, err } // loadTemplates parses the expected templates from the provided directory. @@ -219,8 +219,7 @@ func relativeURL(serverPath, reqPath, assetPath string) string { server, req, asset := splitPath(serverPath), splitPath(reqPath), splitPath(assetPath) // Remove common prefix of request path with server path - // nolint: ineffassign - server, req = stripCommonParts(server, req) + _, req = stripCommonParts(server, req) // Remove common prefix of request path with asset path asset, req = stripCommonParts(asset, req) @@ -276,7 +275,7 @@ func (t *templates) deviceSuccess(r *http.Request, w http.ResponseWriter, client return renderTemplate(w, t.deviceSuccessTmpl, data) } -func (t *templates) login(r *http.Request, w http.ResponseWriter, connectors []connectorInfo, reqPath string) error { +func (t *templates) login(r *http.Request, w http.ResponseWriter, connectors []connectorInfo) error { sort.Sort(byName(connectors)) data := struct { Connectors []connectorInfo @@ -285,7 +284,7 @@ func (t *templates) login(r *http.Request, w http.ResponseWriter, connectors []c return renderTemplate(w, t.loginTmpl, data) } -func (t *templates) password(r *http.Request, w http.ResponseWriter, postURL, lastUsername, usernamePrompt string, lastWasInvalid, showBacklink bool, reqPath string) error { +func (t *templates) password(r *http.Request, w http.ResponseWriter, postURL, lastUsername, usernamePrompt string, lastWasInvalid, showBacklink bool) error { data := struct { PostURL string BackLink bool @@ -297,7 +296,7 @@ func (t *templates) password(r *http.Request, w http.ResponseWriter, postURL, la return renderTemplate(w, t.passwordTmpl, data) } -func (t *templates) approval(r *http.Request, w http.ResponseWriter, authReqID, username, clientName string, scopes []string, reqPath string) error { +func (t *templates) approval(r *http.Request, w http.ResponseWriter, authReqID, username, clientName string, scopes []string) error { accesses := []string{} for _, scope := range scopes { access, ok := scopeDescriptions[scope] @@ -316,7 +315,7 @@ func (t *templates) approval(r *http.Request, w http.ResponseWriter, authReqID, return renderTemplate(w, t.approvalTmpl, data) } -func (t *templates) oob(r *http.Request, w http.ResponseWriter, code string, reqPath string) error { +func (t *templates) oob(r *http.Request, w http.ResponseWriter, code string) error { data := struct { Code string ReqPath string @@ -332,7 +331,7 @@ func (t *templates) err(r *http.Request, w http.ResponseWriter, errCode int, err ReqPath string }{http.StatusText(errCode), errMsg, r.URL.Path} if err := t.errorTmpl.Execute(w, data); err != nil { - return fmt.Errorf("Error rendering template %s: %s", t.errorTmpl.Name(), err) + return fmt.Errorf("rendering template %s failed: %s", t.errorTmpl.Name(), err) } return nil } @@ -355,7 +354,7 @@ func renderTemplate(w http.ResponseWriter, tmpl *template.Template, data interfa // TODO(ericchiang): replace with better internal server error. http.Error(w, "Internal server error", http.StatusInternalServerError) } - return fmt.Errorf("Error rendering template %s: %s", tmpl.Name(), err) + return fmt.Errorf("rendering template %s failed: %s", tmpl.Name(), err) } return nil } diff --git a/storage/kubernetes/types.go b/storage/kubernetes/types.go index f856a731..c3eb4172 100644 --- a/storage/kubernetes/types.go +++ b/storage/kubernetes/types.go @@ -679,7 +679,7 @@ type DeviceRequest struct { Expiry time.Time `json:"expiry"` } -// AuthRequestList is a list of AuthRequests. +// DeviceRequestList is a list of DeviceRequests. type DeviceRequestList struct { k8sapi.TypeMeta `json:",inline"` k8sapi.ListMeta `json:"metadata,omitempty"` diff --git a/storage/sql/crud.go b/storage/sql/crud.go index b74b76e1..325756f6 100644 --- a/storage/sql/crud.go +++ b/storage/sql/crud.go @@ -84,7 +84,9 @@ type scanner interface { Scan(dest ...interface{}) error } -func (c *conn) GarbageCollect(now time.Time) (result storage.GCResult, err error) { +func (c *conn) GarbageCollect(now time.Time) (storage.GCResult, error) { + result := storage.GCResult{} + r, err := c.Exec(`delete from auth_request where expiry < $1`, now) if err != nil { return result, fmt.Errorf("gc auth_request: %v", err) @@ -117,7 +119,7 @@ func (c *conn) GarbageCollect(now time.Time) (result storage.GCResult, err error result.DeviceTokens = n } - return + return result, err } func (c *conn) CreateAuthRequest(a storage.AuthRequest) error { diff --git a/storage/storage.go b/storage/storage.go index d11305e2..82ce2cb1 100644 --- a/storage/storage.go +++ b/storage/storage.go @@ -384,23 +384,24 @@ func randomString(n int) (string, error) { return string(bytes), nil } -//DeviceRequest represents an OIDC device authorization request. It holds the state of a device request until the user -//authenticates using their user code or the expiry time passes. +// DeviceRequest represents an OIDC device authorization request. It holds the state of a device request until the user +// authenticates using their user code or the expiry time passes. type DeviceRequest struct { - //The code the user will enter in a browser + // The code the user will enter in a browser UserCode string - //The unique device code for device authentication + // The unique device code for device authentication DeviceCode string - //The client ID the code is for + // The client ID the code is for ClientID string - //The Client Secret + // The Client Secret ClientSecret string - //The scopes the device requests + // The scopes the device requests Scopes []string - //The expire time + // The expire time Expiry time.Time } +// DeviceToken is a structure which represents the actual token of an authorized device and its rotation parameters type DeviceToken struct { DeviceCode string Status string From 1d83e4749d7e65532c5d903f1370d277132c88c8 Mon Sep 17 00:00:00 2001 From: "m.nabokikh" Date: Sun, 18 Oct 2020 01:54:27 +0400 Subject: [PATCH 2/2] Add gocritic Signed-off-by: m.nabokikh --- .golangci.yml | 2 +- cmd/dex/config.go | 2 +- connector/authproxy/authproxy.go | 2 +- connector/github/github.go | 7 +++--- connector/ldap/ldap.go | 6 ++--- server/api_test.go | 2 +- server/deviceflowhandlers.go | 38 ++++++++++++++--------------- server/deviceflowhandlers_test.go | 10 ++++---- server/handlers.go | 36 +++++++++++++++------------ server/server.go | 2 +- server/server_test.go | 28 ++++++++++----------- server/templates.go | 10 ++++---- storage/conformance/conformance.go | 36 +++++++++++---------------- storage/kubernetes/k8sapi/client.go | 2 +- storage/sql/config.go | 9 ++++--- storage/static.go | 2 +- storage/storage.go | 2 +- 17 files changed, 99 insertions(+), 97 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 64357049..ac9134bf 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -19,6 +19,7 @@ linters: - goimports - golint - gosimple + - gocritic - govet - ineffassign - interfacer @@ -40,6 +41,5 @@ linters: # - gocyclo # - lll # - goconst - # - gocritic # - errcheck # - dupl diff --git a/cmd/dex/config.go b/cmd/dex/config.go index 255e1d95..eba919fa 100644 --- a/cmd/dex/config.go +++ b/cmd/dex/config.go @@ -49,7 +49,7 @@ type Config struct { StaticPasswords []password `json:"staticPasswords"` } -//Validate the configuration +// Validate the configuration func (c Config) Validate() error { // Fast checks. Perform these first for a more responsive CLI. checks := []struct { diff --git a/connector/authproxy/authproxy.go b/connector/authproxy/authproxy.go index f58c4b68..c405c69c 100644 --- a/connector/authproxy/authproxy.go +++ b/connector/authproxy/authproxy.go @@ -34,7 +34,7 @@ func (m *callback) LoginURL(s connector.Scopes, callbackURL, state string) (stri if err != nil { return "", fmt.Errorf("failed to parse callbackURL %q: %v", callbackURL, err) } - u.Path = u.Path + m.pathSuffix + u.Path += m.pathSuffix v := u.Query() v.Set("state", state) u.RawQuery = v.Encode() diff --git a/connector/github/github.go b/connector/github/github.go index e38eb26a..6b95ea8f 100644 --- a/connector/github/github.go +++ b/connector/github/github.go @@ -334,11 +334,12 @@ func (c *githubConnector) Refresh(ctx context.Context, s connector.Scopes, ident // getGroups retrieves GitHub orgs and teams a user is in, if any. func (c *githubConnector) getGroups(ctx context.Context, client *http.Client, groupScope bool, userLogin string) ([]string, error) { - if len(c.orgs) > 0 { + switch { + case len(c.orgs) > 0: return c.groupsForOrgs(ctx, client, userLogin) - } else if c.org != "" { + case c.org != "": return c.teamsForOrg(ctx, client, c.org) - } else if groupScope && c.loadAllGroups { + case groupScope && c.loadAllGroups: return c.userGroups(ctx, client) } return nil, nil diff --git a/connector/ldap/ldap.go b/connector/ldap/ldap.go index 99745b58..7d6ae8dd 100644 --- a/connector/ldap/ldap.go +++ b/connector/ldap/ldap.go @@ -188,7 +188,7 @@ func parseScope(s string) (int, bool) { // See "Config.GroupSearch.UserMatchers" comments for the details func (c *ldapConnector) userMatchers() []UserMatcher { if len(c.GroupSearch.UserMatchers) > 0 && c.GroupSearch.UserMatchers[0].UserAttr != "" { - return c.GroupSearch.UserMatchers[:] + return c.GroupSearch.UserMatchers } return []UserMatcher{ @@ -245,9 +245,9 @@ func (c *Config) openConnector(logger log.Logger) (*ldapConnector, error) { if host, _, err = net.SplitHostPort(c.Host); err != nil { host = c.Host if c.InsecureNoSSL { - c.Host = c.Host + ":389" + c.Host += ":389" } else { - c.Host = c.Host + ":636" + c.Host += ":636" } } diff --git a/server/api_test.go b/server/api_test.go index 61ec9942..573e52b3 100644 --- a/server/api_test.go +++ b/server/api_test.go @@ -291,7 +291,7 @@ func TestRefreshToken(t *testing.T) { t.Errorf("failed to marshal offline session ID: %v", err) } - //Testing the api. + // Testing the api. listReq := api.ListRefreshReq{ UserId: subjectString, } diff --git a/server/deviceflowhandlers.go b/server/deviceflowhandlers.go index 4a8b382d..e0932c54 100644 --- a/server/deviceflowhandlers.go +++ b/server/deviceflowhandlers.go @@ -15,17 +15,17 @@ import ( ) type deviceCodeResponse struct { - //The unique device code for device authentication + // The unique device code for device authentication DeviceCode string `json:"device_code"` - //The code the user will exchange via a browser and log in + // The code the user will exchange via a browser and log in UserCode string `json:"user_code"` - //The url to verify the user code. + // The url to verify the user code. VerificationURI string `json:"verification_uri"` - //The verification uri with the user code appended for pre-filling form + // The verification uri with the user code appended for pre-filling form VerificationURIComplete string `json:"verification_uri_complete"` - //The lifetime of the device code + // The lifetime of the device code ExpireTime int `json:"expires_in"` - //How often the device is allowed to poll to verify that the user login occurred + // How often the device is allowed to poll to verify that the user login occurred PollInterval int `json:"interval"` } @@ -66,27 +66,27 @@ func (s *Server) handleDeviceCode(w http.ResponseWriter, r *http.Request) { return } - //Get the client id and scopes from the post + // Get the client id and scopes from the post clientID := r.Form.Get("client_id") clientSecret := r.Form.Get("client_secret") scopes := strings.Fields(r.Form.Get("scope")) s.logger.Infof("Received device request for client %v with scopes %v", clientID, scopes) - //Make device code + // Make device code deviceCode := storage.NewDeviceCode() - //make user code + // make user code userCode, err := storage.NewUserCode() if err != nil { s.logger.Errorf("Error generating user code: %v", err) s.tokenErrHelper(w, errInvalidRequest, "", http.StatusInternalServerError) } - //Generate the expire time + // Generate the expire time expireTime := time.Now().Add(s.deviceRequestsValidFor) - //Store the Device Request + // Store the Device Request deviceReq := storage.DeviceRequest{ UserCode: userCode, DeviceCode: deviceCode, @@ -102,7 +102,7 @@ func (s *Server) handleDeviceCode(w http.ResponseWriter, r *http.Request) { return } - //Store the device token + // Store the device token deviceToken := storage.DeviceToken{ DeviceCode: deviceCode, Status: deviceTokenPending, @@ -176,7 +176,7 @@ func (s *Server) handleDeviceToken(w http.ResponseWriter, r *http.Request) { now := s.now() - //Grab the device token, check validity + // Grab the device token, check validity deviceToken, err := s.storage.GetDeviceToken(deviceCode) if err != nil { if err != storage.ErrNotFound { @@ -189,13 +189,13 @@ func (s *Server) handleDeviceToken(w http.ResponseWriter, r *http.Request) { return } - //Rate Limiting check + // Rate Limiting check slowDown := false pollInterval := deviceToken.PollIntervalSeconds minRequestTime := deviceToken.LastRequestTime.Add(time.Second * time.Duration(pollInterval)) if now.Before(minRequestTime) { slowDown = true - //Continually increase the poll interval until the user waits the proper time + // Continually increase the poll interval until the user waits the proper time pollInterval += 5 } else { pollInterval = 5 @@ -255,7 +255,7 @@ func (s *Server) handleDeviceCallback(w http.ResponseWriter, r *http.Request) { return } - //Grab the device request from storage + // Grab the device request from storage deviceReq, err := s.storage.GetDeviceRequest(userCode) if err != nil || s.now().After(deviceReq.Expiry) { errCode := http.StatusBadRequest @@ -289,7 +289,7 @@ func (s *Server) handleDeviceCallback(w http.ResponseWriter, r *http.Request) { return } - //Grab the device token from storage + // Grab the device token from storage old, err := s.storage.GetDeviceToken(deviceReq.DeviceCode) if err != nil || s.now().After(old.Expiry) { errCode := http.StatusBadRequest @@ -353,7 +353,7 @@ func (s *Server) verifyUserCode(w http.ResponseWriter, r *http.Request) { userCode = strings.ToUpper(userCode) - //Find the user code in the available requests + // Find the user code in the available requests deviceRequest, err := s.storage.GetDeviceRequest(userCode) if err != nil || s.now().After(deviceRequest.Expiry) { if err != nil && err != storage.ErrNotFound { @@ -366,7 +366,7 @@ func (s *Server) verifyUserCode(w http.ResponseWriter, r *http.Request) { return } - //Redirect to Dex Auth Endpoint + // Redirect to Dex Auth Endpoint authURL := path.Join(s.issuerURL.Path, "/auth") u, err := url.Parse(authURL) if err != nil { diff --git a/server/deviceflowhandlers_test.go b/server/deviceflowhandlers_test.go index 196fc54c..3898d4fc 100644 --- a/server/deviceflowhandlers_test.go +++ b/server/deviceflowhandlers_test.go @@ -24,7 +24,7 @@ func TestDeviceVerificationURI(t *testing.T) { defer cancel() // Setup a dex server. httpServer, s := newTestServer(ctx, t, func(c *Config) { - c.Issuer = c.Issuer + "/non-root-path" + c.Issuer += "/non-root-path" c.Now = now }) defer httpServer.Close() @@ -76,7 +76,7 @@ func TestHandleDeviceCode(t *testing.T) { // Setup a dex server. httpServer, s := newTestServer(ctx, t, func(c *Config) { - c.Issuer = c.Issuer + "/non-root-path" + c.Issuer += "/non-root-path" c.Now = now }) defer httpServer.Close() @@ -322,7 +322,7 @@ func TestDeviceCallback(t *testing.T) { // Setup a dex server. httpServer, s := newTestServer(ctx, t, func(c *Config) { - //c.Issuer = c.Issuer + "/non-root-path" + // c.Issuer = c.Issuer + "/non-root-path" c.Now = now }) defer httpServer.Close() @@ -506,7 +506,7 @@ func TestDeviceTokenResponse(t *testing.T) { // Setup a dex server. httpServer, s := newTestServer(ctx, t, func(c *Config) { - c.Issuer = c.Issuer + "/non-root-path" + c.Issuer += "/non-root-path" c.Now = now }) defer httpServer.Close() @@ -637,7 +637,7 @@ func TestVerifyCodeResponse(t *testing.T) { // Setup a dex server. httpServer, s := newTestServer(ctx, t, func(c *Config) { - c.Issuer = c.Issuer + "/non-root-path" + c.Issuer += "/non-root-path" c.Now = now }) defer httpServer.Close() diff --git a/server/handlers.go b/server/handlers.go index 0bc6dcb4..bd134813 100644 --- a/server/handlers.go +++ b/server/handlers.go @@ -505,7 +505,7 @@ func (s *Server) finalizeLogin(identity connector.Identity, authReq storage.Auth email := claims.Email if !claims.EmailVerified { - email = email + " (unverified)" + email += " (unverified)" } s.logger.Infof("login successful: connector %q, username=%q, preferred_username=%q, email=%q, groups=%q", @@ -518,7 +518,8 @@ func (s *Server) finalizeLogin(identity connector.Identity, authReq storage.Auth } // Try to retrieve an existing OfflineSession object for the corresponding user. - if session, err := s.storage.GetOfflineSessions(identity.UserID, authReq.ConnectorID); err != nil { + session, err := s.storage.GetOfflineSessions(identity.UserID, authReq.ConnectorID) + if err != nil { if err != storage.ErrNotFound { s.logger.Errorf("failed to get offline session: %v", err) return "", err @@ -536,17 +537,19 @@ func (s *Server) finalizeLogin(identity connector.Identity, authReq storage.Auth s.logger.Errorf("failed to create offline session: %v", err) return "", err } - } else { - // Update existing OfflineSession obj with new RefreshTokenRef. - if err := s.storage.UpdateOfflineSessions(session.UserID, session.ConnID, func(old storage.OfflineSessions) (storage.OfflineSessions, error) { - if len(identity.ConnectorData) > 0 { - old.ConnectorData = identity.ConnectorData - } - return old, nil - }); err != nil { - s.logger.Errorf("failed to update offline session: %v", err) - return "", err + + return returnURL, nil + } + + // Update existing OfflineSession obj with new RefreshTokenRef. + if err := s.storage.UpdateOfflineSessions(session.UserID, session.ConnID, func(old storage.OfflineSessions) (storage.OfflineSessions, error) { + if len(identity.ConnectorData) > 0 { + old.ConnectorData = identity.ConnectorData } + return old, nil + }); err != nil { + s.logger.Errorf("failed to update offline session: %v", err) + return "", err } return returnURL, nil @@ -1017,15 +1020,18 @@ func (s *Server) handleRefreshToken(w http.ResponseWriter, r *http.Request, clie } var connectorData []byte - if session, err := s.storage.GetOfflineSessions(refresh.Claims.UserID, refresh.ConnectorID); err != nil { + + session, err := s.storage.GetOfflineSessions(refresh.Claims.UserID, refresh.ConnectorID) + switch { + case err != nil: if err != storage.ErrNotFound { s.logger.Errorf("failed to get offline session: %v", err) return } - } else if len(refresh.ConnectorData) > 0 { + case len(refresh.ConnectorData) > 0: // Use the old connector data if it exists, should be deleted once used connectorData = refresh.ConnectorData - } else { + default: connectorData = session.ConnectorData } diff --git a/server/server.go b/server/server.go index c7d416fa..c37b4fdc 100644 --- a/server/server.go +++ b/server/server.go @@ -305,7 +305,7 @@ func newServer(ctx context.Context, c Config, rotationStrategy rotationStrategy) } r.Handle(path.Join(issuerURL.Path, p), instrumentHandlerCounter(p, handler)) } - r.NotFoundHandler = http.HandlerFunc(http.NotFound) + r.NotFoundHandler = http.NotFoundHandler() discoveryHandler, err := s.discoveryHandler() if err != nil { diff --git a/server/server_test.go b/server/server_test.go index 114d1fc8..c36f2e85 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -177,7 +177,7 @@ func TestDiscovery(t *testing.T) { defer cancel() httpServer, _ := newTestServer(ctx, t, func(c *Config) { - c.Issuer = c.Issuer + "/non-root-path" + c.Issuer += "/non-root-path" }) defer httpServer.Close() @@ -504,7 +504,7 @@ func TestOAuth2CodeFlow(t *testing.T) { // Setup a dex server. httpServer, s := newTestServer(ctx, t, func(c *Config) { - c.Issuer = c.Issuer + "/non-root-path" + c.Issuer += "/non-root-path" c.Now = now c.IDTokensValidFor = idTokensValidFor }) @@ -766,7 +766,7 @@ func TestCrossClientScopes(t *testing.T) { defer cancel() httpServer, s := newTestServer(ctx, t, func(c *Config) { - c.Issuer = c.Issuer + "/non-root-path" + c.Issuer += "/non-root-path" }) defer httpServer.Close() @@ -889,7 +889,7 @@ func TestCrossClientScopesWithAzpInAudienceByDefault(t *testing.T) { defer cancel() httpServer, s := newTestServer(ctx, t, func(c *Config) { - c.Issuer = c.Issuer + "/non-root-path" + c.Issuer += "/non-root-path" }) defer httpServer.Close() @@ -1180,7 +1180,7 @@ type oauth2Client struct { // that only valid refresh tokens can be used to refresh an expired token. func TestRefreshTokenFlow(t *testing.T) { state := "state" - now := func() time.Time { return time.Now() } + now := time.Now ctx, cancel := context.WithCancel(context.Background()) defer cancel() @@ -1300,7 +1300,7 @@ func TestOAuth2DeviceFlow(t *testing.T) { // Setup a dex server. httpServer, s := newTestServer(ctx, t, func(c *Config) { - c.Issuer = c.Issuer + "/non-root-path" + c.Issuer += "/non-root-path" c.Now = now c.IDTokensValidFor = idTokensValidFor }) @@ -1314,7 +1314,7 @@ func TestOAuth2DeviceFlow(t *testing.T) { t.Fatalf("failed to get provider: %v", err) } - //Add the Clients to the test server + // Add the Clients to the test server client := storage.Client{ ID: clientID, RedirectURIs: []string{deviceCallbackURI}, @@ -1324,13 +1324,13 @@ func TestOAuth2DeviceFlow(t *testing.T) { t.Fatalf("failed to create client: %v", err) } - //Grab the issuer that we'll reuse for the different endpoints to hit + // Grab the issuer that we'll reuse for the different endpoints to hit issuer, err := url.Parse(s.issuerURL.String()) if err != nil { t.Errorf("Could not parse issuer URL %v", err) } - //Send a new Device Request + // Send a new Device Request codeURL, _ := url.Parse(issuer.String()) codeURL.Path = path.Join(codeURL.Path, "device/code") @@ -1350,13 +1350,13 @@ func TestOAuth2DeviceFlow(t *testing.T) { t.Errorf("%v - Unexpected Response Type. Expected 200 got %v. Response: %v", tc.name, resp.StatusCode, string(responseBody)) } - //Parse the code response + // Parse the code response var deviceCode deviceCodeResponse if err := json.Unmarshal(responseBody, &deviceCode); err != nil { t.Errorf("Unexpected Device Code Response Format %v", string(responseBody)) } - //Mock the user hitting the verification URI and posting the form + // Mock the user hitting the verification URI and posting the form verifyURL, _ := url.Parse(issuer.String()) verifyURL.Path = path.Join(verifyURL.Path, "/device/auth/verify_code") urlData := url.Values{} @@ -1374,7 +1374,7 @@ func TestOAuth2DeviceFlow(t *testing.T) { t.Errorf("%v - Unexpected Response Type. Expected 200 got %v. Response: %v", tc.name, resp.StatusCode, string(responseBody)) } - //Hit the Token Endpoint, and try and get an access token + // Hit the Token Endpoint, and try and get an access token tokenURL, _ := url.Parse(issuer.String()) tokenURL.Path = path.Join(tokenURL.Path, "/device/token") v := url.Values{} @@ -1393,7 +1393,7 @@ func TestOAuth2DeviceFlow(t *testing.T) { t.Errorf("%v - Unexpected Token Response Type. Expected 200 got %v. Response: %v", tc.name, resp.StatusCode, string(responseBody)) } - //Parse the response + // Parse the response var tokenRes accessTokenReponse if err := json.Unmarshal(responseBody, &tokenRes); err != nil { t.Errorf("Unexpected Device Access Token Response Format %v", string(responseBody)) @@ -1411,7 +1411,7 @@ func TestOAuth2DeviceFlow(t *testing.T) { token.Expiry = time.Now().Add(time.Duration(secs) * time.Second) } - //Run token tests to validate info is correct + // Run token tests to validate info is correct // Create the OAuth2 config. oauth2Config := &oauth2.Config{ ClientID: client.ID, diff --git a/server/templates.go b/server/templates.go index 601aaa08..ac921d55 100644 --- a/server/templates.go +++ b/server/templates.go @@ -178,11 +178,11 @@ func loadTemplates(c webConfig, templatesDir string) (*templates, error) { // 3. For each part of reqPath remaining(minus one), go up one level (..) // 4. For each part of assetPath remaining, append it to result // -//eg -//server listens at localhost/dex so serverPath is dex -//reqPath is /dex/auth -//assetPath is static/main.css -//relativeURL("/dex", "/dex/auth", "static/main.css") = "../static/main.css" +// eg +// server listens at localhost/dex so serverPath is dex +// reqPath is /dex/auth +// assetPath is static/main.css +// relativeURL("/dex", "/dex/auth", "static/main.css") = "../static/main.css" func relativeURL(serverPath, reqPath, assetPath string) string { if u, err := url.ParseRequestURI(assetPath); err == nil && u.Scheme != "" { // assetPath points to the external URL, no changes needed diff --git a/storage/conformance/conformance.go b/storage/conformance/conformance.go index a550a530..f3f208e1 100644 --- a/storage/conformance/conformance.go +++ b/storage/conformance/conformance.go @@ -763,10 +763,8 @@ func testGC(t *testing.T, s storage.Storage) { result, err := s.GarbageCollect(expiry.Add(-time.Hour).In(tz)) if err != nil { t.Errorf("garbage collection failed: %v", err) - } else { - if result.AuthCodes != 0 || result.AuthRequests != 0 { - t.Errorf("expected no garbage collection results, got %#v", result) - } + } else if result.AuthCodes != 0 || result.AuthRequests != 0 { + t.Errorf("expected no garbage collection results, got %#v", result) } if _, err := s.GetAuthCode(c.ID); err != nil { t.Errorf("expected to be able to get auth code after GC: %v", err) @@ -815,10 +813,8 @@ func testGC(t *testing.T, s storage.Storage) { result, err := s.GarbageCollect(expiry.Add(-time.Hour).In(tz)) if err != nil { t.Errorf("garbage collection failed: %v", err) - } else { - if result.AuthCodes != 0 || result.AuthRequests != 0 { - t.Errorf("expected no garbage collection results, got %#v", result) - } + } else if result.AuthCodes != 0 || result.AuthRequests != 0 { + t.Errorf("expected no garbage collection results, got %#v", result) } if _, err := s.GetAuthRequest(a.ID); err != nil { t.Errorf("expected to be able to get auth request after GC: %v", err) @@ -859,10 +855,8 @@ func testGC(t *testing.T, s storage.Storage) { result, err := s.GarbageCollect(expiry.Add(-time.Hour).In(tz)) if err != nil { t.Errorf("garbage collection failed: %v", err) - } else { - if result.DeviceRequests != 0 { - t.Errorf("expected no device garbage collection results, got %#v", result) - } + } else if result.DeviceRequests != 0 { + t.Errorf("expected no device garbage collection results, got %#v", result) } if _, err := s.GetDeviceRequest(d.UserCode); err != nil { t.Errorf("expected to be able to get auth request after GC: %v", err) @@ -897,10 +891,8 @@ func testGC(t *testing.T, s storage.Storage) { result, err := s.GarbageCollect(expiry.Add(-time.Hour).In(tz)) if err != nil { t.Errorf("garbage collection failed: %v", err) - } else { - if result.DeviceTokens != 0 { - t.Errorf("expected no device token garbage collection results, got %#v", result) - } + } else if result.DeviceTokens != 0 { + t.Errorf("expected no device token garbage collection results, got %#v", result) } if _, err := s.GetDeviceToken(dt.DeviceCode); err != nil { t.Errorf("expected to be able to get device token after GC: %v", err) @@ -987,12 +979,12 @@ func testDeviceRequestCRUD(t *testing.T, s storage.Storage) { err = s.CreateDeviceRequest(d1) mustBeErrAlreadyExists(t, "device request", err) - //No manual deletes for device requests, will be handled by garbage collection routines - //see testGC + // No manual deletes for device requests, will be handled by garbage collection routines + // see testGC } func testDeviceTokenCRUD(t *testing.T, s storage.Storage) { - //Create a Token + // Create a Token d1 := storage.DeviceToken{ DeviceCode: storage.NewID(), Status: "pending", @@ -1010,7 +1002,7 @@ func testDeviceTokenCRUD(t *testing.T, s storage.Storage) { err := s.CreateDeviceToken(d1) mustBeErrAlreadyExists(t, "device token", err) - //Update the device token, simulate a redemption + // Update the device token, simulate a redemption if err := s.UpdateDeviceToken(d1.DeviceCode, func(old storage.DeviceToken) (storage.DeviceToken, error) { old.Token = "token data" old.Status = "complete" @@ -1019,13 +1011,13 @@ func testDeviceTokenCRUD(t *testing.T, s storage.Storage) { t.Fatalf("failed to update device token: %v", err) } - //Retrieve the device token + // Retrieve the device token got, err := s.GetDeviceToken(d1.DeviceCode) if err != nil { t.Fatalf("failed to get device token: %v", err) } - //Validate expected result set + // Validate expected result set if got.Status != "complete" { t.Fatalf("update failed, wanted token status=%v got %v", "complete", got.Status) } diff --git a/storage/kubernetes/k8sapi/client.go b/storage/kubernetes/k8sapi/client.go index 4e71dd07..97829bdd 100644 --- a/storage/kubernetes/k8sapi/client.go +++ b/storage/kubernetes/k8sapi/client.go @@ -24,7 +24,7 @@ type Config struct { // Legacy field from pkg/api/types.go TypeMeta. // TODO(jlowdermilk): remove this after eliminating downstream dependencies. Kind string `json:"kind,omitempty"` - // DEPRECATED: APIVersion is the preferred api version for communicating with the kubernetes cluster (v1, v2, etc). + // Deprecated: APIVersion is the preferred api version for communicating with the kubernetes cluster (v1, v2, etc). // Because a cluster can run multiple API groups and potentially multiple versions of each, it no longer makes sense to specify // a single value for the cluster version. // This field isn't really needed anyway, so we are deprecating it without replacement. diff --git a/storage/sql/config.go b/storage/sql/config.go index 170dfd00..fc9dca1b 100644 --- a/storage/sql/config.go +++ b/storage/sql/config.go @@ -289,16 +289,19 @@ func (s *MySQL) open(logger log.Logger) (*conn, error) { cfg.Addr = s.Host } } - if s.SSL.CAFile != "" || s.SSL.CertFile != "" || s.SSL.KeyFile != "" { + + switch { + case s.SSL.CAFile != "" || s.SSL.CertFile != "" || s.SSL.KeyFile != "": if err := s.makeTLSConfig(); err != nil { return nil, fmt.Errorf("failed to make TLS config: %v", err) } cfg.TLSConfig = mysqlSSLCustom - } else if s.SSL.Mode == "" { + case s.SSL.Mode == "": cfg.TLSConfig = mysqlSSLTrue - } else { + default: cfg.TLSConfig = s.SSL.Mode } + for k, v := range s.params { cfg.Params[k] = v } diff --git a/storage/static.go b/storage/static.go index 8a383853..806b61f9 100644 --- a/storage/static.go +++ b/storage/static.go @@ -96,7 +96,7 @@ type staticPasswordsStorage struct { func WithStaticPasswords(s Storage, staticPasswords []Password, logger log.Logger) Storage { passwordsByEmail := make(map[string]Password, len(staticPasswords)) for _, p := range staticPasswords { - //Enable case insensitive email comparison. + // Enable case insensitive email comparison. lowerEmail := strings.ToLower(p.Email) if _, ok := passwordsByEmail[lowerEmail]; ok { logger.Errorf("Attempting to create StaticPasswords with the same email id: %s", p.Email) diff --git a/storage/storage.go b/storage/storage.go index 82ce2cb1..a8eeea40 100644 --- a/storage/storage.go +++ b/storage/storage.go @@ -25,7 +25,7 @@ var ( // TODO(ericchiang): refactor ID creation onto the storage. var encoding = base32.NewEncoding("abcdefghijklmnopqrstuvwxyz234567") -//Valid characters for user codes +// Valid characters for user codes const validUserCharacters = "BCDFGHJKLMNPQRSTVWXZ" // NewDeviceCode returns a 32 char alphanumeric cryptographically secure string