Skip to content

Commit

Permalink
Disallow sharing the shares directory (#1051)
Browse files Browse the repository at this point in the history
  • Loading branch information
@ishank011
ishank011 authored Aug 5, 2020
1 parent ea29c4f commit ceea2c5
Show file tree
Hide file tree
Showing 6 changed files with 75 additions and 30 deletions.
7 changes: 7 additions & 0 deletions changelog/unreleased/shares-fix.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
Bugfix: Disallow sharing the shares directory

Previously, it was possible to create public links for and share the shares
directory itself. However, when the recipient tried to accept the share, it
failed. This PR prevents the creation of such shares in the first place.

https://github.com/cs3org/reva/pull/1051
4 changes: 4 additions & 0 deletions internal/grpc/services/gateway/publicshareprovider.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ import (
)

func (s *svc) CreatePublicShare(ctx context.Context, req *link.CreatePublicShareRequest) (*link.CreatePublicShareResponse, error) {
if s.isSharedFolder(ctx, req.ResourceInfo.GetPath()) {
return nil, errors.New("gateway: can't create a public share of the share folder itself")
}

log := appctx.GetLogger(ctx)
log.Info().Msg("create public share")

Expand Down
20 changes: 19 additions & 1 deletion internal/grpc/services/gateway/storageprovider.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -941,8 +941,26 @@ func (s *svc) Stat(ctx context.Context, req *provider.StatRequest) (*provider.St
Path: target,
},
}

req.Ref = ref
return s.stat(ctx, req)
res, err := s.stat(ctx, req)
if err != nil {
return &provider.StatResponse{
Status: status.NewInternal(ctx, err, "gateway: error stating"),
}, nil
}
if res.Status.Code != rpc.Code_CODE_OK {
err := status.NewErrorFromCode(res.Status.Code, "gateway")
log.Err(err).Msg("gateway: error stating")
return &provider.StatResponse{
Status: status.NewInternal(ctx, err, "gateway: error stating"),
}, nil
}

// we need to make sure we don't expose the reference target in the resource
// information.
res.Info.Path = p
return res, nil
}

panic("gateway: stating an unknown path:" + p)
Expand Down
5 changes: 5 additions & 0 deletions internal/grpc/services/gateway/usershareprovider.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,11 @@ import (

// TODO(labkode): add multi-phase commit logic when commit share or commit ref is enabled.
func (s *svc) CreateShare(ctx context.Context, req *collaboration.CreateShareRequest) (*collaboration.CreateShareResponse, error) {

if s.isSharedFolder(ctx, req.ResourceInfo.GetPath()) {
return nil, errors.New("gateway: can't share the share folder itself")
}

c, err := pool.GetUserShareProviderClient(s.c.UserShareProviderEndpoint)
if err != nil {
return &collaboration.CreateShareResponse{
Expand Down
34 changes: 20 additions & 14 deletions pkg/share/manager/json/json.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,16 +163,17 @@ func (m *mgr) Share(ctx context.Context, md *provider.ResourceInfo, g *collabora
Nanos: uint32(now % 1000000000),
}

// do not allow share to myself if share is for a user
// do not allow share to myself or the owner if share is for a user
// TODO(labkode): should not this be caught already at the gw level?
if g.Grantee.Type == provider.GranteeType_GRANTEE_TYPE_USER &&
g.Grantee.Id.Idp == user.Id.Idp && g.Grantee.Id.OpaqueId == user.Id.OpaqueId {
return nil, errors.New("json: user and grantee are the same")
((g.Grantee.Id.Idp == user.Id.Idp && g.Grantee.Id.OpaqueId == user.Id.OpaqueId) ||
(g.Grantee.Id.Idp == md.Owner.Idp && g.Grantee.Id.OpaqueId == md.Owner.OpaqueId)) {
return nil, errors.New("json: owner/creator and grantee are the same")
}

// check if share already exists.
key := &collaboration.ShareKey{
Owner: user.Id,
Owner: md.Owner,
ResourceId: md.Id,
Grantee: g.Grantee,
}
Expand All @@ -190,7 +191,7 @@ func (m *mgr) Share(ctx context.Context, md *provider.ResourceInfo, g *collabora
ResourceId: md.Id,
Permissions: g.Permissions,
Grantee: g.Grantee,
Owner: user.Id,
Owner: md.Owner,
Creator: user.Id,
Ctime: ts,
Mtime: ts,
Expand Down Expand Up @@ -223,7 +224,8 @@ func (m *mgr) getByKey(ctx context.Context, key *collaboration.ShareKey) (*colla
m.Lock()
defer m.Unlock()
for _, s := range m.model.Shares {
if key.Owner.Idp == s.Owner.Idp && key.Owner.OpaqueId == s.Owner.OpaqueId &&
if ((key.Owner.Idp == s.Owner.Idp && key.Owner.OpaqueId == s.Owner.OpaqueId) ||
(key.Owner.Idp == s.Creator.Idp && key.Owner.OpaqueId == s.Creator.OpaqueId)) &&
key.ResourceId.StorageId == s.ResourceId.StorageId && key.ResourceId.OpaqueId == s.ResourceId.OpaqueId &&
key.Grantee.Type == s.Grantee.Type && key.Grantee.Id.Idp == s.Grantee.Id.Idp && key.Grantee.Id.OpaqueId == s.Grantee.Id.OpaqueId {
return s, nil
Expand All @@ -247,9 +249,9 @@ func (m *mgr) get(ctx context.Context, ref *collaboration.ShareReference) (s *co
}

// check if we are the owner
// TODO(labkode): check for creator also.
user := user.ContextMustGetUser(ctx)
if user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId {
if (user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId) ||
(user.Id.Idp == s.Creator.Idp && user.Id.OpaqueId == s.Creator.OpaqueId) {
return s, nil
}

Expand All @@ -272,7 +274,8 @@ func (m *mgr) Unshare(ctx context.Context, ref *collaboration.ShareReference) er
user := user.ContextMustGetUser(ctx)
for i, s := range m.model.Shares {
if equal(ref, s) {
if user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId {
if (user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId) ||
(user.Id.Idp == s.Creator.Idp && user.Id.OpaqueId == s.Creator.OpaqueId) {
m.model.Shares[len(m.model.Shares)-1], m.model.Shares[i] = m.model.Shares[i], m.model.Shares[len(m.model.Shares)-1]
m.model.Shares = m.model.Shares[:len(m.model.Shares)-1]
if err := m.model.Save(); err != nil {
Expand All @@ -293,7 +296,8 @@ func equal(ref *collaboration.ShareReference, s *collaboration.Share) bool {
return true
}
} else if ref.GetKey() != nil {
if reflect.DeepEqual(*ref.GetKey().Owner, *s.Owner) && reflect.DeepEqual(*ref.GetKey().ResourceId, *s.ResourceId) && reflect.DeepEqual(*ref.GetKey().Grantee, *s.Grantee) {
if (reflect.DeepEqual(*ref.GetKey().Owner, *s.Owner) || reflect.DeepEqual(*ref.GetKey().Owner, *s.Creator)) &&
reflect.DeepEqual(*ref.GetKey().ResourceId, *s.ResourceId) && reflect.DeepEqual(*ref.GetKey().Grantee, *s.Grantee) {
return true
}
}
Expand All @@ -306,7 +310,8 @@ func (m *mgr) UpdateShare(ctx context.Context, ref *collaboration.ShareReference
user := user.ContextMustGetUser(ctx)
for i, s := range m.model.Shares {
if equal(ref, s) {
if user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId {
if (user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId) ||
(user.Id.Idp == s.Creator.Idp && user.Id.OpaqueId == s.Creator.OpaqueId) {
now := time.Now().UnixNano()
m.model.Shares[i].Permissions = p
m.model.Shares[i].Mtime = &typespb.Timestamp{
Expand All @@ -331,7 +336,8 @@ func (m *mgr) ListShares(ctx context.Context, filters []*collaboration.ListShare
user := user.ContextMustGetUser(ctx)
for _, s := range m.model.Shares {
// TODO(labkode): add check for creator.
if user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId {
if (user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId) ||
(user.Id.Idp == s.Creator.Idp && user.Id.OpaqueId == s.Creator.OpaqueId) {
// no filter we return earlier
if len(filters) == 0 {
ss = append(ss, s)
Expand All @@ -358,9 +364,9 @@ func (m *mgr) ListReceivedShares(ctx context.Context) ([]*collaboration.Received
defer m.Unlock()
user := user.ContextMustGetUser(ctx)
for _, s := range m.model.Shares {
if user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId {
if (user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId) ||
(user.Id.Idp == s.Creator.Idp && user.Id.OpaqueId == s.Creator.OpaqueId) {
// omit shares created by me
// TODO(labkode): apply check for s.Creator also.
continue
}
if s.Grantee.Type == provider.GranteeType_GRANTEE_TYPE_USER {
Expand Down
35 changes: 20 additions & 15 deletions pkg/share/manager/memory/memory.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,15 +75,16 @@ func (m *manager) Share(ctx context.Context, md *provider.ResourceInfo, g *colla
Nanos: uint32(now % 1000000000),
}

// do not allow share to myself if share is for a user
// do not allow share to myself or the owner if share is for a user
if g.Grantee.Type == provider.GranteeType_GRANTEE_TYPE_USER &&
g.Grantee.Id.Idp == user.Id.Idp && g.Grantee.Id.OpaqueId == user.Id.OpaqueId {
return nil, errors.New("memory: user and grantee are the same")
((g.Grantee.Id.Idp == user.Id.Idp && g.Grantee.Id.OpaqueId == user.Id.OpaqueId) ||
(g.Grantee.Id.Idp == md.Owner.Idp && g.Grantee.Id.OpaqueId == md.Owner.OpaqueId)) {
return nil, errors.New("json: owner/creator and grantee are the same")
}

// check if share already exists.
key := &collaboration.ShareKey{
Owner: user.Id,
Owner: md.Owner,
ResourceId: md.Id,
Grantee: g.Grantee,
}
Expand All @@ -100,7 +101,7 @@ func (m *manager) Share(ctx context.Context, md *provider.ResourceInfo, g *colla
ResourceId: md.Id,
Permissions: g.Permissions,
Grantee: g.Grantee,
Owner: user.Id,
Owner: md.Owner,
Creator: user.Id,
Ctime: ts,
Mtime: ts,
Expand All @@ -125,7 +126,8 @@ func (m *manager) getByKey(ctx context.Context, key *collaboration.ShareKey) (*c
m.lock.Lock()
defer m.lock.Unlock()
for _, s := range m.shares {
if key.Owner.Idp == s.Owner.Idp && key.Owner.OpaqueId == s.Owner.OpaqueId &&
if ((key.Owner.Idp == s.Owner.Idp && key.Owner.OpaqueId == s.Owner.OpaqueId) ||
(key.Owner.Idp == s.Creator.Idp && key.Owner.OpaqueId == s.Creator.OpaqueId)) &&
key.ResourceId.StorageId == s.ResourceId.StorageId && key.ResourceId.OpaqueId == s.ResourceId.OpaqueId &&
key.Grantee.Type == s.Grantee.Type && key.Grantee.Id.Idp == s.Grantee.Id.Idp && key.Grantee.Id.OpaqueId == s.Grantee.Id.OpaqueId {
return s, nil
Expand All @@ -149,9 +151,9 @@ func (m *manager) get(ctx context.Context, ref *collaboration.ShareReference) (s
}

// check if we are the owner
// TODO(labkode): check for creator also.
user := user.ContextMustGetUser(ctx)
if user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId {
if (user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId) ||
(user.Id.Idp == s.Creator.Idp && user.Id.OpaqueId == s.Creator.OpaqueId) {
return s, nil
}

Expand All @@ -174,7 +176,8 @@ func (m *manager) Unshare(ctx context.Context, ref *collaboration.ShareReference
user := user.ContextMustGetUser(ctx)
for i, s := range m.shares {
if equal(ref, s) {
if user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId {
if (user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId) ||
(user.Id.Idp == s.Creator.Idp && user.Id.OpaqueId == s.Creator.OpaqueId) {
m.shares[len(m.shares)-1], m.shares[i] = m.shares[i], m.shares[len(m.shares)-1]
m.shares = m.shares[:len(m.shares)-1]
return nil
Expand All @@ -191,7 +194,8 @@ func equal(ref *collaboration.ShareReference, s *collaboration.Share) bool {
return true
}
} else if ref.GetKey() != nil {
if reflect.DeepEqual(*ref.GetKey().Owner, *s.Owner) && reflect.DeepEqual(*ref.GetKey().ResourceId, *s.ResourceId) && reflect.DeepEqual(*ref.GetKey().Grantee, *s.Grantee) {
if (reflect.DeepEqual(*ref.GetKey().Owner, *s.Owner) || reflect.DeepEqual(*ref.GetKey().Owner, *s.Creator)) &&
reflect.DeepEqual(*ref.GetKey().ResourceId, *s.ResourceId) && reflect.DeepEqual(*ref.GetKey().Grantee, *s.Grantee) {
return true
}
}
Expand All @@ -204,7 +208,8 @@ func (m *manager) UpdateShare(ctx context.Context, ref *collaboration.ShareRefer
user := user.ContextMustGetUser(ctx)
for i, s := range m.shares {
if equal(ref, s) {
if user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId {
if (user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId) ||
(user.Id.Idp == s.Creator.Idp && user.Id.OpaqueId == s.Creator.OpaqueId) {
now := time.Now().UnixNano()
m.shares[i].Permissions = p
m.shares[i].Mtime = &typespb.Timestamp{
Expand All @@ -224,8 +229,8 @@ func (m *manager) ListShares(ctx context.Context, filters []*collaboration.ListS
defer m.lock.Unlock()
user := user.ContextMustGetUser(ctx)
for _, s := range m.shares {
// TODO(labkode): add check for creator.
if user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId {
if (user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId) ||
(user.Id.Idp == s.Creator.Idp && user.Id.OpaqueId == s.Creator.OpaqueId) {
// no filter we return earlier
if len(filters) == 0 {
ss = append(ss, s)
Expand All @@ -252,9 +257,9 @@ func (m *manager) ListReceivedShares(ctx context.Context) ([]*collaboration.Rece
defer m.lock.Unlock()
user := user.ContextMustGetUser(ctx)
for _, s := range m.shares {
if user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId {
if (user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId) ||
(user.Id.Idp == s.Creator.Idp && user.Id.OpaqueId == s.Creator.OpaqueId) {
// omit shares created by me
// TODO(labkode): apply check for s.Creator also.
continue
}
if s.Grantee.Type == provider.GranteeType_GRANTEE_TYPE_USER {
Expand Down

0 comments on commit ceea2c5

Please sign in to comment.