From ba2ef44ea06fdd2bbcbe84a881f75f793736a533 Mon Sep 17 00:00:00 2001 From: David Christofas Date: Fri, 28 Jan 2022 12:41:55 +0100 Subject: [PATCH 1/2] fix graph code smells --- graph/pkg/identity/backend.go | 8 ++++---- graph/pkg/identity/cs3.go | 10 +++++++--- graph/pkg/identity/ldap.go | 28 +++++++++++++++++----------- graph/pkg/identity/ldap/reconnect.go | 14 +++++++++----- sonar-project.properties | 2 +- 5 files changed, 38 insertions(+), 24 deletions(-) diff --git a/graph/pkg/identity/backend.go b/graph/pkg/identity/backend.go index d230a6b133..62d9ccd131 100644 --- a/graph/pkg/identity/backend.go +++ b/graph/pkg/identity/backend.go @@ -13,15 +13,15 @@ type Backend interface { CreateUser(ctx context.Context, user libregraph.User) (*libregraph.User, error) // DeleteUser deletes a given user, identified by username or id, from the backend - DeleteUser(ctx context.Context, nameOrId string) error + DeleteUser(ctx context.Context, nameOrID string) error // UpdateUser applies changes to given user, identified by username or id - UpdateUser(ctx context.Context, nameOrId string, user libregraph.User) (*libregraph.User, error) + UpdateUser(ctx context.Context, nameOrID string, user libregraph.User) (*libregraph.User, error) - GetUser(ctx context.Context, nameOrId string) (*libregraph.User, error) + GetUser(ctx context.Context, nameOrID string) (*libregraph.User, error) GetUsers(ctx context.Context, queryParam url.Values) ([]*libregraph.User, error) - GetGroup(ctx context.Context, nameOrId string) (*libregraph.Group, error) + GetGroup(ctx context.Context, nameOrID string) (*libregraph.Group, error) GetGroups(ctx context.Context, queryParam url.Values) ([]*libregraph.Group, error) } diff --git a/graph/pkg/identity/cs3.go b/graph/pkg/identity/cs3.go index 231107d11d..36e6108bd3 100644 --- a/graph/pkg/identity/cs3.go +++ b/graph/pkg/identity/cs3.go @@ -15,6 +15,10 @@ import ( "github.com/owncloud/ocis/ocis-pkg/log" ) +var ( + errNotImplemented = errorcode.New(errorcode.NotSupported, "not implemented") +) + type CS3 struct { Config *config.Reva Logger *log.Logger @@ -22,17 +26,17 @@ type CS3 struct { // CreateUser implements the Backend Interface. It's currently not supported for the CS3 backend func (i *CS3) CreateUser(ctx context.Context, user libregraph.User) (*libregraph.User, error) { - return nil, errorcode.New(errorcode.NotSupported, "not implemented") + return nil, errNotImplemented } // DeleteUser implements the Backend Interface. It's currently not supported for the CS3 backend func (i *CS3) DeleteUser(ctx context.Context, nameOrID string) error { - return errorcode.New(errorcode.NotSupported, "not implemented") + return errNotImplemented } // UpdateUser implements the Backend Interface. It's currently not suported for the CS3 backend func (i *CS3) UpdateUser(ctx context.Context, nameOrID string, user libregraph.User) (*libregraph.User, error) { - return nil, errorcode.New(errorcode.NotSupported, "not implemented") + return nil, errNotImplemented } func (i *CS3) GetUser(ctx context.Context, userID string) (*libregraph.User, error) { diff --git a/graph/pkg/identity/ldap.go b/graph/pkg/identity/ldap.go index bf16afe93c..04c8d7e97c 100644 --- a/graph/pkg/identity/ldap.go +++ b/graph/pkg/identity/ldap.go @@ -2,6 +2,7 @@ package identity import ( "context" + "errors" "fmt" "net/url" @@ -14,6 +15,11 @@ import ( "github.com/owncloud/ocis/ocis-pkg/log" ) +var ( + errReadOnly = errorcode.New(errorcode.NotAllowed, "server is configured read-only") + errNotFound = errorcode.New(errorcode.ItemNotFound, "not found") +) + type LDAP struct { useServerUUID bool writeEnabled bool @@ -47,7 +53,7 @@ type groupAttributeMap struct { func NewLDAPBackend(lc ldap.Client, config config.LDAP, logger *log.Logger) (*LDAP, error) { if config.UserDisplayNameAttribute == "" || config.UserIDAttribute == "" || config.UserEmailAttribute == "" || config.UserNameAttribute == "" { - return nil, fmt.Errorf("Invalid user attribute mappings") + return nil, errors.New("invalid user attribute mappings") } uam := userAttributeMap{ displayName: config.UserDisplayNameAttribute, @@ -57,7 +63,7 @@ func NewLDAPBackend(lc ldap.Client, config config.LDAP, logger *log.Logger) (*LD } if config.GroupNameAttribute == "" || config.GroupIDAttribute == "" { - return nil, fmt.Errorf("Invalid group attribute mappings") + return nil, errors.New("invalid group attribute mappings") } gam := groupAttributeMap{ name: config.GroupNameAttribute, @@ -67,11 +73,11 @@ func NewLDAPBackend(lc ldap.Client, config config.LDAP, logger *log.Logger) (*LD var userScope, groupScope int var err error if userScope, err = stringToScope(config.UserSearchScope); err != nil { - return nil, fmt.Errorf("Error configuring user scope: %w", err) + return nil, fmt.Errorf("error configuring user scope: %w", err) } if groupScope, err = stringToScope(config.GroupSearchScope); err != nil { - return nil, fmt.Errorf("Error configuring group scope: %w", err) + return nil, fmt.Errorf("error configuring group scope: %w", err) } return &LDAP{ @@ -95,7 +101,7 @@ func NewLDAPBackend(lc ldap.Client, config config.LDAP, logger *log.Logger) (*LD // configured LDAP server func (i *LDAP) CreateUser(ctx context.Context, user libregraph.User) (*libregraph.User, error) { if !i.writeEnabled { - return nil, errorcode.New(errorcode.NotAllowed, "server is configured read-only") + return nil, errReadOnly } ar := ldap.AddRequest{ DN: fmt.Sprintf("uid=%s,%s", *user.OnPremisesSamAccountName, i.userBaseDN), @@ -161,7 +167,7 @@ func (i *LDAP) CreateUser(ctx context.Context, user libregraph.User) (*libregrap // by name or id from the LDAP server func (i *LDAP) DeleteUser(ctx context.Context, nameOrID string) error { if !i.writeEnabled { - return errorcode.New(errorcode.NotAllowed, "server is configured read-only") + return errReadOnly } e, err := i.getLDAPUserByNameOrID(nameOrID) if err != nil { @@ -177,7 +183,7 @@ func (i *LDAP) DeleteUser(ctx context.Context, nameOrID string) error { // UpdateUser implements the Backend Interface. It's currently not suported for the CS3 backedn func (i *LDAP) UpdateUser(ctx context.Context, nameOrID string, user libregraph.User) (*libregraph.User, error) { if !i.writeEnabled { - return nil, errorcode.New(errorcode.NotAllowed, "server is configured read-only") + return nil, errReadOnly } e, err := i.getLDAPUserByNameOrID(nameOrID) if err != nil { @@ -249,7 +255,7 @@ func (i *LDAP) getUserByDN(dn string) (*ldap.Entry, error) { return nil, errorcode.New(errorcode.ItemNotFound, err.Error()) } if len(res.Entries) == 0 { - return nil, errorcode.New(errorcode.ItemNotFound, "not found") + return nil, errNotFound } return res.Entries[0], nil @@ -283,7 +289,7 @@ func (i *LDAP) getLDAPUserByNameOrID(nameOrID string) (*ldap.Entry, error) { return nil, errorcode.New(errorcode.ItemNotFound, errmsg) } if len(res.Entries) == 0 { - return nil, errorcode.New(errorcode.ItemNotFound, "not found") + return nil, errNotFound } return res.Entries[0], nil @@ -367,7 +373,7 @@ func (i *LDAP) GetGroup(ctx context.Context, groupID string) (*libregraph.Group, return nil, errorcode.New(errorcode.ItemNotFound, errmsg) } if len(res.Entries) == 0 { - return nil, errorcode.New(errorcode.ItemNotFound, "not found") + return nil, errNotFound } return i.createGroupModelFromLDAP(res.Entries[0]), nil @@ -449,7 +455,7 @@ func stringToScope(scope string) (int, error) { case "base": s = ldap.ScopeBaseObject default: - return 0, fmt.Errorf("Invalid Scope '%s'", scope) + return 0, fmt.Errorf("invalid Scope '%s'", scope) } return s, nil } diff --git a/graph/pkg/identity/ldap/reconnect.go b/graph/pkg/identity/ldap/reconnect.go index 8fba2e6101..8442bbd663 100644 --- a/graph/pkg/identity/ldap/reconnect.go +++ b/graph/pkg/identity/ldap/reconnect.go @@ -14,6 +14,10 @@ import ( "github.com/owncloud/ocis/ocis-pkg/log" ) +var ( + errMaxRetries = errors.New("max retries") +) + type ldapConnection struct { Conn *ldap.Conn Error error @@ -61,7 +65,7 @@ func (c ConnWithReconnect) Search(sr *ldap.SearchRequest) (*ldap.SearchResult, e c.logger.Debug().Msg("retrying LDAP Search") } // if we get here we reached the maximum retries. So return an error - return nil, ldap.NewError(ldap.ErrorNetwork, errors.New("max retries")) + return nil, ldap.NewError(ldap.ErrorNetwork, errMaxRetries) } func (c ConnWithReconnect) Add(a *ldap.AddRequest) error { @@ -84,7 +88,7 @@ func (c ConnWithReconnect) Add(a *ldap.AddRequest) error { c.logger.Debug().Msg("retrying LDAP Add") } // if we get here we reached the maximum retries. So return an error - return ldap.NewError(ldap.ErrorNetwork, errors.New("max retries")) + return ldap.NewError(ldap.ErrorNetwork, errMaxRetries) } func (c ConnWithReconnect) Del(d *ldap.DelRequest) error { @@ -108,7 +112,7 @@ func (c ConnWithReconnect) Del(d *ldap.DelRequest) error { c.logger.Debug().Msg("retrying LDAP Del") } // if we get here we reached the maximum retries. So return an error - return ldap.NewError(ldap.ErrorNetwork, errors.New("max retries")) + return ldap.NewError(ldap.ErrorNetwork, errMaxRetries) } func (c ConnWithReconnect) Modify(m *ldap.ModifyRequest) error { @@ -132,7 +136,7 @@ func (c ConnWithReconnect) Modify(m *ldap.ModifyRequest) error { c.logger.Debug().Msg("retrying LDAP Modify") } // if we get here we reached the maximum retries. So return an error - return ldap.NewError(ldap.ErrorNetwork, errors.New("max retries")) + return ldap.NewError(ldap.ErrorNetwork, errMaxRetries) } func (c ConnWithReconnect) ModifyDN(m *ldap.ModifyDNRequest) error { @@ -156,7 +160,7 @@ func (c ConnWithReconnect) ModifyDN(m *ldap.ModifyDNRequest) error { c.logger.Debug().Msg("retrying LDAP ModifyDN") } // if we get here we reached the maximum retries. So return an error - return ldap.NewError(ldap.ErrorNetwork, errors.New("max retries")) + return ldap.NewError(ldap.ErrorNetwork, errMaxRetries) } func (c ConnWithReconnect) GetConnection() (*ldap.Conn, error) { diff --git a/sonar-project.properties b/sonar-project.properties index 41c6a0089a..4b378d31e6 100644 --- a/sonar-project.properties +++ b/sonar-project.properties @@ -36,5 +36,5 @@ sonar.go.coverage.reportPaths=cache/coverage/* sonar.go.golangci-lint.reportPaths=cache/checkstyle/accounts_checkstyle.xml,cache/checkstyle/glauth_checkstyle.xml,cache/checkstyle/graph_checkstyle.xml,cache/checkstyle/graph-explorer_checkstyle.xml,cache/checkstyle/idp_checkstyle.xml,cache/checkstyle/ocis_checkstyle.xml,cache/checkstyle/ocis-pkg_checkstyle.xml,cache/checkstyle/ocs_checkstyle.xml,cache/checkstyle/proxy_checkstyle.xml,cache/checkstyle/settings_checkstyle.xml,cache/checkstyle/storage_checkstyle.xml,cache/checkstyle/store_checkstyle.xml,cache/checkstyle/thumbnails_checkstyle.xml,cache/checkstyle/web_checkstyle.xml,cache/checkstyle/webdav_checkstyle.xml # Exclude files -sonar.exclusions=**/third_party,docs/**,changelog/**,*/pkg/assets/embed.go,idp/assets/identifier/**,**/package.json,**/rollup.config.js,CHANGELOG.md,**/pkg/proto/**/*.pb.*,deployments/**,tests/**,vendor-bin/**,README.md +sonar.exclusions=**/third_party,docs/**,changelog/**,*/pkg/assets/embed.go,idp/assets/identifier/**,**/package.json,**/rollup.config.js,CHANGELOG.md,**/pkg/proto/**/*.pb.*,deployments/**,tests/**,vendor-bin/**,README.md,**/mocks/ sonar.coverage.exclusions=**/*_test.go From 28496e558b64e5f323398b7cd429e22647934bcd Mon Sep 17 00:00:00 2001 From: David Christofas Date: Fri, 28 Jan 2022 13:48:34 +0100 Subject: [PATCH 2/2] fix storage code smells --- storage/pkg/config/defaultconfig.go | 65 ++++++++++++++++------------- 1 file changed, 37 insertions(+), 28 deletions(-) diff --git a/storage/pkg/config/defaultconfig.go b/storage/pkg/config/defaultconfig.go index 23602bbf16..3afa311aae 100644 --- a/storage/pkg/config/defaultconfig.go +++ b/storage/pkg/config/defaultconfig.go @@ -7,6 +7,15 @@ import ( "github.com/owncloud/ocis/ocis-pkg/config/defaults" ) +const ( + defaultPublicURL = "https://localhost:9200" + defaultShareFolder = "/Shares" + defaultStorageNamespace = "/users/{{.Id.OpaqueId}}" + defaultGatewayAddr = "127.0.0.1:9142" + defaultUserLayout = "{{.Id.OpaqueId}}" + defaultServiceUserUUID = "95cb8724-03b2-11eb-a0a6-c33ef8ef53ad" +) + func DefaultConfig() *Config { return &Config{ // log is inherited @@ -19,7 +28,7 @@ func DefaultConfig() *Config { TransferSecret: "replace-me-with-a-transfer-secret", TransferExpires: 24 * 60 * 60, OIDC: OIDC{ - Issuer: "https://localhost:9200", + Issuer: defaultPublicURL, Insecure: false, IDClaim: "preferred_username", }, @@ -40,7 +49,7 @@ func DefaultConfig() *Config { GroupMemberFilter: "(&(objectclass=posixAccount)(ownclouduuid={{.OpaqueId}}*))", BindDN: "cn=reva,ou=sysusers,dc=ocis,dc=test", BindPassword: "reva", - IDP: "https://localhost:9200", + IDP: defaultPublicURL, UserSchema: LDAPUserSchema{ UID: "ownclouduuid", Mail: "mail", @@ -66,15 +75,15 @@ func DefaultConfig() *Config { DBHost: "mysql", DBPort: 3306, DBName: "owncloud", - Idp: "https://localhost:9200", + Idp: defaultPublicURL, Nobody: 90, JoinUsername: false, JoinOwnCloudUUID: false, EnableMedialSearch: false, }, OCDav: OCDav{ - WebdavNamespace: "/users/{{.Id.OpaqueId}}", - DavFilesNamespace: "/users/{{.Id.OpaqueId}}", + WebdavNamespace: defaultStorageNamespace, + DavFilesNamespace: defaultStorageNamespace, }, Archiver: Archiver{ MaxNumFiles: 10000, @@ -85,7 +94,7 @@ func DefaultConfig() *Config { EOS: DriverEOS{ DriverCommon: DriverCommon{ Root: "/eos/dockertest/reva", - ShareFolder: "/Shares", + ShareFolder: defaultShareFolder, UserLayout: "{{substr 0 1 .Username}}/{{.Username}}", }, ShadowNamespace: "", // Defaults to path.Join(c.Namespace, ".shadow") @@ -95,19 +104,19 @@ func DefaultConfig() *Config { MasterURL: "root://eos-mgm1.eoscluster.cern.ch:1094", SlaveURL: "root://eos-mgm1.eoscluster.cern.ch:1094", CacheDirectory: os.TempDir(), - GatewaySVC: "127.0.0.1:9142", + GatewaySVC: defaultGatewayAddr, }, Local: DriverCommon{ Root: path.Join(defaults.BaseDataPath(), "storage", "local", "users"), - ShareFolder: "/Shares", + ShareFolder: defaultShareFolder, UserLayout: "{{.Username}}", EnableHome: false, }, OwnCloud: DriverOwnCloud{ DriverCommon: DriverCommon{ Root: path.Join(defaults.BaseDataPath(), "storage", "owncloud"), - ShareFolder: "/Shares", - UserLayout: "{{.Id.OpaqueId}}", + ShareFolder: defaultShareFolder, + UserLayout: defaultUserLayout, EnableHome: false, }, UploadInfoDir: path.Join(defaults.BaseDataPath(), "storage", "uploadinfo"), @@ -117,7 +126,7 @@ func DefaultConfig() *Config { OwnCloudSQL: DriverOwnCloudSQL{ DriverCommon: DriverCommon{ Root: path.Join(defaults.BaseDataPath(), "storage", "owncloud"), - ShareFolder: "/Shares", + ShareFolder: defaultShareFolder, UserLayout: "{{.Username}}", EnableHome: false, }, @@ -139,11 +148,11 @@ func DefaultConfig() *Config { S3NG: DriverS3NG{ DriverCommon: DriverCommon{ Root: path.Join(defaults.BaseDataPath(), "storage", "users"), - ShareFolder: "/Shares", - UserLayout: "{{.Id.OpaqueId}}", + ShareFolder: defaultShareFolder, + UserLayout: defaultUserLayout, EnableHome: false, }, - ServiceUserUUID: "95cb8724-03b2-11eb-a0a6-c33ef8ef53ad", + ServiceUserUUID: defaultServiceUserUUID, Region: "default", AccessKey: "", SecretKey: "", @@ -153,17 +162,17 @@ func DefaultConfig() *Config { OCIS: DriverOCIS{ DriverCommon: DriverCommon{ Root: path.Join(defaults.BaseDataPath(), "storage", "users"), - ShareFolder: "/Shares", - UserLayout: "{{.Id.OpaqueId}}", + ShareFolder: defaultShareFolder, + UserLayout: defaultUserLayout, }, - ServiceUserUUID: "95cb8724-03b2-11eb-a0a6-c33ef8ef53ad", + ServiceUserUUID: defaultServiceUserUUID, }, }, MetadataStorage: StorageConfig{ EOS: DriverEOS{ DriverCommon: DriverCommon{ Root: "/eos/dockertest/reva", - ShareFolder: "/Shares", + ShareFolder: defaultShareFolder, UserLayout: "{{substr 0 1 .Username}}/{{.Username}}", EnableHome: false, }, @@ -182,7 +191,7 @@ func DefaultConfig() *Config { SecProtocol: "", Keytab: "", SingleUsername: "", - GatewaySVC: "127.0.0.1:9142", + GatewaySVC: defaultGatewayAddr, }, Local: DriverCommon{ Root: path.Join(defaults.BaseDataPath(), "storage", "local", "metadata"), @@ -197,10 +206,10 @@ func DefaultConfig() *Config { DriverCommon: DriverCommon{ Root: path.Join(defaults.BaseDataPath(), "storage", "metadata"), ShareFolder: "", - UserLayout: "{{.Id.OpaqueId}}", + UserLayout: defaultUserLayout, EnableHome: false, }, - ServiceUserUUID: "95cb8724-03b2-11eb-a0a6-c33ef8ef53ad", + ServiceUserUUID: defaultServiceUserUUID, Region: "default", AccessKey: "", SecretKey: "", @@ -211,10 +220,10 @@ func DefaultConfig() *Config { DriverCommon: DriverCommon{ Root: path.Join(defaults.BaseDataPath(), "storage", "metadata"), ShareFolder: "", - UserLayout: "{{.Id.OpaqueId}}", + UserLayout: defaultUserLayout, EnableHome: false, }, - ServiceUserUUID: "95cb8724-03b2-11eb-a0a6-c33ef8ef53ad", + ServiceUserUUID: defaultServiceUserUUID, }, }, Frontend: FrontendPort{ @@ -243,9 +252,9 @@ func DefaultConfig() *Config { OCDavInsecure: false, // true? OCDavPrefix: "", OCSPrefix: "ocs", - OCSSharePrefix: "/Shares", - OCSHomeNamespace: "/users/{{.Id.OpaqueId}}", - PublicURL: "https://localhost:9200", + OCSSharePrefix: defaultShareFolder, + OCSHomeNamespace: defaultStorageNamespace, + PublicURL: defaultPublicURL, OCSCacheWarmupDriver: "", OCSAdditionalInfoAttribute: "{{.Mail}}", OCSResourceInfoCacheTTL: 0, @@ -257,10 +266,10 @@ func DefaultConfig() *Config { }, Gateway: Gateway{ Port: Port{ - Endpoint: "127.0.0.1:9142", + Endpoint: defaultGatewayAddr, DebugAddr: "127.0.0.1:9143", GRPCNetwork: "tcp", - GRPCAddr: "127.0.0.1:9142", + GRPCAddr: defaultGatewayAddr, }, CommitShareToStorageGrant: true, CommitShareToStorageRef: true,