From b2e9f67edc793cd7743ca67a1674416a86d27a8e Mon Sep 17 00:00:00 2001 From: "m.nabokikh" Date: Fri, 15 Jan 2021 19:22:38 +0400 Subject: [PATCH] Enable unparam, prealloc, sqlclosecheck linters Signed-off-by: m.nabokikh --- .golangci.yml | 6 +++--- connector/atlassiancrowd/atlassiancrowd.go | 14 ++++---------- .../atlassiancrowd/atlassiancrowd_test.go | 18 +++++------------- connector/bitbucketcloud/bitbucketcloud.go | 2 +- connector/gitea/gitea.go | 2 +- connector/ldap/ldap.go | 2 +- connector/saml/saml.go | 2 +- server/api.go | 6 +++--- server/deviceflowhandlers.go | 6 +----- server/server.go | 10 ++++------ storage/conformance/conformance.go | 17 ++++------------- storage/etcd/etcd.go | 12 ++++++------ storage/kubernetes/k8sapi/client.go | 7 ++++--- storage/sql/crud.go | 7 +++++++ storage/storage.go | 13 +++++-------- 15 files changed, 50 insertions(+), 74 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index fa8f5045..e182d624 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -32,13 +32,16 @@ linters: - misspell - nakedret - nolintlint + - prealloc - rowserrcheck + - sqlclosecheck - staticcheck - structcheck - stylecheck - tparallel - typecheck - unconvert + - unparam - unused - varcheck - whitespace @@ -51,8 +54,6 @@ linters: # - godot # - nlreturn # - noctx - # - prealloc - # - sqlclosecheck # - wrapcheck # TODO: fix linter errors before enabling (from original config) @@ -63,7 +64,6 @@ linters: # - gosec # - lll # - scopelint - # - unparam # unused # - depguard diff --git a/connector/atlassiancrowd/atlassiancrowd.go b/connector/atlassiancrowd/atlassiancrowd.go index dbc60a74..6f034060 100644 --- a/connector/atlassiancrowd/atlassiancrowd.go +++ b/connector/atlassiancrowd/atlassiancrowd.go @@ -128,10 +128,7 @@ func (c *crowdConnector) Login(ctx context.Context, s connector.Scopes, username return connector.Identity{}, false, err } - if ident, err = c.identityFromCrowdUser(user); err != nil { - return connector.Identity{}, false, err - } - + ident = c.identityFromCrowdUser(user) if s.Groups { userGroups, err := c.getGroups(ctx, client, s.Groups, ident.Username) if err != nil { @@ -165,10 +162,7 @@ func (c *crowdConnector) Refresh(ctx context.Context, s connector.Scopes, ident return ident, fmt.Errorf("crowd: get user %q: %v", data.Username, err) } - newIdent, err := c.identityFromCrowdUser(user) - if err != nil { - return ident, err - } + newIdent := c.identityFromCrowdUser(user) newIdent.ConnectorData = ident.ConnectorData // If user exists, authenticate it to prolong sso session. @@ -366,7 +360,7 @@ func (c *crowdConnector) groups(ctx context.Context, client *http.Client, userna } // identityFromCrowdUser converts crowdUser to Identity -func (c *crowdConnector) identityFromCrowdUser(user crowdUser) (connector.Identity, error) { +func (c *crowdConnector) identityFromCrowdUser(user crowdUser) connector.Identity { identity := connector.Identity{ Username: user.Name, UserID: user.Key, @@ -387,7 +381,7 @@ func (c *crowdConnector) identityFromCrowdUser(user crowdUser) (connector.Identi } } - return identity, nil + return identity } // getGroups retrieves a list of user's groups and filters it diff --git a/connector/atlassiancrowd/atlassiancrowd_test.go b/connector/atlassiancrowd/atlassiancrowd_test.go index 213c1829..3559445d 100644 --- a/connector/atlassiancrowd/atlassiancrowd_test.go +++ b/connector/atlassiancrowd/atlassiancrowd_test.go @@ -71,9 +71,6 @@ func TestUserLoginFlow(t *testing.T) { expectEquals(t, user.Name, "testuser") expectEquals(t, user.Email, "testuser@example.com") - _, err = c.identityFromCrowdUser(user) - expectNil(t, err) - err = c.authenticateUser(context.Background(), newClient(), "testuser") expectNil(t, err) @@ -119,8 +116,7 @@ func TestIdentityFromCrowdUser(t *testing.T) { expectEquals(t, user.Email, "testuser@example.com") // Test unconfigured behaviour - i, err := c.identityFromCrowdUser(user) - expectNil(t, err) + i := c.identityFromCrowdUser(user) expectEquals(t, i.UserID, "12345") expectEquals(t, i.Username, "testuser") expectEquals(t, i.Email, "testuser@example.com") @@ -131,23 +127,19 @@ func TestIdentityFromCrowdUser(t *testing.T) { expectEquals(t, i.PreferredUsername, "") c.Config.PreferredUsernameField = "key" - i, err = c.identityFromCrowdUser(user) - expectNil(t, err) + i = c.identityFromCrowdUser(user) expectEquals(t, i.PreferredUsername, "12345") c.Config.PreferredUsernameField = "name" - i, err = c.identityFromCrowdUser(user) - expectNil(t, err) + i = c.identityFromCrowdUser(user) expectEquals(t, i.PreferredUsername, "testuser") c.Config.PreferredUsernameField = "email" - i, err = c.identityFromCrowdUser(user) - expectNil(t, err) + i = c.identityFromCrowdUser(user) expectEquals(t, i.PreferredUsername, "testuser@example.com") c.Config.PreferredUsernameField = "invalidstring" - i, err = c.identityFromCrowdUser(user) - expectNil(t, err) + i = c.identityFromCrowdUser(user) expectEquals(t, i.PreferredUsername, "") } diff --git a/connector/bitbucketcloud/bitbucketcloud.go b/connector/bitbucketcloud/bitbucketcloud.go index 039084fe..e81893da 100644 --- a/connector/bitbucketcloud/bitbucketcloud.go +++ b/connector/bitbucketcloud/bitbucketcloud.go @@ -421,7 +421,6 @@ type group struct { } func (b *bitbucketConnector) userTeamGroups(ctx context.Context, client *http.Client, teamName string) ([]string, error) { - var teamGroups []string apiURL := b.legacyAPIURL + "/groups/" + teamName var response []group @@ -429,6 +428,7 @@ func (b *bitbucketConnector) userTeamGroups(ctx context.Context, client *http.Cl return nil, fmt.Errorf("get user team %q groups: %v", teamName, err) } + teamGroups := make([]string, 0, len(response)) for _, group := range response { teamGroups = append(teamGroups, teamName+"/"+group.Slug) } diff --git a/connector/gitea/gitea.go b/connector/gitea/gitea.go index c7da2f1c..33cc3126 100644 --- a/connector/gitea/gitea.go +++ b/connector/gitea/gitea.go @@ -72,7 +72,7 @@ type giteaConnector struct { useLoginAsID bool } -func (c *giteaConnector) oauth2Config(scopes connector.Scopes) *oauth2.Config { +func (c *giteaConnector) oauth2Config(_ connector.Scopes) *oauth2.Config { giteaEndpoint := oauth2.Endpoint{AuthURL: c.baseURL + "/login/oauth/authorize", TokenURL: c.baseURL + "/login/oauth/access_token"} return &oauth2.Config{ ClientID: c.clientID, diff --git a/connector/ldap/ldap.go b/connector/ldap/ldap.go index 37937f85..2325b25a 100644 --- a/connector/ldap/ldap.go +++ b/connector/ldap/ldap.go @@ -609,7 +609,7 @@ func (c *ldapConnector) groups(ctx context.Context, user ldap.Entry) ([]string, } } - var groupNames []string + groupNames := make([]string, 0, len(groups)) for _, group := range groups { name := getAttr(*group, c.GroupSearch.NameAttr) if name == "" { diff --git a/connector/saml/saml.go b/connector/saml/saml.go index 877bf4ac..0d52b131 100644 --- a/connector/saml/saml.go +++ b/connector/saml/saml.go @@ -488,7 +488,7 @@ func (p *provider) validateSubject(subject *subject, inResponseTo string) error return fmt.Errorf("subject contained no SubjectConfirmations") } - var errs []error + errs := make([]error, 0, len(subject.SubjectConfirmations)) // One of these must match our assumptions, not all. for _, c := range subject.SubjectConfirmations { err := func() error { diff --git a/server/api.go b/server/api.go index 98fd20b8..65315061 100644 --- a/server/api.go +++ b/server/api.go @@ -233,7 +233,7 @@ func (d dexAPI) ListPasswords(ctx context.Context, req *api.ListPasswordReq) (*a return nil, fmt.Errorf("list passwords: %v", err) } - var passwords []*api.Password + passwords := make([]*api.Password, 0, len(passwordList)) for _, password := range passwordList { p := api.Password{ Email: password.Email, @@ -286,20 +286,20 @@ func (d dexAPI) ListRefresh(ctx context.Context, req *api.ListRefreshReq) (*api. return nil, err } - var refreshTokenRefs []*api.RefreshTokenRef offlineSessions, err := d.s.GetOfflineSessions(id.UserId, id.ConnId) if err != nil { if err == storage.ErrNotFound { // This means that this user-client pair does not have a refresh token yet. // An empty list should be returned instead of an error. return &api.ListRefreshResp{ - RefreshTokens: refreshTokenRefs, + RefreshTokens: nil, }, nil } d.logger.Errorf("api: failed to list refresh tokens %t here : %v", err == storage.ErrNotFound, err) return nil, err } + refreshTokenRefs := make([]*api.RefreshTokenRef, 0, len(offlineSessions.Refresh)) for _, session := range offlineSessions.Refresh { r := api.RefreshTokenRef{ Id: session.ID, diff --git a/server/deviceflowhandlers.go b/server/deviceflowhandlers.go index e0932c54..7ed08f99 100644 --- a/server/deviceflowhandlers.go +++ b/server/deviceflowhandlers.go @@ -77,11 +77,7 @@ func (s *Server) handleDeviceCode(w http.ResponseWriter, r *http.Request) { deviceCode := storage.NewDeviceCode() // 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) - } + userCode := storage.NewUserCode() // Generate the expire time expireTime := time.Now().Add(s.deviceRequestsValidFor) diff --git a/server/server.go b/server/server.go index 776939e7..6fd4d8b7 100644 --- a/server/server.go +++ b/server/server.go @@ -252,10 +252,8 @@ func newServer(ctx context.Context, c Config, rotationStrategy rotationStrategy) } } - instrumentHandlerCounter := func(handlerName string, handler http.Handler) http.HandlerFunc { - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - handler.ServeHTTP(w, r) - }) + instrumentHandlerCounter := func(_ string, handler http.Handler) http.HandlerFunc { + return handler.ServeHTTP } if c.PrometheusRegistry != nil { @@ -270,10 +268,10 @@ func newServer(ctx context.Context, c Config, rotationStrategy rotationStrategy) } instrumentHandlerCounter = func(handlerName string, handler http.Handler) http.HandlerFunc { - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + return func(w http.ResponseWriter, r *http.Request) { m := httpsnoop.CaptureMetrics(handler, w, r) requestCounter.With(prometheus.Labels{"handler": handlerName, "code": strconv.Itoa(m.Code), "method": r.Method}).Inc() - }) + } } } diff --git a/storage/conformance/conformance.go b/storage/conformance/conformance.go index 563a4030..3f5e2aa1 100644 --- a/storage/conformance/conformance.go +++ b/storage/conformance/conformance.go @@ -285,7 +285,7 @@ func testClientCRUD(t *testing.T, s storage.Storage) { t.Fatalf("create client: %v", err) } - getAndCompare := func(id string, want storage.Client) { + getAndCompare := func(_ string, want storage.Client) { gc, err := s.GetClient(id1) if err != nil { t.Errorf("get client: %v", err) @@ -845,13 +845,8 @@ func testGC(t *testing.T, s storage.Storage) { t.Errorf("expected storage.ErrNotFound, got %v", err) } - userCode, err := storage.NewUserCode() - if err != nil { - t.Errorf("Unexpected Error: %v", err) - } - d := storage.DeviceRequest{ - UserCode: userCode, + UserCode: storage.NewUserCode(), DeviceCode: storage.NewID(), ClientID: "client1", ClientSecret: "secret1", @@ -970,12 +965,8 @@ func testTimezones(t *testing.T, s storage.Storage) { } func testDeviceRequestCRUD(t *testing.T, s storage.Storage) { - userCode, err := storage.NewUserCode() - if err != nil { - panic(err) - } d1 := storage.DeviceRequest{ - UserCode: userCode, + UserCode: storage.NewUserCode(), DeviceCode: storage.NewID(), ClientID: "client1", ClientSecret: "secret1", @@ -988,7 +979,7 @@ func testDeviceRequestCRUD(t *testing.T, s storage.Storage) { } // Attempt to create same DeviceRequest twice. - err = s.CreateDeviceRequest(d1) + err := s.CreateDeviceRequest(d1) mustBeErrAlreadyExists(t, "device request", err) // No manual deletes for device requests, will be handled by garbage collection routines diff --git a/storage/etcd/etcd.go b/storage/etcd/etcd.go index 6f320e22..97aced45 100644 --- a/storage/etcd/etcd.go +++ b/storage/etcd/etcd.go @@ -338,13 +338,13 @@ func (c *conn) ListPasswords() (passwords []storage.Password, err error) { func (c *conn) CreateOfflineSessions(s storage.OfflineSessions) error { ctx, cancel := context.WithTimeout(context.Background(), defaultStorageTimeout) defer cancel() - return c.txnCreate(ctx, keySession(offlineSessionPrefix, s.UserID, s.ConnID), fromStorageOfflineSessions(s)) + return c.txnCreate(ctx, keySession(s.UserID, s.ConnID), fromStorageOfflineSessions(s)) } func (c *conn) UpdateOfflineSessions(userID string, connID string, updater func(s storage.OfflineSessions) (storage.OfflineSessions, error)) error { ctx, cancel := context.WithTimeout(context.Background(), defaultStorageTimeout) defer cancel() - return c.txnUpdate(ctx, keySession(offlineSessionPrefix, userID, connID), func(currentValue []byte) ([]byte, error) { + return c.txnUpdate(ctx, keySession(userID, connID), func(currentValue []byte) ([]byte, error) { var current OfflineSessions if len(currentValue) > 0 { if err := json.Unmarshal(currentValue, ¤t); err != nil { @@ -363,7 +363,7 @@ func (c *conn) GetOfflineSessions(userID string, connID string) (s storage.Offli ctx, cancel := context.WithTimeout(context.Background(), defaultStorageTimeout) defer cancel() var os OfflineSessions - if err = c.getKey(ctx, keySession(offlineSessionPrefix, userID, connID), &os); err != nil { + if err = c.getKey(ctx, keySession(userID, connID), &os); err != nil { return } return toStorageOfflineSessions(os), nil @@ -372,7 +372,7 @@ func (c *conn) GetOfflineSessions(userID string, connID string) (s storage.Offli func (c *conn) DeleteOfflineSessions(userID string, connID string) error { ctx, cancel := context.WithTimeout(context.Background(), defaultStorageTimeout) defer cancel() - return c.deleteKey(ctx, keySession(offlineSessionPrefix, userID, connID)) + return c.deleteKey(ctx, keySession(userID, connID)) } func (c *conn) CreateConnector(connector storage.Connector) error { @@ -564,8 +564,8 @@ func (c *conn) txnUpdate(ctx context.Context, key string, update func(current [] func keyID(prefix, id string) string { return prefix + id } func keyEmail(prefix, email string) string { return prefix + strings.ToLower(email) } -func keySession(prefix, userID, connID string) string { - return prefix + strings.ToLower(userID+"|"+connID) +func keySession(userID, connID string) string { + return offlineSessionPrefix + strings.ToLower(userID+"|"+connID) } func (c *conn) CreateDeviceRequest(d storage.DeviceRequest) error { diff --git a/storage/kubernetes/k8sapi/client.go b/storage/kubernetes/k8sapi/client.go index 261f7007..9320a297 100644 --- a/storage/kubernetes/k8sapi/client.go +++ b/storage/kubernetes/k8sapi/client.go @@ -19,7 +19,7 @@ package k8sapi // Where possible, json tags match the cli argument names. // Top level config objects and all values required for proper functioning are not "omitempty". Any truly optional piece of config is allowed to be omitted. -// Config holds the information needed to build connect to remote kubernetes clusters as a given user +// Config holds the information needed to build connect to remote kubernetes clusters as a given user. type Config struct { // Legacy field from pkg/api/types.go TypeMeta. // TODO(jlowdermilk): remove this after eliminating downstream dependencies. @@ -51,7 +51,7 @@ type Preferences struct { Extensions []NamedExtension `json:"extensions,omitempty"` } -// Cluster contains information about how to communicate with a kubernetes cluster +// Cluster contains information about how to communicate with a kubernetes cluster. type Cluster struct { // Server is the address of the kubernetes cluster (https://hostname:port). Server string `json:"server"` @@ -97,7 +97,8 @@ type AuthInfo struct { Extensions []NamedExtension `json:"extensions,omitempty"` } -// Context is a tuple of references to a cluster (how do I communicate with a kubernetes cluster), a user (how do I identify myself), and a namespace (what subset of resources do I want to work with) +// Context is a tuple of references to a cluster (how do I communicate with a kubernetes cluster), +// a user (how do I identify myself), and a namespace (what subset of resources do I want to work with). type Context struct { // Cluster is the name of the cluster for this context Cluster string `json:"cluster"` diff --git a/storage/sql/crud.go b/storage/sql/crud.go index c804fc43..4451e5c5 100644 --- a/storage/sql/crud.go +++ b/storage/sql/crud.go @@ -378,6 +378,8 @@ func (c *conn) ListRefreshTokens() ([]storage.RefreshToken, error) { if err != nil { return nil, fmt.Errorf("query: %v", err) } + defer rows.Close() + var tokens []storage.RefreshToken for rows.Next() { r, err := scanRefresh(rows) @@ -556,6 +558,8 @@ func (c *conn) ListClients() ([]storage.Client, error) { if err != nil { return nil, err } + defer rows.Close() + var clients []storage.Client for rows.Next() { cli, err := scanClient(rows) @@ -652,6 +656,7 @@ func (c *conn) ListPasswords() ([]storage.Password, error) { if err != nil { return nil, err } + defer rows.Close() var passwords []storage.Password for rows.Next() { @@ -837,6 +842,8 @@ func (c *conn) ListConnectors() ([]storage.Connector, error) { if err != nil { return nil, err } + defer rows.Close() + var connectors []storage.Connector for rows.Next() { conn, err := scanConnector(rows) diff --git a/storage/storage.go b/storage/storage.go index 3a57d522..c308ac46 100644 --- a/storage/storage.go +++ b/storage/storage.go @@ -386,22 +386,19 @@ type Keys struct { // NewUserCode returns a randomized 8 character user code for the device flow. // No vowels are included to prevent accidental generation of words -func NewUserCode() (string, error) { - code, err := randomString(8) - if err != nil { - return "", err - } - return code[:4] + "-" + code[4:], nil +func NewUserCode() string { + code := randomString(8) + return code[:4] + "-" + code[4:] } -func randomString(n int) (string, error) { +func randomString(n int) string { v := big.NewInt(int64(len(validUserCharacters))) bytes := make([]byte, n) for i := 0; i < n; i++ { c, _ := rand.Int(rand.Reader, v) bytes[i] = validUserCharacters[c.Int64()] } - return string(bytes), nil + return string(bytes) } // DeviceRequest represents an OIDC device authorization request. It holds the state of a device request until the user