From 1fd3032f02f56f89d0c6718bde677f4c9a7bfd04 Mon Sep 17 00:00:00 2001 From: Michael Barz Date: Wed, 6 Dec 2023 09:25:06 +0100 Subject: [PATCH 1/7] style: cleanup code style --- services/graph/pkg/service/v0/driveitems.go | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/services/graph/pkg/service/v0/driveitems.go b/services/graph/pkg/service/v0/driveitems.go index 73090d679e..78243d9802 100644 --- a/services/graph/pkg/service/v0/driveitems.go +++ b/services/graph/pkg/service/v0/driveitems.go @@ -618,10 +618,9 @@ func (g Graph) DeletePermission(w http.ResponseWriter, r *http.Request) { } permissionID, err := url.PathUnescape(chi.URLParam(r, "permissionID")) - if err != nil { - g.logger.Debug().Err(err).Msg("could not parse driveID") - errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "invalid driveID") + g.logger.Debug().Err(err).Msg("could not parse permissionID") + errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "invalid permissionID") return } @@ -632,7 +631,7 @@ func (g Graph) DeletePermission(w http.ResponseWriter, r *http.Request) { sharedResourceId, err := g.getUserPermissionResourceID(ctx, permissionID) var errcode *errorcode.Error if err != nil && errors.As(err, &errcode) && errcode.GetCode() == errorcode.ItemNotFound { - // there is no user share with that ID, so lets check if it is refering to a public link + // there is no user share with that ID, so lets check if it is referring to a public link isUserPermission = false sharedResourceId, err = g.getLinkPermissionResourceID(ctx, permissionID) } @@ -644,9 +643,7 @@ func (g Graph) DeletePermission(w http.ResponseWriter, r *http.Request) { // The resourceID of the shared resource need to match the item ID from the Request Path // otherwise this is an invalid Request. - if sharedResourceId.GetStorageId() != itemID.GetStorageId() || - sharedResourceId.GetSpaceId() != itemID.GetSpaceId() || - sharedResourceId.GetOpaqueId() != itemID.GetOpaqueId() { + if !utils.ResourceIDEqual(sharedResourceId, &itemID) { g.logger.Debug().Msg("resourceID of shared does not match itemID") errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "permissionID and itemID do not match") return From 0b7162204ac35660802d2b7e1f73d27d1071b416 Mon Sep 17 00:00:00 2001 From: Michael Barz Date: Wed, 6 Dec 2023 09:26:48 +0100 Subject: [PATCH 2/7] feat: handle more status codes --- services/graph/pkg/service/v0/sharedbyme.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/services/graph/pkg/service/v0/sharedbyme.go b/services/graph/pkg/service/v0/sharedbyme.go index 3c45c8c390..085b44c4a4 100644 --- a/services/graph/pkg/service/v0/sharedbyme.go +++ b/services/graph/pkg/service/v0/sharedbyme.go @@ -219,6 +219,10 @@ func cs3StatusToErrCode(code rpc.Code) (errcode errorcode.ErrorCode) { errcode = errorcode.ItemNotFound case rpc.Code_CODE_LOCKED: errcode = errorcode.ItemIsLocked + case rpc.Code_CODE_INVALID_ARGUMENT: + errcode = errorcode.InvalidRequest + case rpc.Code_CODE_FAILED_PRECONDITION: + errcode = errorcode.InvalidRequest default: errcode = errorcode.GeneralException } From 6755aef725c725148279cc9f0de9d26c026e5122 Mon Sep 17 00:00:00 2001 From: Michael Barz Date: Wed, 6 Dec 2023 09:28:17 +0100 Subject: [PATCH 3/7] feat(config): password policy and gateway addr --- services/frontend/pkg/config/config.go | 12 +-- services/sharing/pkg/command/server.go | 5 +- services/sharing/pkg/config/config.go | 13 ++++ .../pkg/config/defaults/defaultconfig.go | 10 ++- services/sharing/pkg/revaconfig/config.go | 74 ++++++++++++++++--- 5 files changed, 97 insertions(+), 17 deletions(-) diff --git a/services/frontend/pkg/config/config.go b/services/frontend/pkg/config/config.go index 0975ac68a7..d4bb9c62c1 100644 --- a/services/frontend/pkg/config/config.go +++ b/services/frontend/pkg/config/config.go @@ -179,10 +179,10 @@ type ServiceAccount struct { // PasswordPolicy configures reva password policy type PasswordPolicy struct { - MinCharacters int `yaml:"min_characters,omitempty" env:"FRONTEND_PASSWORD_POLICY_MIN_CHARACTERS" desc:"Define the minimum password length. Defaults to 0 if not set."` - MinLowerCaseCharacters int `yaml:"min_lowercase_characters" env:"FRONTEND_PASSWORD_POLICY_MIN_LOWERCASE_CHARACTERS" desc:"Define the minimum number of uppercase letters. Defaults to 0 if not set."` - MinUpperCaseCharacters int `yaml:"min_uppercase_characters" env:"FRONTEND_PASSWORD_POLICY_MIN_UPPERCASE_CHARACTERS" desc:"Define the minimum number of lowercase letters. Defaults to 0 if not set."` - MinDigits int `yaml:"min_digits" env:"FRONTEND_PASSWORD_POLICY_MIN_DIGITS" desc:"Define the minimum number of digits. Defaults to 0 if not set."` - MinSpecialCharacters int `yaml:"min_special_characters" env:"FRONTEND_PASSWORD_POLICY_MIN_SPECIAL_CHARACTERS" desc:"Define the minimum number of characters from the special characters list to be present. Defaults to 0 if not set."` - BannedPasswordsList string `yaml:"banned_passwords_list" env:"FRONTEND_PASSWORD_POLICY_BANNED_PASSWORDS_LIST" desc:"Path to the 'banned passwords list' file. See the documentation for more details."` + MinCharacters int `yaml:"min_characters,omitempty" env:"OCIS_PASSWORD_POLICY_MIN_CHARACTERS;FRONTEND_PASSWORD_POLICY_MIN_CHARACTERS" desc:"Define the minimum password length. Defaults to 0 if not set."` + MinLowerCaseCharacters int `yaml:"min_lowercase_characters" env:"OCIS_PASSWORD_POLICY_MIN_LOWERCASE_CHARACTERS;FRONTEND_PASSWORD_POLICY_MIN_LOWERCASE_CHARACTERS" desc:"Define the minimum number of uppercase letters. Defaults to 0 if not set."` + MinUpperCaseCharacters int `yaml:"min_uppercase_characters" env:"OCIS_PASSWORD_POLICY_MIN_UPPERCASE_CHARACTERS;FRONTEND_PASSWORD_POLICY_MIN_UPPERCASE_CHARACTERS" desc:"Define the minimum number of lowercase letters. Defaults to 0 if not set."` + MinDigits int `yaml:"min_digits" env:"OCIS_PASSWORD_POLICY_MIN_DIGITS;FRONTEND_PASSWORD_POLICY_MIN_DIGITS" desc:"Define the minimum number of digits. Defaults to 0 if not set."` + MinSpecialCharacters int `yaml:"min_special_characters" env:"OCIS_PASSWORD_POLICY_MIN_SPECIAL_CHARACTERS;FRONTEND_PASSWORD_POLICY_MIN_SPECIAL_CHARACTERS" desc:"Define the minimum number of characters from the special characters list to be present. Defaults to 0 if not set."` + BannedPasswordsList string `yaml:"banned_passwords_list" env:"OCIS_PASSWORD_POLICY_BANNED_PASSWORDS_LIST;FRONTEND_PASSWORD_POLICY_BANNED_PASSWORDS_LIST" desc:"Path to the 'banned passwords list' file. See the documentation for more details."` } diff --git a/services/sharing/pkg/command/server.go b/services/sharing/pkg/command/server.go index 027c169837..8d18a3adcb 100644 --- a/services/sharing/pkg/command/server.go +++ b/services/sharing/pkg/command/server.go @@ -57,7 +57,10 @@ func Server(cfg *config.Config) *cli.Command { gr.Add(func() error { pidFile := path.Join(os.TempDir(), "revad-"+cfg.Service.Name+"-"+uuid.Must(uuid.NewV4()).String()+".pid") - rCfg := revaconfig.SharingConfigFromStruct(cfg) + rCfg, err := revaconfig.SharingConfigFromStruct(cfg, logger) + if err != nil { + return err + } reg := registry.GetRegistry() runtime.RunWithOptions(rCfg, pidFile, diff --git a/services/sharing/pkg/config/config.go b/services/sharing/pkg/config/config.go index 8dc2ea8c53..f21d98474a 100644 --- a/services/sharing/pkg/config/config.go +++ b/services/sharing/pkg/config/config.go @@ -26,9 +26,12 @@ type Config struct { PublicSharingDriver string `yaml:"public_sharing_driver" env:"SHARING_PUBLIC_DRIVER" desc:"Driver to be used to persist public shares. Supported values are 'jsoncs3', 'json' and 'cs3'."` PublicSharingDrivers PublicSharingDrivers `yaml:"public_sharing_drivers"` WriteableShareMustHavePassword bool `yaml:"public_sharing_writeableshare_must_have_password" env:"OCIS_SHARING_PUBLIC_WRITEABLE_SHARE_MUST_HAVE_PASSWORD;SHARING_PUBLIC_WRITEABLE_SHARE_MUST_HAVE_PASSWORD" desc:"Set this to true if you want to enforce passwords on Uploader, Editor or Contributor shares. If not using the global OCIS_SHARING_PUBLIC_WRITEABLE_SHARE_MUST_HAVE_PASSWORD, you must define the FRONTEND_OCS_PUBLIC_WRITEABLE_SHARE_MUST_HAVE_PASSWORD in the frontend service."` + PublicShareMustHavePassword bool `yaml:"public_sharing_share_must_have_password" env:"OCIS_SHARING_PUBLIC_SHARE_MUST_HAVE_PASSWORD;SHARING_PUBLIC_SHARE_MUST_HAVE_PASSWORD" desc:"Set this to true if you want to enforce passwords on all public shares."` EnableExpiredSharesCleanup bool `yaml:"enable_expired_shares_cleanup"` Supervised bool `yaml:"-"` Context context.Context `yaml:"-"` + + PasswordPolicy PasswordPolicy `yaml:"password_policy"` } type Log struct { @@ -151,3 +154,13 @@ type Events struct { TLSRootCaCertPath string `yaml:"tls_root_ca_cert_path" env:"OCIS_EVENTS_TLS_ROOT_CA_CERTIFICATE;SHARING_EVENTS_TLS_ROOT_CA_CERTIFICATE;SHARING_EVENTS_TLS_ROOT_CA_CERT" desc:"The root CA certificate used to validate the server's TLS certificate. If provided SHARING_EVENTS_TLS_INSECURE will be seen as false." deprecationVersion:"4.0.3" removalVersion:"5.0.0" deprecationInfo:"SHARING_EVENTS_TLS_ROOT_CA_CERT changing name for consistency" deprecationReplacement:"SHARING_EVENTS_TLS_ROOT_CA_CERTIFICATE"` EnableTLS bool `yaml:"enable_tls" env:"OCIS_EVENTS_ENABLE_TLS;SHARING_EVENTS_ENABLE_TLS" desc:"Enable TLS for the connection to the events broker. The events broker is the ocis service which receives and delivers events between the services.."` } + +// PasswordPolicy configures reva password policy +type PasswordPolicy struct { + MinCharacters int `yaml:"min_characters,omitempty" env:"OCIS_PASSWORD_POLICY_MIN_CHARACTERS;SHARING_PASSWORD_POLICY_MIN_CHARACTERS" desc:"Define the minimum password length. Defaults to 0 if not set."` + MinLowerCaseCharacters int `yaml:"min_lowercase_characters" env:"OCIS_PASSWORD_POLICY_MIN_LOWERCASE_CHARACTERS;SHARING_PASSWORD_POLICY_MIN_LOWERCASE_CHARACTERS" desc:"Define the minimum number of uppercase letters. Defaults to 0 if not set."` + MinUpperCaseCharacters int `yaml:"min_uppercase_characters" env:"OCIS_PASSWORD_POLICY_MIN_UPPERCASE_CHARACTERS;SHARING_PASSWORD_POLICY_MIN_UPPERCASE_CHARACTERS" desc:"Define the minimum number of lowercase letters. Defaults to 0 if not set."` + MinDigits int `yaml:"min_digits" env:"OCIS_PASSWORD_POLICY_MIN_DIGITS;SHARING_PASSWORD_POLICY_MIN_DIGITS" desc:"Define the minimum number of digits. Defaults to 0 if not set."` + MinSpecialCharacters int `yaml:"min_special_characters" env:"OCIS_PASSWORD_POLICY_MIN_SPECIAL_CHARACTERS;SHARING_PASSWORD_POLICY_MIN_SPECIAL_CHARACTERS" desc:"Define the minimum number of characters from the special characters list to be present. Defaults to 0 if not set."` + BannedPasswordsList string `yaml:"banned_passwords_list" env:"OCIS_PASSWORD_POLICY_BANNED_PASSWORDS_LIST;SHARING_PASSWORD_POLICY_BANNED_PASSWORDS_LIST" desc:"Path to the 'banned passwords list' file. See the documentation for more details."` +} diff --git a/services/sharing/pkg/config/defaults/defaultconfig.go b/services/sharing/pkg/config/defaults/defaultconfig.go index 1a916b0949..57026e0b9a 100644 --- a/services/sharing/pkg/config/defaults/defaultconfig.go +++ b/services/sharing/pkg/config/defaults/defaultconfig.go @@ -75,7 +75,15 @@ func DefaultConfig() *config.Config { ClusterID: "ocis-cluster", EnableTLS: false, }, - EnableExpiredSharesCleanup: true, + EnableExpiredSharesCleanup: true, + PublicShareMustHavePassword: true, + PasswordPolicy: config.PasswordPolicy{ + MinCharacters: 8, + MinLowerCaseCharacters: 1, + MinUpperCaseCharacters: 1, + MinDigits: 1, + MinSpecialCharacters: 1, + }, } } diff --git a/services/sharing/pkg/revaconfig/config.go b/services/sharing/pkg/revaconfig/config.go index 4f3e236b16..ac3102fa0a 100644 --- a/services/sharing/pkg/revaconfig/config.go +++ b/services/sharing/pkg/revaconfig/config.go @@ -1,11 +1,29 @@ package revaconfig import ( + "bufio" + "fmt" + "os" + "path/filepath" + + "github.com/owncloud/ocis/v2/ocis-pkg/log" + + "github.com/owncloud/ocis/v2/ocis-pkg/config/defaults" "github.com/owncloud/ocis/v2/services/sharing/pkg/config" ) // SharingConfigFromStruct will adapt an oCIS config struct into a reva mapstructure to start a reva service. -func SharingConfigFromStruct(cfg *config.Config) map[string]interface{} { +func SharingConfigFromStruct(cfg *config.Config, logger log.Logger) (map[string]interface{}, error) { + var bannedPasswordsList map[string]struct{} + var err error + if cfg.PasswordPolicy.BannedPasswordsList != "" { + bannedPasswordsList, err = readMultilineFile(cfg.PasswordPolicy.BannedPasswordsList) + if err != nil { + err = fmt.Errorf("failed to load the banned passwords from a file %s: %w", cfg.PasswordPolicy.BannedPasswordsList, err) + logger.Err(err).Send() + return nil, err + } + } rcfg := map[string]interface{}{ "shared": map[string]interface{}{ "jwt_secret": cfg.TokenManager.JWTSecret, @@ -73,6 +91,17 @@ func SharingConfigFromStruct(cfg *config.Config) map[string]interface{} { }, }, "publicshareprovider": map[string]interface{}{ + "gateway_addr": cfg.Reva.Address, + "writeable_share_must_have_password": cfg.WriteableShareMustHavePassword, + "public_share_must_have_password": cfg.PublicShareMustHavePassword, + "password_policy": map[string]interface{}{ + "min_digits": cfg.PasswordPolicy.MinDigits, + "min_characters": cfg.PasswordPolicy.MinCharacters, + "min_lowercase_characters": cfg.PasswordPolicy.MinLowerCaseCharacters, + "min_uppercase_characters": cfg.PasswordPolicy.MinUpperCaseCharacters, + "min_special_characters": cfg.PasswordPolicy.MinSpecialCharacters, + "banned_passwords_list": bannedPasswordsList, + }, "driver": cfg.PublicSharingDriver, "drivers": map[string]interface{}{ "json": map[string]interface{}{ @@ -97,13 +126,12 @@ func SharingConfigFromStruct(cfg *config.Config) map[string]interface{} { "machine_auth_apikey": cfg.PublicSharingDrivers.CS3.SystemUserAPIKey, }, "jsoncs3": map[string]interface{}{ - "gateway_addr": cfg.Reva.Address, - "provider_addr": cfg.PublicSharingDrivers.JSONCS3.ProviderAddr, - "service_user_id": cfg.PublicSharingDrivers.JSONCS3.SystemUserID, - "service_user_idp": cfg.PublicSharingDrivers.JSONCS3.SystemUserIDP, - "machine_auth_apikey": cfg.PublicSharingDrivers.JSONCS3.SystemUserAPIKey, - "writeable_share_must_have_password": cfg.WriteableShareMustHavePassword, - "enable_expired_shares_cleanup": cfg.EnableExpiredSharesCleanup, + "gateway_addr": cfg.Reva.Address, + "provider_addr": cfg.PublicSharingDrivers.JSONCS3.ProviderAddr, + "service_user_id": cfg.PublicSharingDrivers.JSONCS3.SystemUserID, + "service_user_idp": cfg.PublicSharingDrivers.JSONCS3.SystemUserIDP, + "machine_auth_apikey": cfg.PublicSharingDrivers.JSONCS3.SystemUserAPIKey, + "enable_expired_shares_cleanup": cfg.EnableExpiredSharesCleanup, }, }, }, @@ -126,5 +154,33 @@ func SharingConfigFromStruct(cfg *config.Config) map[string]interface{} { }, }, } - return rcfg + return rcfg, nil +} + +func readMultilineFile(path string) (map[string]struct{}, error) { + if !fileExists(path) { + path = filepath.Join(defaults.BaseConfigPath(), path) + } + file, err := os.Open(path) + if err != nil { + return nil, err + } + defer file.Close() + scanner := bufio.NewScanner(file) + data := make(map[string]struct{}) + for scanner.Scan() { + line := scanner.Text() + if line != "" { + data[line] = struct{}{} + } + } + return data, err +} + +func fileExists(path string) bool { + info, err := os.Stat(path) + if err != nil { + return false + } + return !info.IsDir() } From 1f9c6d44b4bd537ce1ed0b216d0ac44f353f82e5 Mon Sep 17 00:00:00 2001 From: Michael Barz Date: Wed, 6 Dec 2023 09:30:15 +0100 Subject: [PATCH 4/7] feat: improve expiration date handling --- services/graph/pkg/service/v0/links.go | 30 ++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/services/graph/pkg/service/v0/links.go b/services/graph/pkg/service/v0/links.go index c42e1debc7..035fb1b650 100644 --- a/services/graph/pkg/service/v0/links.go +++ b/services/graph/pkg/service/v0/links.go @@ -8,14 +8,14 @@ import ( "net/url" "path" "strconv" + "time" rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1" link "github.com/cs3org/go-cs3apis/cs3/sharing/link/v1beta1" providerv1beta1 "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" + types "github.com/cs3org/go-cs3apis/cs3/types/v1beta1" "github.com/go-chi/render" libregraph "github.com/owncloud/libre-graph-api-go" - - "github.com/cs3org/reva/v2/pkg/utils" "github.com/owncloud/ocis/v2/services/graph/pkg/errorcode" "github.com/owncloud/ocis/v2/services/graph/pkg/linktype" ) @@ -94,7 +94,12 @@ func (g Graph) createLink(ctx context.Context, driveItemID *providerv1beta1.Reso } expirationDate, isSet := createLink.GetExpirationDateTimeOk() if isSet { - req.GetGrant().Expiration = utils.TimeToTS(*expirationDate) + expireTime := parseAndFillUpTime(expirationDate) + if expireTime == nil { + g.logger.Debug().Interface("createLink", createLink).Msg(err.Error()) + return nil, errorcode.New(errorcode.InvalidRequest, "invalid expiration date") + } + req.GetGrant().Expiration = expireTime } // set displayname and password protected as arbitrary metadata @@ -139,7 +144,24 @@ func (g Graph) libreGraphPermissionFromCS3PublicShare(createdLink *link.PublicSh // set expiration date if createdLink.GetExpiration() != nil { - perm.SetExpirationDateTime(cs3TimestampToTime(createdLink.GetExpiration())) + perm.SetExpirationDateTime(cs3TimestampToTime(createdLink.GetExpiration()).UTC()) } return perm, nil } + +func parseAndFillUpTime(t *time.Time) *types.Timestamp { + if t == nil { + return nil + } + + // the link needs to be valid for the whole day + tLink := time.Date(t.Year(), t.Month(), t.Day(), 0, 0, 0, 0, t.Location()) + tLink = tLink.Add(23*time.Hour + 59*time.Minute + 59*time.Second) + + final := tLink.UnixNano() + + return &types.Timestamp{ + Seconds: uint64(final / 1000000000), + Nanos: uint32(final % 1000000000), + } +} From 51f21b20f5955e5a9a1a779fb9c53ad53034a3f0 Mon Sep 17 00:00:00 2001 From: Michael Barz Date: Wed, 6 Dec 2023 10:35:25 +0100 Subject: [PATCH 5/7] ci: use global env variables for pw policy --- .drone.star | 13 ++-- .../enforcePasswordPublicLink.feature | 73 +++++++++---------- 2 files changed, 43 insertions(+), 43 deletions(-) diff --git a/.drone.star b/.drone.star index db4bc4b50f..e6ca75f643 100644 --- a/.drone.star +++ b/.drone.star @@ -1065,11 +1065,11 @@ def uiTestPipeline(ctx, filterTags, runPart = 1, numberOfParts = 1, storage = "o extra_server_environment = { "OCIS_SHARING_PUBLIC_SHARE_MUST_HAVE_PASSWORD": False, - "FRONTEND_PASSWORD_POLICY_MIN_CHARACTERS": 1, - "FRONTEND_PASSWORD_POLICY_MIN_LOWERCASE_CHARACTERS": 0, - "FRONTEND_PASSWORD_POLICY_MIN_UPPERCASE_CHARACTERS": 0, - "FRONTEND_PASSWORD_POLICY_MIN_DIGITS": 0, - "FRONTEND_PASSWORD_POLICY_MIN_SPECIAL_CHARACTERS": 0, + "OCIS_PASSWORD_POLICY_MIN_CHARACTERS": 1, + "OCIS_PASSWORD_POLICY_MIN_LOWERCASE_CHARACTERS": 0, + "OCIS_PASSWORD_POLICY_MIN_UPPERCASE_CHARACTERS": 0, + "OCIS_PASSWORD_POLICY_MIN_DIGITS": 0, + "OCIS_PASSWORD_POLICY_MIN_SPECIAL_CHARACTERS": 0, } return { @@ -1137,7 +1137,7 @@ def e2eTests(ctx): } extra_server_environment = { - "FRONTEND_PASSWORD_POLICY_BANNED_PASSWORDS_LIST": "%s" % dirs["bannedPasswordList"], + "OCIS_PASSWORD_POLICY_BANNED_PASSWORDS_LIST": "%s" % dirs["bannedPasswordList"], } e2e_trigger = { @@ -1929,6 +1929,7 @@ def ocisServer(storage, accounts_hash_difficulty = 4, volumes = [], depends_on = if deploy_type == "cs3api_validator": environment["GATEWAY_GRPC_ADDR"] = "0.0.0.0:9142" # make gateway available to cs3api-validator + environment["OCIS_SHARING_PUBLIC_SHARE_MUST_HAVE_PASSWORD"] = False if deploy_type == "wopi_validator": environment["GATEWAY_GRPC_ADDR"] = "0.0.0.0:9142" # make gateway available to wopi server diff --git a/tests/acceptance/features/apiGraph/enforcePasswordPublicLink.feature b/tests/acceptance/features/apiGraph/enforcePasswordPublicLink.feature index 50c3048a19..e223a68f5e 100644 --- a/tests/acceptance/features/apiGraph/enforcePasswordPublicLink.feature +++ b/tests/acceptance/features/apiGraph/enforcePasswordPublicLink.feature @@ -6,11 +6,11 @@ Feature: enforce password on public link Password requirements. set by default: | OCIS_SHARING_PUBLIC_SHARE_MUST_HAVE_PASSWORD | true | - | FRONTEND_PASSWORD_POLICY_MIN_CHARACTERS | 8 | - | FRONTEND_PASSWORD_POLICY_MIN_LOWERCASE_CHARACTERS | 1 | - | FRONTEND_PASSWORD_POLICY_MIN_UPPERCASE_CHARACTERS | 1 | - | FRONTEND_PASSWORD_POLICY_MIN_DIGITS | 1 | - | FRONTEND_PASSWORD_POLICY_MIN_SPECIAL_CHARACTERS | 1 | + | OCIS_PASSWORD_POLICY_MIN_CHARACTERS | 8 | + | OCIS_PASSWORD_POLICY_MIN_LOWERCASE_CHARACTERS | 1 | + | OCIS_PASSWORD_POLICY_MIN_UPPERCASE_CHARACTERS | 1 | + | OCIS_PASSWORD_POLICY_MIN_DIGITS | 1 | + | OCIS_PASSWORD_POLICY_MIN_SPECIAL_CHARACTERS | 1 | Scenario Outline: create a public link with edit permission without a password when enforce-password is enabled @@ -82,11 +82,11 @@ Feature: enforce password on public link Given the following configs have been set: | config | value | | OCIS_SHARING_PUBLIC_WRITEABLE_SHARE_MUST_HAVE_PASSWORD | true | - | FRONTEND_PASSWORD_POLICY_MIN_CHARACTERS | 13 | - | FRONTEND_PASSWORD_POLICY_MIN_LOWERCASE_CHARACTERS | 3 | - | FRONTEND_PASSWORD_POLICY_MIN_UPPERCASE_CHARACTERS | 2 | - | FRONTEND_PASSWORD_POLICY_MIN_DIGITS | 2 | - | FRONTEND_PASSWORD_POLICY_MIN_SPECIAL_CHARACTERS | 2 | + | OCIS_PASSWORD_POLICY_MIN_CHARACTERS | 13 | + | OCIS_PASSWORD_POLICY_MIN_LOWERCASE_CHARACTERS | 3 | + | OCIS_PASSWORD_POLICY_MIN_UPPERCASE_CHARACTERS | 2 | + | OCIS_PASSWORD_POLICY_MIN_DIGITS | 2 | + | OCIS_PASSWORD_POLICY_MIN_SPECIAL_CHARACTERS | 2 | And user "Alice" has been created with default attributes and without skeleton files And user "Alice" has uploaded file with content "test file" to "/testfile.txt" And using OCS API version "" @@ -109,11 +109,11 @@ Feature: enforce password on public link Scenario Outline: try to create a public link with a password that does not comply with the password policy Given the following configs have been set: | config | value | - | FRONTEND_PASSWORD_POLICY_MIN_CHARACTERS | 13 | - | FRONTEND_PASSWORD_POLICY_MIN_LOWERCASE_CHARACTERS | 3 | - | FRONTEND_PASSWORD_POLICY_MIN_UPPERCASE_CHARACTERS | 2 | - | FRONTEND_PASSWORD_POLICY_MIN_DIGITS | 2 | - | FRONTEND_PASSWORD_POLICY_MIN_SPECIAL_CHARACTERS | 2 | + | OCIS_PASSWORD_POLICY_MIN_CHARACTERS | 13 | + | OCIS_PASSWORD_POLICY_MIN_LOWERCASE_CHARACTERS | 3 | + | OCIS_PASSWORD_POLICY_MIN_UPPERCASE_CHARACTERS | 2 | + | OCIS_PASSWORD_POLICY_MIN_DIGITS | 2 | + | OCIS_PASSWORD_POLICY_MIN_SPECIAL_CHARACTERS | 2 | And user "Alice" has been created with default attributes and without skeleton files And user "Alice" has uploaded file with content "test file" to "/testfile.txt" And using OCS API version "" @@ -142,11 +142,11 @@ Feature: enforce password on public link | config | value | | OCIS_SHARING_PUBLIC_SHARE_MUST_HAVE_PASSWORD | false | | OCIS_SHARING_PUBLIC_WRITEABLE_SHARE_MUST_HAVE_PASSWORD | true | - | FRONTEND_PASSWORD_POLICY_MIN_CHARACTERS | 13 | - | FRONTEND_PASSWORD_POLICY_MIN_LOWERCASE_CHARACTERS | 3 | - | FRONTEND_PASSWORD_POLICY_MIN_UPPERCASE_CHARACTERS | 2 | - | FRONTEND_PASSWORD_POLICY_MIN_DIGITS | 1 | - | FRONTEND_PASSWORD_POLICY_MIN_SPECIAL_CHARACTERS | 2 | + | OCIS_PASSWORD_POLICY_MIN_CHARACTERS | 13 | + | OCIS_PASSWORD_POLICY_MIN_LOWERCASE_CHARACTERS | 3 | + | OCIS_PASSWORD_POLICY_MIN_UPPERCASE_CHARACTERS | 2 | + | OCIS_PASSWORD_POLICY_MIN_DIGITS | 1 | + | OCIS_PASSWORD_POLICY_MIN_SPECIAL_CHARACTERS | 2 | And user "Alice" has been created with default attributes and without skeleton files And user "Alice" has uploaded file with content "test file" to "/testfile.txt" And using OCS API version "" @@ -173,11 +173,11 @@ Feature: enforce password on public link | config | value | | OCIS_SHARING_PUBLIC_SHARE_MUST_HAVE_PASSWORD | false | | OCIS_SHARING_PUBLIC_WRITEABLE_SHARE_MUST_HAVE_PASSWORD | true | - | FRONTEND_PASSWORD_POLICY_MIN_CHARACTERS | 13 | - | FRONTEND_PASSWORD_POLICY_MIN_LOWERCASE_CHARACTERS | 3 | - | FRONTEND_PASSWORD_POLICY_MIN_UPPERCASE_CHARACTERS | 2 | - | FRONTEND_PASSWORD_POLICY_MIN_DIGITS | 1 | - | FRONTEND_PASSWORD_POLICY_MIN_SPECIAL_CHARACTERS | 2 | + | OCIS_PASSWORD_POLICY_MIN_CHARACTERS | 13 | + | OCIS_PASSWORD_POLICY_MIN_LOWERCASE_CHARACTERS | 3 | + | OCIS_PASSWORD_POLICY_MIN_UPPERCASE_CHARACTERS | 2 | + | OCIS_PASSWORD_POLICY_MIN_DIGITS | 1 | + | OCIS_PASSWORD_POLICY_MIN_SPECIAL_CHARACTERS | 2 | And user "Alice" has been created with default attributes and without skeleton files And user "Alice" has uploaded file with content "test file" to "/testfile.txt" And using OCS API version "" @@ -220,14 +220,14 @@ Feature: enforce password on public link But the public should be able to download file "/textfile.txt" from inside the last public link shared folder using the new public WebDAV API with password "" Examples: | config | config-value | password | - | FRONTEND_PASSWORD_POLICY_MIN_CHARACTERS | 4 | Ps-1 | - | FRONTEND_PASSWORD_POLICY_MIN_CHARACTERS | 14 | Ps1:with space | - | FRONTEND_PASSWORD_POLICY_MIN_LOWERCASE_CHARACTERS | 4 | PS1:test | - | FRONTEND_PASSWORD_POLICY_MIN_UPPERCASE_CHARACTERS | 3 | PS1:TeƒsT | - | FRONTEND_PASSWORD_POLICY_MIN_DIGITS | 2 | PS1:test2 | - | FRONTEND_PASSWORD_POLICY_MIN_SPECIAL_CHARACTERS | 2 | PS1:test pass | - | FRONTEND_PASSWORD_POLICY_MIN_SPECIAL_CHARACTERS | 33 | pS1! #$%&'()*+,-./:;<=>?@[\]^_`{ }~ | - | FRONTEND_PASSWORD_POLICY_MIN_SPECIAL_CHARACTERS | 5 | 1sameCharacterShouldWork!!!!! | + | OCIS_PASSWORD_POLICY_MIN_CHARACTERS | 4 | Ps-1 | + | OCIS_PASSWORD_POLICY_MIN_CHARACTERS | 14 | Ps1:with space | + | OCIS_PASSWORD_POLICY_MIN_LOWERCASE_CHARACTERS | 4 | PS1:test | + | OCIS_PASSWORD_POLICY_MIN_UPPERCASE_CHARACTERS | 3 | PS1:TeƒsT | + | OCIS_PASSWORD_POLICY_MIN_DIGITS | 2 | PS1:test2 | + | OCIS_PASSWORD_POLICY_MIN_SPECIAL_CHARACTERS | 2 | PS1:test pass | + | OCIS_PASSWORD_POLICY_MIN_SPECIAL_CHARACTERS | 33 | pS1! #$%&'()*+,-./:;<=>?@[\]^_`{ }~ | + | OCIS_PASSWORD_POLICY_MIN_SPECIAL_CHARACTERS | 5 | 1sameCharacterShouldWork!!!!! | Scenario Outline: try to create a public link with a password that does not comply with the password policy (invalid cases) @@ -250,7 +250,7 @@ Feature: enforce password on public link Scenario Outline: update a public link with a password that is listed in the Banned-Password-List - Given the config "FRONTEND_PASSWORD_POLICY_BANNED_PASSWORDS_LIST" has been set to path "config/drone/banned-password-list.txt" + Given the config "OCIS_PASSWORD_POLICY_BANNED_PASSWORDS_LIST" has been set to path "config/drone/banned-password-list.txt" And using OCS API version "2" And user "Alice" has been created with default attributes and without skeleton files And user "Alice" has uploaded file with content "test file" to "/testfile.txt" @@ -268,10 +268,10 @@ Feature: enforce password on public link | 123 | 400 | 400 | Unfortunately, your password is commonly used. please pick a harder-to-guess password for your safety | | password | 400 | 400 | Unfortunately, your password is commonly used. please pick a harder-to-guess password for your safety | | ownCloud | 400 | 400 | Unfortunately, your password is commonly used. please pick a harder-to-guess password for your safety | - + Scenario Outline: create a public link with a password that is listed in the Banned-Password-List - Given the config "FRONTEND_PASSWORD_POLICY_BANNED_PASSWORDS_LIST" has been set to path "config/drone/banned-password-list.txt" + Given the config "OCIS_PASSWORD_POLICY_BANNED_PASSWORDS_LIST" has been set to path "config/drone/banned-password-list.txt" And using OCS API version "2" And user "Alice" has been created with default attributes and without skeleton files And user "Alice" has uploaded file with content "test file" to "/testfile.txt" @@ -287,4 +287,3 @@ Feature: enforce password on public link | 123 | 400 | 400 | Unfortunately, your password is commonly used. please pick a harder-to-guess password for your safety | | password | 400 | 400 | Unfortunately, your password is commonly used. please pick a harder-to-guess password for your safety | | ownCloud | 400 | 400 | Unfortunately, your password is commonly used. please pick a harder-to-guess password for your safety | - \ No newline at end of file From 956a0427cf97d39314f3c587aba3c0441eba2ce3 Mon Sep 17 00:00:00 2001 From: Michael Barz Date: Wed, 6 Dec 2023 09:44:02 +0100 Subject: [PATCH 6/7] chore: update reva --- go.mod | 2 +- go.sum | 6 +- .../services/gateway/usershareprovider.go | 6 +- .../publicshareprovider.go | 241 ++++++++++++++++-- .../usershareprovider/usershareprovider.go | 4 +- .../v2/pkg/publicshare/manager/json/json.go | 47 ++-- .../reva/v2/pkg/publicshare/publicshare.go | 6 - vendor/modules.txt | 2 +- 8 files changed, 251 insertions(+), 63 deletions(-) diff --git a/go.mod b/go.mod index a144487aa5..2eefbc5e79 100644 --- a/go.mod +++ b/go.mod @@ -13,7 +13,7 @@ require ( github.com/coreos/go-oidc v2.2.1+incompatible github.com/coreos/go-oidc/v3 v3.8.0 github.com/cs3org/go-cs3apis v0.0.0-20231023073225-7748710e0781 - github.com/cs3org/reva/v2 v2.16.1-0.20231201122033-a389ddc645c4 + github.com/cs3org/reva/v2 v2.16.1-0.20231206110211-7198abf507f6 github.com/dhowden/tag v0.0.0-20230630033851-978a0926ee25 github.com/disintegration/imaging v1.6.2 github.com/dutchcoders/go-clamd v0.0.0-20170520113014-b970184f4d9e diff --git a/go.sum b/go.sum index 1cc2921a21..e1b5d013c1 100644 --- a/go.sum +++ b/go.sum @@ -1017,8 +1017,8 @@ github.com/crewjam/saml v0.4.14 h1:g9FBNx62osKusnFzs3QTN5L9CVA/Egfgm+stJShzw/c= github.com/crewjam/saml v0.4.14/go.mod h1:UVSZCf18jJkk6GpWNVqcyQJMD5HsRugBPf4I1nl2mME= github.com/cs3org/go-cs3apis v0.0.0-20231023073225-7748710e0781 h1:BUdwkIlf8IS2FasrrPg8gGPHQPOrQ18MS1Oew2tmGtY= github.com/cs3org/go-cs3apis v0.0.0-20231023073225-7748710e0781/go.mod h1:UXha4TguuB52H14EMoSsCqDj7k8a/t7g4gVP+bgY5LY= -github.com/cs3org/reva/v2 v2.16.1-0.20231201122033-a389ddc645c4 h1:61AwMfov2OxrUElWXXKHZfBsuxgNIVwZVQW4PlJoqnM= -github.com/cs3org/reva/v2 v2.16.1-0.20231201122033-a389ddc645c4/go.mod h1:zcrrYVsBv/DwhpyO2/W5hoSZ/k6az6Z2EYQok65uqZY= +github.com/cs3org/reva/v2 v2.16.1-0.20231206110211-7198abf507f6 h1:wR1XlTT8ilWd5Yd943yayvRPyz1GBaAt7vZ9SOxHzqI= +github.com/cs3org/reva/v2 v2.16.1-0.20231206110211-7198abf507f6/go.mod h1:zcrrYVsBv/DwhpyO2/W5hoSZ/k6az6Z2EYQok65uqZY= github.com/cyberdelia/templates v0.0.0-20141128023046-ca7fffd4298c/go.mod h1:GyV+0YP4qX0UQ7r2MoYZ+AvYDp12OF5yg4q8rGnyNh4= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= @@ -1682,6 +1682,8 @@ github.com/maxymania/go-system v0.0.0-20170110133659-647cc364bf0b h1:Q53idHrTuQD github.com/maxymania/go-system v0.0.0-20170110133659-647cc364bf0b/go.mod h1:KirJrATYGbTyUwVR26xIkaipRqRcMRXBf8N5dacvGus= github.com/mendsley/gojwk v0.0.0-20141217222730-4d5ec6e58103 h1:Z/i1e+gTZrmcGeZyWckaLfucYG6KYOXLWo4co8pZYNY= github.com/mendsley/gojwk v0.0.0-20141217222730-4d5ec6e58103/go.mod h1:o9YPB5aGP8ob35Vy6+vyq3P3bWe7NQWzf+JLiXCiMaE= +github.com/micbar/reva/v2 v2.0.0-20231206102158-0fd5eba44b02 h1:J28bUNh/2K4OUFJdWoCt6zGQF2QALu4K/r6H3bTYodQ= +github.com/micbar/reva/v2 v2.0.0-20231206102158-0fd5eba44b02/go.mod h1:zcrrYVsBv/DwhpyO2/W5hoSZ/k6az6Z2EYQok65uqZY= github.com/miekg/dns v1.0.14/go.mod h1:W1PPwlIAgtquWBMBEV9nkV9Cazfe8ScdGz/Lj7v3Nrg= github.com/miekg/dns v1.1.26/go.mod h1:bPDLeHnStXmXAq1m/Ch/hvfNHr14JKNPMBo3VZKjuso= github.com/miekg/dns v1.1.40/go.mod h1:KNUDUusw/aVsxyTYZM1oqvCicbwhgbNgztCETuNZ7xM= diff --git a/vendor/github.com/cs3org/reva/v2/internal/grpc/services/gateway/usershareprovider.go b/vendor/github.com/cs3org/reva/v2/internal/grpc/services/gateway/usershareprovider.go index bbcfbf0bc4..fd5a57f9e9 100644 --- a/vendor/github.com/cs3org/reva/v2/internal/grpc/services/gateway/usershareprovider.go +++ b/vendor/github.com/cs3org/reva/v2/internal/grpc/services/gateway/usershareprovider.go @@ -123,9 +123,9 @@ func (s *svc) updateShare(ctx context.Context, req *collaboration.UpdateShareReq if s.c.CommitShareToStorageGrant { creator := ctxpkg.ContextMustGetUser(ctx) grant := &provider.Grant{ - Grantee: req.GetShare().GetGrantee(), - Permissions: req.GetShare().GetPermissions().GetPermissions(), - Expiration: req.GetShare().GetExpiration(), + Grantee: res.GetShare().GetGrantee(), + Permissions: res.GetShare().GetPermissions().GetPermissions(), + Expiration: res.GetShare().GetExpiration(), Creator: creator.GetId(), } updateGrantStatus, err := s.updateGrant(ctx, res.GetShare().GetResourceId(), grant, nil) diff --git a/vendor/github.com/cs3org/reva/v2/internal/grpc/services/publicshareprovider/publicshareprovider.go b/vendor/github.com/cs3org/reva/v2/internal/grpc/services/publicshareprovider/publicshareprovider.go index 33b7bcd3dc..8687a4da45 100644 --- a/vendor/github.com/cs3org/reva/v2/internal/grpc/services/publicshareprovider/publicshareprovider.go +++ b/vendor/github.com/cs3org/reva/v2/internal/grpc/services/publicshareprovider/publicshareprovider.go @@ -20,10 +20,21 @@ package publicshareprovider import ( "context" + "fmt" "regexp" + "strconv" + "time" + gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1" link "github.com/cs3org/go-cs3apis/cs3/sharing/link/v1beta1" + provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" + "github.com/cs3org/reva/v2/pkg/password" + "github.com/cs3org/reva/v2/pkg/permission" + "github.com/cs3org/reva/v2/pkg/rgrpc/todo/pool" + "github.com/cs3org/reva/v2/pkg/sharedconf" + "github.com/cs3org/reva/v2/pkg/storage/utils/grants" + "github.com/cs3org/reva/v2/pkg/utils" "github.com/mitchellh/mapstructure" "github.com/pkg/errors" "google.golang.org/grpc" @@ -38,6 +49,8 @@ import ( "github.com/cs3org/reva/v2/pkg/rgrpc/status" ) +const getUserCtxErrMsg = "error getting user from context" + func init() { rgrpc.Register("publicshareprovider", New) } @@ -45,9 +58,21 @@ func init() { type config struct { Driver string `mapstructure:"driver"` Drivers map[string]map[string]interface{} `mapstructure:"drivers"` + GatewayAddr string `mapstructure:"gateway_addr"` AllowedPathsForShares []string `mapstructure:"allowed_paths_for_shares"` EnableExpiredSharesCleanup bool `mapstructure:"enable_expired_shares_cleanup"` WriteableShareMustHavePassword bool `mapstructure:"writeable_share_must_have_password"` + PublicShareMustHavePassword bool `mapstructure:"public_share_must_have_password"` + PasswordPolicy map[string]interface{} `mapstructure:"password_policy"` +} + +type passwordPolicy struct { + MinCharacters int `mapstructure:"min_characters"` + MinLowerCaseCharacters int `mapstructure:"min_lowercase_characters"` + MinUpperCaseCharacters int `mapstructure:"min_uppercase_characters"` + MinDigits int `mapstructure:"min_digits"` + MinSpecialCharacters int `mapstructure:"min_special_characters"` + BannedPasswordsList map[string]struct{} `mapstructure:"banned_passwords_list"` } func (c *config) init() { @@ -59,7 +84,9 @@ func (c *config) init() { type service struct { conf *config sm publicshare.Manager + gatewaySelector pool.Selectable[gateway.GatewayAPIClient] allowedPathsForShares []*regexp.Regexp + passwordValidator password.Validator } func getShareManager(c *config) (publicshare.Manager, error) { @@ -84,12 +111,21 @@ func (s *service) Register(ss *grpc.Server) { func parseConfig(m map[string]interface{}) (*config, error) { c := &config{} if err := mapstructure.Decode(m, c); err != nil { - err = errors.Wrap(err, "error decoding conf") + err = errors.Wrap(err, "error decoding config") return nil, err } return c, nil } +func parsePasswordPolicy(m map[string]interface{}) (*passwordPolicy, error) { + p := &passwordPolicy{} + if err := mapstructure.Decode(m, p); err != nil { + err = errors.Wrap(err, "error decoding password policy config") + return nil, err + } + return p, nil +} + // New creates a new user share provider svc func New(m map[string]interface{}, ss *grpc.Server) (rgrpc.Service, error) { @@ -97,6 +133,10 @@ func New(m map[string]interface{}, ss *grpc.Server) (rgrpc.Service, error) { if err != nil { return nil, err } + p, err := parsePasswordPolicy(c.PasswordPolicy) + if err != nil { + return nil, err + } c.init() @@ -114,15 +154,36 @@ func New(m map[string]interface{}, ss *grpc.Server) (rgrpc.Service, error) { allowedPathsForShares = append(allowedPathsForShares, regex) } + gatewaySelector, err := pool.GatewaySelector(sharedconf.GetGatewaySVC(c.GatewayAddr)) + if err != nil { + return nil, err + } + service := &service{ conf: c, sm: sm, + gatewaySelector: gatewaySelector, allowedPathsForShares: allowedPathsForShares, + passwordValidator: newPasswordPolicy(p), } return service, nil } +func newPasswordPolicy(c *passwordPolicy) password.Validator { + if c == nil { + return password.NewPasswordPolicy(0, 0, 0, 0, 0, nil) + } + return password.NewPasswordPolicy( + c.MinCharacters, + c.MinLowerCaseCharacters, + c.MinUpperCaseCharacters, + c.MinDigits, + c.MinSpecialCharacters, + c.BannedPasswordsList, + ) +} + func (s *service) isPathAllowed(path string) bool { if len(s.allowedPathsForShares) == 0 { return true @@ -139,33 +200,129 @@ func (s *service) CreatePublicShare(ctx context.Context, req *link.CreatePublicS log := appctx.GetLogger(ctx) log.Info().Str("publicshareprovider", "create").Msg("create public share") - if !conversions.SufficientCS3Permissions(req.GetResourceInfo().GetPermissionSet(), req.GetGrant().GetPermissions().GetPermissions()) { + gatewayClient, err := s.gatewaySelector.Next() + if err != nil { + return nil, err + } + + isInternalLink := grants.PermissionsEqual(req.GetGrant().GetPermissions().GetPermissions(), &provider.ResourcePermissions{}) + + sRes, err := gatewayClient.Stat(ctx, &provider.StatRequest{Ref: &provider.Reference{ResourceId: req.GetResourceInfo().GetId()}}) + if err != nil { + log.Err(err).Interface("resource_id", req.GetResourceInfo().GetId()).Msg("failed to stat resource to share") return &link.CreatePublicShareResponse{ - Status: status.NewInvalid(ctx, "insufficient permissions to create that kind of share"), + Status: status.NewInternal(ctx, "failed to stat resource to share"), + }, err + } + + // all users can create internal links + if !isInternalLink { + // check if the user has the permission in the user role + ok, err := utils.CheckPermission(ctx, permission.WritePublicLink, gatewayClient) + if err != nil { + return &link.CreatePublicShareResponse{ + Status: status.NewInternal(ctx, "failed check user permission to write public link"), + }, err + } + if !ok { + return &link.CreatePublicShareResponse{ + Status: status.NewPermissionDenied(ctx, nil, "no permission to create public links"), + }, nil + } + } + + // check that user has share permissions + if !sRes.GetInfo().GetPermissionSet().AddGrant { + return &link.CreatePublicShareResponse{ + Status: status.NewInvalidArg(ctx, "no share permission"), }, nil } - if !s.isPathAllowed(req.ResourceInfo.Path) { + // check if the user can share with the desired permissions + if !conversions.SufficientCS3Permissions(sRes.GetInfo().GetPermissionSet(), req.GetGrant().GetPermissions().GetPermissions()) { return &link.CreatePublicShareResponse{ - Status: status.NewInvalid(ctx, "share creation is not allowed for the specified path"), + Status: status.NewInvalidArg(ctx, "insufficient permissions to create that kind of share"), }, nil } + // validate path + if !s.isPathAllowed(req.GetResourceInfo().GetPath()) { + return &link.CreatePublicShareResponse{ + Status: status.NewFailedPrecondition(ctx, nil, "share creation is not allowed for the specified path"), + }, nil + } + + // check that this is a not a personal space root + if req.GetResourceInfo().GetId().GetOpaqueId() == req.GetResourceInfo().GetId().GetSpaceId() && + req.GetResourceInfo().GetSpace().GetSpaceType() == "personal" { + return &link.CreatePublicShareResponse{ + Status: status.NewInvalidArg(ctx, "cannot create link on personal space root"), + }, nil + } + + // quick link returns the existing one if already present + quickLink, err := checkQuicklink(req.GetResourceInfo()) + if err != nil { + return &link.CreatePublicShareResponse{ + Status: status.NewInvalidArg(ctx, "invalid quicklink value"), + }, nil + } + if quickLink { + f := []*link.ListPublicSharesRequest_Filter{publicshare.ResourceIDFilter(req.GetResourceInfo().GetId())} + req := link.ListPublicSharesRequest{Filters: f} + res, err := s.ListPublicShares(ctx, &req) + if err != nil || res.GetStatus().GetCode() != rpc.Code_CODE_OK { + return &link.CreatePublicShareResponse{ + Status: status.NewInternal(ctx, "could not list public links"), + }, nil + } + for _, l := range res.GetShare() { + if l.Quicklink { + return &link.CreatePublicShareResponse{ + Status: status.NewOK(ctx), + Share: l, + }, nil + } + } + } + grant := req.GetGrant() - if grant != nil && s.conf.WriteableShareMustHavePassword && - publicshare.IsWriteable(grant.GetPermissions()) && grant.Password == "" { + + // validate expiration date + if grant.GetExpiration() != nil { + expirationDateTime := utils.TSToTime(grant.GetExpiration()).UTC() + if expirationDateTime.Before(time.Now().UTC()) { + msg := fmt.Sprintf("expiration date is in the past: %s", expirationDateTime.Format(time.RFC3339)) + return &link.CreatePublicShareResponse{ + Status: status.NewInvalidArg(ctx, msg), + }, nil + } + } + + // enforce password if needed + setPassword := grant.GetPassword() + if !isInternalLink && enforcePassword(grant, s.conf) && len(setPassword) == 0 { return &link.CreatePublicShareResponse{ - Status: status.NewInvalid(ctx, "writeable shares must have a password protection"), + Status: status.NewInvalidArg(ctx, "password protection is enforced"), }, nil } + // validate password policy + if len(setPassword) > 0 { + if err := s.passwordValidator.Validate(setPassword); err != nil { + return &link.CreatePublicShareResponse{ + Status: status.NewInvalidArg(ctx, err.Error()), + }, nil + } + } + u, ok := ctxpkg.ContextGetUser(ctx) if !ok { - log.Error().Msg("error getting user from context") + log.Error().Msg(getUserCtxErrMsg) } res := &link.CreatePublicShareResponse{} - share, err := s.sm.CreatePublicShare(ctx, u, req.ResourceInfo, req.Grant) + share, err := s.sm.CreatePublicShare(ctx, u, req.GetResourceInfo(), req.GetGrant()) switch { case err != nil: log.Error().Err(err).Interface("request", req).Msg("could not write public share") @@ -179,11 +336,37 @@ func (s *service) CreatePublicShare(ctx context.Context, req *link.CreatePublicS } func (s *service) RemovePublicShare(ctx context.Context, req *link.RemovePublicShareRequest) (*link.RemovePublicShareResponse, error) { + gatewayClient, err := s.gatewaySelector.Next() + if err != nil { + return nil, err + } + log := appctx.GetLogger(ctx) log.Info().Str("publicshareprovider", "remove").Msg("remove public share") user := ctxpkg.ContextMustGetUser(ctx) - err := s.sm.RevokePublicShare(ctx, user, req.Ref) + ps, err := s.sm.GetPublicShare(ctx, user, req.GetRef(), false) + if err != nil { + return &link.RemovePublicShareResponse{ + Status: status.NewInternal(ctx, "error loading public share"), + }, err + } + if !publicshare.IsCreatedByUser(*ps, user) { + sRes, err := gatewayClient.Stat(ctx, &provider.StatRequest{Ref: &provider.Reference{ResourceId: ps.ResourceId}}) + if err != nil { + log.Err(err).Interface("resource_id", ps.ResourceId).Msg("failed to stat shared resource") + return &link.RemovePublicShareResponse{ + Status: status.NewInternal(ctx, "failed to stat shared resource"), + }, err + } + + if !sRes.GetInfo().GetPermissionSet().RemoveGrant { + return &link.RemovePublicShareResponse{ + Status: status.NewPermissionDenied(ctx, nil, "no permission to delete public share"), + }, err + } + } + err = s.sm.RevokePublicShare(ctx, user, req.Ref) if err != nil { return &link.RemovePublicShareResponse{ Status: status.NewInternal(ctx, "error deleting public share"), @@ -227,7 +410,7 @@ func (s *service) GetPublicShare(ctx context.Context, req *link.GetPublicShareRe u, ok := ctxpkg.ContextGetUser(ctx) if !ok { - log.Error().Msg("error getting user from context") + log.Error().Msg(getUserCtxErrMsg) } ps, err := s.sm.GetPublicShare(ctx, u, req.Ref, req.GetSign()) @@ -281,16 +464,11 @@ func (s *service) UpdatePublicShare(ctx context.Context, req *link.UpdatePublicS u, ok := ctxpkg.ContextGetUser(ctx) if !ok { - log.Error().Msg("error getting user from context") + log.Error().Msg(getUserCtxErrMsg) } updateR, err := s.sm.UpdatePublicShare(ctx, u, req) if err != nil { - if errors.Is(err, publicshare.ErrShareNeedsPassword) { - return &link.UpdatePublicShareResponse{ - Status: status.NewInvalid(ctx, err.Error()), - }, nil - } return &link.UpdatePublicShareResponse{ Status: status.NewInternal(ctx, err.Error()), }, nil @@ -302,3 +480,30 @@ func (s *service) UpdatePublicShare(ctx context.Context, req *link.UpdatePublicS } return res, nil } + +func enforcePassword(grant *link.Grant, conf *config) bool { + if conf.PublicShareMustHavePassword { + return true + } + isReadOnly := conversions.SufficientCS3Permissions(conversions.NewViewerRole(true).CS3ResourcePermissions(), grant.GetPermissions().GetPermissions()) + return !isReadOnly && conf.WriteableShareMustHavePassword +} + +func checkQuicklink(info *provider.ResourceInfo) (bool, error) { + if info == nil { + return false, nil + } + if m := info.GetArbitraryMetadata().GetMetadata(); m != nil { + q, ok := m["quicklink"] + // empty string would trigger an error in ParseBool() + if !ok || q == "" { + return false, nil + } + quickLink, err := strconv.ParseBool(q) + if err != nil { + return false, err + } + return quickLink, nil + } + return false, nil +} diff --git a/vendor/github.com/cs3org/reva/v2/internal/grpc/services/usershareprovider/usershareprovider.go b/vendor/github.com/cs3org/reva/v2/internal/grpc/services/usershareprovider/usershareprovider.go index 313928c39e..bc4ccd46d0 100644 --- a/vendor/github.com/cs3org/reva/v2/internal/grpc/services/usershareprovider/usershareprovider.go +++ b/vendor/github.com/cs3org/reva/v2/internal/grpc/services/usershareprovider/usershareprovider.go @@ -156,13 +156,13 @@ func (s *service) CreateShare(ctx context.Context, req *collaboration.CreateShar req.GetGrant().GetPermissions().GetPermissions(), ); !shareCreationAllowed { return &collaboration.CreateShareResponse{ - Status: status.NewInvalid(ctx, "insufficient permissions to create that kind of share"), + Status: status.NewPermissionDenied(ctx, nil, "insufficient permissions to create that kind of share"), }, nil } if !s.isPathAllowed(req.GetResourceInfo().GetPath()) { return &collaboration.CreateShareResponse{ - Status: status.NewInvalid(ctx, "share creation is not allowed for the specified path"), + Status: status.NewFailedPrecondition(ctx, nil, "share creation is not allowed for the specified path"), }, nil } diff --git a/vendor/github.com/cs3org/reva/v2/pkg/publicshare/manager/json/json.go b/vendor/github.com/cs3org/reva/v2/pkg/publicshare/manager/json/json.go index 5d7673b824..960a2eea6e 100644 --- a/vendor/github.com/cs3org/reva/v2/pkg/publicshare/manager/json/json.go +++ b/vendor/github.com/cs3org/reva/v2/pkg/publicshare/manager/json/json.go @@ -76,7 +76,7 @@ func NewFile(c map[string]interface{}) (publicshare.Manager, error) { return nil, err } - return New(conf.GatewayAddr, conf.SharePasswordHashCost, conf.JanitorRunInterval, conf.EnableExpiredSharesCleanup, p, conf.WriteableShareMustHavePassword) + return New(conf.GatewayAddr, conf.SharePasswordHashCost, conf.JanitorRunInterval, conf.EnableExpiredSharesCleanup, p) } // NewMemory returns a new in-memory public shares manager. @@ -93,7 +93,7 @@ func NewMemory(c map[string]interface{}) (publicshare.Manager, error) { return nil, err } - return New(conf.GatewayAddr, conf.SharePasswordHashCost, conf.JanitorRunInterval, conf.EnableExpiredSharesCleanup, p, conf.WriteableShareMustHavePassword) + return New(conf.GatewayAddr, conf.SharePasswordHashCost, conf.JanitorRunInterval, conf.EnableExpiredSharesCleanup, p) } // NewCS3 returns a new cs3 public shares manager. @@ -115,19 +115,18 @@ func NewCS3(c map[string]interface{}) (publicshare.Manager, error) { return nil, err } - return New(conf.GatewayAddr, conf.SharePasswordHashCost, conf.JanitorRunInterval, conf.EnableExpiredSharesCleanup, p, conf.WriteableShareMustHavePassword) + return New(conf.GatewayAddr, conf.SharePasswordHashCost, conf.JanitorRunInterval, conf.EnableExpiredSharesCleanup, p) } // New returns a new public share manager instance -func New(gwAddr string, pwHashCost, janitorRunInterval int, enableCleanup bool, p persistence.Persistence, writeableShareMustHavePassword bool) (publicshare.Manager, error) { +func New(gwAddr string, pwHashCost, janitorRunInterval int, enableCleanup bool, p persistence.Persistence) (publicshare.Manager, error) { m := &manager{ - gatewayAddr: gwAddr, - mutex: &sync.Mutex{}, - passwordHashCost: pwHashCost, - janitorRunInterval: janitorRunInterval, - enableExpiredSharesCleanup: enableCleanup, - persistence: p, - writeableShareMustHavePassword: writeableShareMustHavePassword, + gatewayAddr: gwAddr, + mutex: &sync.Mutex{}, + passwordHashCost: pwHashCost, + janitorRunInterval: janitorRunInterval, + enableExpiredSharesCleanup: enableCleanup, + persistence: p, } go m.startJanitorRun() @@ -135,11 +134,10 @@ func New(gwAddr string, pwHashCost, janitorRunInterval int, enableCleanup bool, } type commonConfig struct { - GatewayAddr string `mapstructure:"gateway_addr"` - SharePasswordHashCost int `mapstructure:"password_hash_cost"` - JanitorRunInterval int `mapstructure:"janitor_run_interval"` - EnableExpiredSharesCleanup bool `mapstructure:"enable_expired_shares_cleanup"` - WriteableShareMustHavePassword bool `mapstructure:"writeable_share_must_have_password"` + GatewayAddr string `mapstructure:"gateway_addr"` + SharePasswordHashCost int `mapstructure:"password_hash_cost"` + JanitorRunInterval int `mapstructure:"janitor_run_interval"` + EnableExpiredSharesCleanup bool `mapstructure:"enable_expired_shares_cleanup"` } type fileConfig struct { @@ -171,10 +169,9 @@ type manager struct { mutex *sync.Mutex persistence persistence.Persistence - passwordHashCost int - janitorRunInterval int - enableExpiredSharesCleanup bool - writeableShareMustHavePassword bool + passwordHashCost int + janitorRunInterval int + enableExpiredSharesCleanup bool } func (m *manager) startJanitorRun() { @@ -343,12 +340,6 @@ func (m *manager) UpdatePublicShare(ctx context.Context, u *user.User, req *link old, _ := json.Marshal(share.Permissions) new, _ := json.Marshal(req.Update.GetGrant().Permissions) - if m.writeableShareMustHavePassword && - publicshare.IsWriteable(req.GetUpdate().GetGrant().GetPermissions()) && - (!share.PasswordProtected && req.GetUpdate().GetGrant().GetPassword() == "") { - return nil, publicshare.ErrShareNeedsPassword - } - if req.GetUpdate().GetGrant().GetPassword() != "" { passwordChanged = true h, err := bcrypt.GenerateFromPassword([]byte(req.Update.GetGrant().Password), m.passwordHashCost) @@ -369,10 +360,6 @@ func (m *manager) UpdatePublicShare(ctx context.Context, u *user.User, req *link case link.UpdatePublicShareRequest_Update_TYPE_PASSWORD: passwordChanged = true if req.Update.GetGrant().Password == "" { - if m.writeableShareMustHavePassword && publicshare.IsWriteable(share.Permissions) { - return nil, publicshare.ErrShareNeedsPassword - } - share.PasswordProtected = false newPasswordEncoded = "" } else { diff --git a/vendor/github.com/cs3org/reva/v2/pkg/publicshare/publicshare.go b/vendor/github.com/cs3org/reva/v2/pkg/publicshare/publicshare.go index 187c3165c5..4eca5cdccb 100644 --- a/vendor/github.com/cs3org/reva/v2/pkg/publicshare/publicshare.go +++ b/vendor/github.com/cs3org/reva/v2/pkg/publicshare/publicshare.go @@ -24,7 +24,6 @@ import ( "crypto/sha256" "crypto/sha512" "encoding/hex" - "errors" "time" user "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" @@ -41,11 +40,6 @@ const ( StorageIDFilterType link.ListPublicSharesRequest_Filter_Type = 4 ) -var ( - // ErrShareNeedsPassword is an error which is returned when a public share must have a password. - ErrShareNeedsPassword = errors.New("the public share needs to have a password") -) - // Manager manipulates public shares. type Manager interface { CreatePublicShare(ctx context.Context, u *user.User, md *provider.ResourceInfo, g *link.Grant) (*link.PublicShare, error) diff --git a/vendor/modules.txt b/vendor/modules.txt index ec3026fb60..4185eac179 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -359,7 +359,7 @@ github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1 github.com/cs3org/go-cs3apis/cs3/storage/registry/v1beta1 github.com/cs3org/go-cs3apis/cs3/tx/v1beta1 github.com/cs3org/go-cs3apis/cs3/types/v1beta1 -# github.com/cs3org/reva/v2 v2.16.1-0.20231201122033-a389ddc645c4 +# github.com/cs3org/reva/v2 v2.16.1-0.20231206110211-7198abf507f6 ## explicit; go 1.20 github.com/cs3org/reva/v2/cmd/revad/internal/grace github.com/cs3org/reva/v2/cmd/revad/runtime From 3b56a57798679b668beebdceb1e15e9a3ea84338 Mon Sep 17 00:00:00 2001 From: Michael Barz Date: Wed, 6 Dec 2023 12:22:37 +0100 Subject: [PATCH 7/7] docs: add changelog --- .../unreleased/add-validation-to-public-share-provider.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changelog/unreleased/add-validation-to-public-share-provider.md diff --git a/changelog/unreleased/add-validation-to-public-share-provider.md b/changelog/unreleased/add-validation-to-public-share-provider.md new file mode 100644 index 0000000000..e0a28e997f --- /dev/null +++ b/changelog/unreleased/add-validation-to-public-share-provider.md @@ -0,0 +1,6 @@ +Enhancement: Add validation to public share provider + +We changed the implementation of the public share provider in reva to do the validation on the CS3 Api side. This makes the implementation on the graph side smaller. + +https://github.com/owncloud/ocis/pull/7877 +https://github.com/owncloud/ocis/issues/6993