From bd12ce33f1fd597cc5771a93c1b56b6b7f5790ce Mon Sep 17 00:00:00 2001 From: David Christofas Date: Tue, 29 Mar 2022 15:54:30 +0200 Subject: [PATCH 1/3] clean up the json public share manager --- pkg/publicshare/manager/cs3/cs3.go | 4 +-- pkg/publicshare/manager/json/json.go | 47 ++++++++++++++-------------- pkg/publicshare/publicshare.go | 16 +++++++--- 3 files changed, 38 insertions(+), 29 deletions(-) diff --git a/pkg/publicshare/manager/cs3/cs3.go b/pkg/publicshare/manager/cs3/cs3.go index 28551a46b2..3eb916ee63 100644 --- a/pkg/publicshare/manager/cs3/cs3.go +++ b/pkg/publicshare/manager/cs3/cs3.go @@ -314,7 +314,7 @@ func (m *Manager) ListPublicShares(ctx context.Context, u *user.User, filters [] return nil, err } - if publicshare.MatchesFilters(ps.PublicShare, filters) && !publicshare.IsExpired(ps.PublicShare) { + if publicshare.MatchesFilters(*ps.PublicShare, filters) && !publicshare.IsExpired(*ps.PublicShare) { result = append(result, ps.PublicShare) } @@ -361,7 +361,7 @@ func (m *Manager) GetPublicShareByToken(ctx context.Context, token string, auth return nil, err } - if publicshare.IsExpired(ps.PublicShare) { + if publicshare.IsExpired(*ps.PublicShare) { return nil, errtypes.NotFound("public share has expired") } diff --git a/pkg/publicshare/manager/json/json.go b/pkg/publicshare/manager/json/json.go index aa7ad191a0..4bf8d99500 100644 --- a/pkg/publicshare/manager/json/json.go +++ b/pkg/publicshare/manager/json/json.go @@ -165,8 +165,8 @@ func (m *manager) CreatePublicShare(ctx context.Context, u *user.User, rInfo *pr } createdAt := &typespb.Timestamp{ - Seconds: uint64(now / 1000000000), - Nanos: uint32(now % 1000000000), + Seconds: uint64(now / int64(time.Second)), + Nanos: uint32(now % int64(time.Second)), } s := link.PublicShare{ @@ -262,8 +262,8 @@ func (m *manager) UpdatePublicShare(ctx context.Context, u *user.User, req *link } share.Mtime = &typespb.Timestamp{ - Seconds: uint64(now / 1000000000), - Nanos: uint32(now % 1000000000), + Seconds: uint64(now / int64(time.Second)), + Nanos: uint32(now % int64(time.Second)), } m.mutex.Lock() @@ -333,7 +333,7 @@ func (m *manager) GetPublicShare(ctx context.Context, u *user.User, ref *link.Pu } if ref.GetId().GetOpaqueId() == ps.Id.OpaqueId { - if publicshare.IsExpired(&ps) { + if publicshare.IsExpired(ps) { if err := m.revokeExpiredPublicShare(ctx, &ps, u); err != nil { return nil, err } @@ -354,45 +354,46 @@ func (m *manager) GetPublicShare(ctx context.Context, u *user.User, ref *link.Pu // ListPublicShares retrieves all the shares on the manager that are valid. func (m *manager) ListPublicShares(ctx context.Context, u *user.User, filters []*link.ListPublicSharesRequest_Filter, sign bool) ([]*link.PublicShare, error) { - var shares []*link.PublicShare - m.mutex.Lock() defer m.mutex.Unlock() + log := appctx.GetLogger(ctx) + db, err := m.readDb() if err != nil { return nil, err } + var shares []*link.PublicShare for _, v := range db { var local publicShare if err := utils.UnmarshalJSONToProtoV1([]byte(v.(map[string]interface{})["share"].(string)), &local.PublicShare); err != nil { return nil, err } - // skip if the share isn't created by the current user. - if local.Creator.GetOpaqueId() != u.Id.OpaqueId || (local.Creator.GetIdp() != "" && u.Id.Idp != local.Creator.GetIdp()) { - continue - } - - if local.PublicShare.PasswordProtected && sign { - if err := publicshare.AddSignature(&local.PublicShare, local.Password); err != nil { - return nil, err + if publicshare.IsExpired(local.PublicShare) { + if err := m.revokeExpiredPublicShare(ctx, &local.PublicShare, u); err != nil { + log.Error().Err(err). + Str("share_token", local.Token). + Msg("failed to revoke expired public share") } + continue } - if len(filters) == 0 { - shares = append(shares, &local.PublicShare) + // skip if the share isn't created by the current user or if the share + // doesn't match the given filters. + if !(publicshare.IsCreatedByUser(local.PublicShare, u) && + publicshare.MatchesFilters(local.PublicShare, filters)) { continue } - if publicshare.MatchesFilters(&local.PublicShare, filters) { - if !publicshare.IsExpired(&local.PublicShare) { - shares = append(shares, &local.PublicShare) - } else if err := m.revokeExpiredPublicShare(ctx, &local.PublicShare, u); err != nil { + if local.PublicShare.PasswordProtected && sign { + if err := publicshare.AddSignature(&local.PublicShare, local.Password); err != nil { return nil, err } } + + shares = append(shares, &local.PublicShare) } return shares, nil @@ -410,7 +411,7 @@ func (m *manager) cleanupExpiredShares() { var ps link.PublicShare _ = utils.UnmarshalJSONToProtoV1([]byte(d.(string)), &ps) - if publicshare.IsExpired(&ps) { + if publicshare.IsExpired(ps) { _ = m.revokeExpiredPublicShare(context.Background(), &ps, nil) } } @@ -512,7 +513,7 @@ func (m *manager) GetPublicShareByToken(ctx context.Context, token string, auth } if local.Token == token { - if publicshare.IsExpired(&local) { + if publicshare.IsExpired(local) { // TODO user is not needed at all in this API. if err := m.revokeExpiredPublicShare(ctx, &local, nil); err != nil { return nil, err diff --git a/pkg/publicshare/publicshare.go b/pkg/publicshare/publicshare.go index d85507048a..35c5d05a46 100644 --- a/pkg/publicshare/publicshare.go +++ b/pkg/publicshare/publicshare.go @@ -115,7 +115,7 @@ func StorageIDFilter(id string) *link.ListPublicSharesRequest_Filter { } // MatchesFilter tests if the share passes the filter. -func MatchesFilter(share *link.PublicShare, filter *link.ListPublicSharesRequest_Filter) bool { +func MatchesFilter(share link.PublicShare, filter *link.ListPublicSharesRequest_Filter) bool { switch filter.Type { case link.ListPublicSharesRequest_Filter_TYPE_RESOURCE_ID: return utils.ResourceIDEqual(share.ResourceId, filter.GetResourceId()) @@ -127,7 +127,7 @@ func MatchesFilter(share *link.PublicShare, filter *link.ListPublicSharesRequest } // MatchesAnyFilter checks if the share passes at least one of the given filters. -func MatchesAnyFilter(share *link.PublicShare, filters []*link.ListPublicSharesRequest_Filter) bool { +func MatchesAnyFilter(share link.PublicShare, filters []*link.ListPublicSharesRequest_Filter) bool { for _, f := range filters { if MatchesFilter(share, f) { return true @@ -140,7 +140,10 @@ func MatchesAnyFilter(share *link.PublicShare, filters []*link.ListPublicSharesR // Filters of the same type form a disjuntion, a logical OR. Filters of separate type form a conjunction, a logical AND. // Here is an example: // (resource_id=1 OR resource_id=2) AND (grantee_type=USER OR grantee_type=GROUP) -func MatchesFilters(share *link.PublicShare, filters []*link.ListPublicSharesRequest_Filter) bool { +func MatchesFilters(share link.PublicShare, filters []*link.ListPublicSharesRequest_Filter) bool { + if len(filters) == 0 { + return true + } grouped := GroupFiltersByType(filters) for _, f := range grouped { if !MatchesAnyFilter(share, f) { @@ -160,7 +163,7 @@ func GroupFiltersByType(filters []*link.ListPublicSharesRequest_Filter) map[link } // IsExpired tests whether a public share is expired -func IsExpired(s *link.PublicShare) bool { +func IsExpired(s link.PublicShare) bool { expiration := time.Unix(int64(s.Expiration.GetSeconds()), int64(s.Expiration.GetNanos())) return s.Expiration != nil && expiration.Before(time.Now()) } @@ -187,3 +190,8 @@ func Authenticate(share *link.PublicShare, pw string, auth *link.PublicShareAuth } return false } + +// IsCreatedByUser checks if a share was created by the user. +func IsCreatedByUser(share link.PublicShare, user *user.User) bool { + return utils.UserEqual(user.Id, share.Owner) || utils.UserEqual(user.Id, share.Creator) +} From c67187b714ec0e5ddccb1b6841249bbb45cae008 Mon Sep 17 00:00:00 2001 From: David Christofas Date: Tue, 29 Mar 2022 17:11:36 +0200 Subject: [PATCH 2/3] enable all space members to see public shares --- pkg/publicshare/manager/json/json.go | 43 ++++++++++++++++++++++------ 1 file changed, 35 insertions(+), 8 deletions(-) diff --git a/pkg/publicshare/manager/json/json.go b/pkg/publicshare/manager/json/json.go index 4bf8d99500..7d72cb00b4 100644 --- a/pkg/publicshare/manager/json/json.go +++ b/pkg/publicshare/manager/json/json.go @@ -26,6 +26,7 @@ import ( "os" "os/signal" "path/filepath" + "strings" "sync" "syscall" "time" @@ -34,6 +35,7 @@ import ( "golang.org/x/crypto/bcrypt" user "github.com/cs3org/go-cs3apis/cs3/identity/user/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" typespb "github.com/cs3org/go-cs3apis/cs3/types/v1beta1" @@ -41,6 +43,7 @@ import ( "github.com/cs3org/reva/v2/pkg/errtypes" "github.com/cs3org/reva/v2/pkg/publicshare" "github.com/cs3org/reva/v2/pkg/publicshare/manager/registry" + "github.com/cs3org/reva/v2/pkg/rgrpc/todo/pool" "github.com/cs3org/reva/v2/pkg/utils" "github.com/mitchellh/mapstructure" "github.com/pkg/errors" @@ -60,6 +63,7 @@ func New(c map[string]interface{}) (publicshare.Manager, error) { conf.init() m := manager{ + gatewayAddr: conf.GatewayAddr, mutex: &sync.Mutex{}, file: conf.File, passwordHashCost: conf.SharePasswordHashCost, @@ -93,6 +97,7 @@ func New(c map[string]interface{}) (publicshare.Manager, error) { } type config struct { + GatewayAddr string `mapstructure:"gateway_addr"` File string `mapstructure:"file"` SharePasswordHashCost int `mapstructure:"password_hash_cost"` JanitorRunInterval int `mapstructure:"janitor_run_interval"` @@ -112,8 +117,9 @@ func (c *config) init() { } type manager struct { - mutex *sync.Mutex - file string + gatewayAddr string + mutex *sync.Mutex + file string passwordHashCost int janitorRunInterval int @@ -364,7 +370,13 @@ func (m *manager) ListPublicShares(ctx context.Context, u *user.User, filters [] return nil, err } - var shares []*link.PublicShare + client, err := pool.GetGatewayServiceClient(m.gatewayAddr) + if err != nil { + return nil, errors.Wrap(err, "failed to list shares") + } + cache := make(map[string]struct{}) + + shares := []*link.PublicShare{} for _, v := range db { var local publicShare if err := utils.UnmarshalJSONToProtoV1([]byte(v.(map[string]interface{})["share"].(string)), &local.PublicShare); err != nil { @@ -380,13 +392,29 @@ func (m *manager) ListPublicShares(ctx context.Context, u *user.User, filters [] continue } - // skip if the share isn't created by the current user or if the share - // doesn't match the given filters. - if !(publicshare.IsCreatedByUser(local.PublicShare, u) && - publicshare.MatchesFilters(local.PublicShare, filters)) { + if !publicshare.MatchesFilters(local.PublicShare, filters) { continue } + key := strings.Join([]string{local.ResourceId.StorageId, local.ResourceId.OpaqueId}, "!") + if _, hit := cache[key]; !hit && !publicshare.IsCreatedByUser(local.PublicShare, u) { + sRes, err := client.Stat(ctx, &provider.StatRequest{Ref: &provider.Reference{ResourceId: local.ResourceId}}) + if err != nil || sRes.Status.Code != rpc.Code_CODE_OK { + log.Error(). + Err(err). + Interface("status", sRes.Status). + Interface("resource_id", local.ResourceId). + Msg("ListShares: could not stat resource") + continue + } + if !sRes.Info.PermissionSet.ListGrants { + // skip because the user doesn't have the permissions to list + // shares of this file. + continue + } + cache[key] = struct{}{} + } + if local.PublicShare.PasswordProtected && sign { if err := publicshare.AddSignature(&local.PublicShare, local.Password); err != nil { return nil, err @@ -395,7 +423,6 @@ func (m *manager) ListPublicShares(ctx context.Context, u *user.User, filters [] shares = append(shares, &local.PublicShare) } - return shares, nil } From 6bb0ad0925732f5d6229cc1bdbcf7278904bbeef Mon Sep 17 00:00:00 2001 From: David Christofas Date: Wed, 30 Mar 2022 13:33:08 +0200 Subject: [PATCH 3/3] enable all space members to see public shares with the cs3 manager --- changelog/unreleased/spaces-public-shares.md | 6 + pkg/publicshare/manager/cs3/cs3.go | 148 +++++++++++++++++-- pkg/publicshare/manager/cs3/cs3_test.go | 5 +- 3 files changed, 147 insertions(+), 12 deletions(-) create mode 100644 changelog/unreleased/spaces-public-shares.md diff --git a/changelog/unreleased/spaces-public-shares.md b/changelog/unreleased/spaces-public-shares.md new file mode 100644 index 0000000000..c9bafc65fe --- /dev/null +++ b/changelog/unreleased/spaces-public-shares.md @@ -0,0 +1,6 @@ +Enhancement: Enable all spaces members to list public shares + +Enhanced the json and cs3 public share manager so that it lists shares in spaces for all members. + +https://github.com/owncloud/ocis/issues/3370 +https://github.com/cs3org/reva/pull/2697 diff --git a/pkg/publicshare/manager/cs3/cs3.go b/pkg/publicshare/manager/cs3/cs3.go index 3eb916ee63..bddbac9528 100644 --- a/pkg/publicshare/manager/cs3/cs3.go +++ b/pkg/publicshare/manager/cs3/cs3.go @@ -24,6 +24,7 @@ import ( "fmt" "net/url" "path" + "strings" "sync" "time" @@ -31,14 +32,17 @@ import ( "github.com/pkg/errors" "golang.org/x/crypto/bcrypt" + gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" user "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" - userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/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" typespb "github.com/cs3org/go-cs3apis/cs3/types/v1beta1" + "github.com/cs3org/reva/v2/pkg/appctx" "github.com/cs3org/reva/v2/pkg/errtypes" "github.com/cs3org/reva/v2/pkg/publicshare" "github.com/cs3org/reva/v2/pkg/publicshare/manager/registry" + "github.com/cs3org/reva/v2/pkg/rgrpc/todo/pool" "github.com/cs3org/reva/v2/pkg/storage/utils/indexer" "github.com/cs3org/reva/v2/pkg/storage/utils/indexer/option" "github.com/cs3org/reva/v2/pkg/storage/utils/metadata" @@ -51,6 +55,7 @@ func init() { // Manager implements a publicshare manager using a cs3 storage backend type Manager struct { + gatewayClient gateway.GatewayAPIClient sync.RWMutex storage metadata.Storage @@ -88,12 +93,18 @@ func NewDefault(m map[string]interface{}) (publicshare.Manager, error) { } indexer := indexer.CreateIndexer(s) - return New(s, indexer, bcrypt.DefaultCost) + client, err := pool.GetGatewayServiceClient(c.GatewayAddr) + if err != nil { + return nil, err + } + + return New(client, s, indexer, bcrypt.DefaultCost) } // New returns a new manager instance -func New(storage metadata.Storage, indexer indexer.Indexer, passwordHashCost int) (publicshare.Manager, error) { +func New(gatewayClient gateway.GatewayAPIClient, storage metadata.Storage, indexer indexer.Indexer, passwordHashCost int) (publicshare.Manager, error) { return &Manager{ + gatewayClient: gatewayClient, storage: storage, indexer: indexer, passwordHashCost: passwordHashCost, @@ -131,6 +142,20 @@ func (m *Manager) initialize() error { if err != nil { return err } + err = m.indexer.AddIndex(&link.PublicShare{}, option.IndexByFunc{ + Name: "Creator", + Func: indexCreatorFunc, + }, "Token", "publicshares", "non_unique", nil, true) + if err != nil { + return err + } + err = m.indexer.AddIndex(&link.PublicShare{}, option.IndexByFunc{ + Name: "ResourceId", + Func: indexResourceIDFunc, + }, "Token", "publicshares", "non_unique", nil, true) + if err != nil { + return err + } m.initialized = true return nil } @@ -168,8 +193,8 @@ func (m *Manager) CreatePublicShare(ctx context.Context, u *user.User, ri *provi } createdAt := &typespb.Timestamp{ - Seconds: uint64(now / 1000000000), - Nanos: uint32(now % 1000000000), + Seconds: uint64(now / int64(time.Second)), + Nanos: uint32(now % int64(time.Second)), } s := &PublicShareWithPassword{ @@ -300,22 +325,43 @@ func (m *Manager) ListPublicShares(ctx context.Context, u *user.User, filters [] return nil, err } - tokens, err := m.indexer.FindBy(&link.PublicShare{}, + log := appctx.GetLogger(ctx) + + createdShareTokens, err := m.indexer.FindBy(&link.PublicShare{}, indexer.NewField("Owner", userIDToIndex(u.Id)), + indexer.NewField("Creator", userIDToIndex(u.Id)), ) if err != nil { return nil, err } + // We use shareMem as a temporary lookup store to check which shares were + // already added. This is to prevent duplicates. + shareMem := make(map[string]struct{}) result := []*link.PublicShare{} - for _, token := range tokens { + for _, token := range createdShareTokens { ps, err := m.getByToken(ctx, token) if err != nil { return nil, err } - if publicshare.MatchesFilters(*ps.PublicShare, filters) && !publicshare.IsExpired(*ps.PublicShare) { - result = append(result, ps.PublicShare) + if !publicshare.MatchesFilters(*ps.PublicShare, filters) { + continue + } + + if publicshare.IsExpired(*ps.PublicShare) { + ref := &link.PublicShareReference{ + Spec: &link.PublicShareReference_Id{ + Id: ps.PublicShare.Id, + }, + } + if err := m.RevokePublicShare(ctx, u, ref); err != nil { + log.Error().Err(err). + Str("public_share_token", ps.PublicShare.Token). + Str("public_share_id", ps.PublicShare.Id.OpaqueId). + Msg("failed to revoke expired public share") + } + continue } if ps.PublicShare.PasswordProtected && sign { @@ -324,8 +370,70 @@ func (m *Manager) ListPublicShares(ctx context.Context, u *user.User, filters [] return nil, err } } + result = append(result, ps.PublicShare) + shareMem[ps.PublicShare.Token] = struct{}{} + } + + // If a user requests to list shares which have not been created by them + // we have to explicitly fetch these shares and check if the user is + // allowed to list the shares. + // Only then can we add these shares to the result. + grouped := publicshare.GroupFiltersByType(filters) + idFilter, ok := grouped[link.ListPublicSharesRequest_Filter_TYPE_RESOURCE_ID] + if !ok { + return result, nil + } + + tokensByResourceID := make(map[string]*provider.ResourceId) + for _, filter := range idFilter { + resourceID := filter.GetResourceId() + tokens, err := m.indexer.FindBy(&link.PublicShare{}, + indexer.NewField("ResourceId", resourceIDToIndex(resourceID)), + ) + if err != nil { + continue + } + for _, token := range tokens { + tokensByResourceID[token] = resourceID + } } + // statMem is used as a local cache to prevent statting resources which + // already have been checked. + statMem := make(map[string]struct{}) + for token, resourceID := range tokensByResourceID { + if _, handled := shareMem[token]; handled { + // We don't want to add a share multiple times when we added it + // already. + continue + } + + if _, checked := statMem[resourceIDToIndex(resourceID)]; !checked { + sReq := &provider.StatRequest{ + Ref: &provider.Reference{ResourceId: resourceID}, + } + sRes, err := m.gatewayClient.Stat(ctx, sReq) + if err != nil { + continue + } + if sRes.Status.Code != rpc.Code_CODE_OK { + continue + } + if !sRes.Info.PermissionSet.ListGrants { + continue + } + statMem[resourceIDToIndex(resourceID)] = struct{}{} + } + + s, err := m.getByToken(ctx, token) + if err != nil { + return nil, err + } + if publicshare.MatchesFilters(*s.PublicShare, filters) { + result = append(result, s.PublicShare) + shareMem[s.PublicShare.Token] = struct{}{} + } + } return result, nil } @@ -382,10 +490,30 @@ func indexOwnerFunc(v interface{}) (string, error) { return userIDToIndex(ps.Owner), nil } -func userIDToIndex(id *userpb.UserId) string { +func indexCreatorFunc(v interface{}) (string, error) { + ps, ok := v.(*link.PublicShare) + if !ok { + return "", fmt.Errorf("given entity is not a public share") + } + return userIDToIndex(ps.Creator), nil +} + +func indexResourceIDFunc(v interface{}) (string, error) { + ps, ok := v.(*link.PublicShare) + if !ok { + return "", fmt.Errorf("given entity is not a public share") + } + return resourceIDToIndex(ps.ResourceId), nil +} + +func userIDToIndex(id *user.UserId) string { return url.QueryEscape(id.Idp + ":" + id.OpaqueId) } +func resourceIDToIndex(id *provider.ResourceId) string { + return strings.Join([]string{id.StorageId, id.OpaqueId}, "!") +} + func (m *Manager) persist(ctx context.Context, ps *PublicShareWithPassword) error { data, err := json.Marshal(ps) if err != nil { diff --git a/pkg/publicshare/manager/cs3/cs3_test.go b/pkg/publicshare/manager/cs3/cs3_test.go index 244b5578bb..dee708dd99 100644 --- a/pkg/publicshare/manager/cs3/cs3_test.go +++ b/pkg/publicshare/manager/cs3/cs3_test.go @@ -110,7 +110,7 @@ var _ = Describe("Cs3", func() { JustBeforeEach(func() { var err error - m, err = cs3.New(storage, indexer, bcrypt.DefaultCost) + m, err = cs3.New(nil, storage, indexer, bcrypt.DefaultCost) Expect(err).ToNot(HaveOccurred()) }) @@ -122,7 +122,7 @@ var _ = Describe("Cs3", func() { Describe("New", func() { It("returns a new instance", func() { - m, err := cs3.New(storage, indexer, bcrypt.DefaultCost) + m, err := cs3.New(nil, storage, indexer, bcrypt.DefaultCost) Expect(err).ToNot(HaveOccurred()) Expect(m).ToNot(BeNil()) }) @@ -226,6 +226,7 @@ var _ = Describe("Cs3", func() { grant.Expiration = &typespb.Timestamp{ Seconds: uint64(t.Unix()), } + storage.On("Delete", mock.Anything, mock.Anything).Return(nil, nil) }) It("does not consider the share", func() {