Skip to content

Commit ceea2c5

Browse files
ishank011ishank011
authored
Disallow sharing the shares directory (#1051)
1 parent ea29c4f commit ceea2c5

File tree

6 files changed

+75
-30
lines changed

6 files changed

+75
-30
lines changed

‎changelog/unreleased/shares-fix.md

+7
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
Bugfix: Disallow sharing the shares directory
2+
3+
Previously, it was possible to create public links for and share the shares
4+
directory itself. However, when the recipient tried to accept the share, it
5+
failed. This PR prevents the creation of such shares in the first place.
6+
7+
https://github.com/cs3org/reva/pull/1051

‎internal/grpc/services/gateway/publicshareprovider.go

+4
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,10 @@ import (
2929
)
3030

3131
func (s *svc) CreatePublicShare(ctx context.Context, req *link.CreatePublicShareRequest) (*link.CreatePublicShareResponse, error) {
32+
if s.isSharedFolder(ctx, req.ResourceInfo.GetPath()) {
33+
return nil, errors.New("gateway: can't create a public share of the share folder itself")
34+
}
35+
3236
log := appctx.GetLogger(ctx)
3337
log.Info().Msg("create public share")
3438

‎internal/grpc/services/gateway/storageprovider.go

+19-1
Original file line numberDiff line numberDiff line change
@@ -941,8 +941,26 @@ func (s *svc) Stat(ctx context.Context, req *provider.StatRequest) (*provider.St
941941
Path: target,
942942
},
943943
}
944+
944945
req.Ref = ref
945-
return s.stat(ctx, req)
946+
res, err := s.stat(ctx, req)
947+
if err != nil {
948+
return &provider.StatResponse{
949+
Status: status.NewInternal(ctx, err, "gateway: error stating"),
950+
}, nil
951+
}
952+
if res.Status.Code != rpc.Code_CODE_OK {
953+
err := status.NewErrorFromCode(res.Status.Code, "gateway")
954+
log.Err(err).Msg("gateway: error stating")
955+
return &provider.StatResponse{
956+
Status: status.NewInternal(ctx, err, "gateway: error stating"),
957+
}, nil
958+
}
959+
960+
// we need to make sure we don't expose the reference target in the resource
961+
// information.
962+
res.Info.Path = p
963+
return res, nil
946964
}
947965

948966
panic("gateway: stating an unknown path:" + p)

‎internal/grpc/services/gateway/usershareprovider.go

+5
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,11 @@ import (
3535

3636
// TODO(labkode): add multi-phase commit logic when commit share or commit ref is enabled.
3737
func (s *svc) CreateShare(ctx context.Context, req *collaboration.CreateShareRequest) (*collaboration.CreateShareResponse, error) {
38+
39+
if s.isSharedFolder(ctx, req.ResourceInfo.GetPath()) {
40+
return nil, errors.New("gateway: can't share the share folder itself")
41+
}
42+
3843
c, err := pool.GetUserShareProviderClient(s.c.UserShareProviderEndpoint)
3944
if err != nil {
4045
return &collaboration.CreateShareResponse{

‎pkg/share/manager/json/json.go

+20-14
Original file line numberDiff line numberDiff line change
@@ -163,16 +163,17 @@ func (m *mgr) Share(ctx context.Context, md *provider.ResourceInfo, g *collabora
163163
Nanos: uint32(now % 1000000000),
164164
}
165165

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

173174
// check if share already exists.
174175
key := &collaboration.ShareKey{
175-
Owner: user.Id,
176+
Owner: md.Owner,
176177
ResourceId: md.Id,
177178
Grantee: g.Grantee,
178179
}
@@ -190,7 +191,7 @@ func (m *mgr) Share(ctx context.Context, md *provider.ResourceInfo, g *collabora
190191
ResourceId: md.Id,
191192
Permissions: g.Permissions,
192193
Grantee: g.Grantee,
193-
Owner: user.Id,
194+
Owner: md.Owner,
194195
Creator: user.Id,
195196
Ctime: ts,
196197
Mtime: ts,
@@ -223,7 +224,8 @@ func (m *mgr) getByKey(ctx context.Context, key *collaboration.ShareKey) (*colla
223224
m.Lock()
224225
defer m.Unlock()
225226
for _, s := range m.model.Shares {
226-
if key.Owner.Idp == s.Owner.Idp && key.Owner.OpaqueId == s.Owner.OpaqueId &&
227+
if ((key.Owner.Idp == s.Owner.Idp && key.Owner.OpaqueId == s.Owner.OpaqueId) ||
228+
(key.Owner.Idp == s.Creator.Idp && key.Owner.OpaqueId == s.Creator.OpaqueId)) &&
227229
key.ResourceId.StorageId == s.ResourceId.StorageId && key.ResourceId.OpaqueId == s.ResourceId.OpaqueId &&
228230
key.Grantee.Type == s.Grantee.Type && key.Grantee.Id.Idp == s.Grantee.Id.Idp && key.Grantee.Id.OpaqueId == s.Grantee.Id.OpaqueId {
229231
return s, nil
@@ -247,9 +249,9 @@ func (m *mgr) get(ctx context.Context, ref *collaboration.ShareReference) (s *co
247249
}
248250

249251
// check if we are the owner
250-
// TODO(labkode): check for creator also.
251252
user := user.ContextMustGetUser(ctx)
252-
if user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId {
253+
if (user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId) ||
254+
(user.Id.Idp == s.Creator.Idp && user.Id.OpaqueId == s.Creator.OpaqueId) {
253255
return s, nil
254256
}
255257

@@ -272,7 +274,8 @@ func (m *mgr) Unshare(ctx context.Context, ref *collaboration.ShareReference) er
272274
user := user.ContextMustGetUser(ctx)
273275
for i, s := range m.model.Shares {
274276
if equal(ref, s) {
275-
if user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId {
277+
if (user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId) ||
278+
(user.Id.Idp == s.Creator.Idp && user.Id.OpaqueId == s.Creator.OpaqueId) {
276279
m.model.Shares[len(m.model.Shares)-1], m.model.Shares[i] = m.model.Shares[i], m.model.Shares[len(m.model.Shares)-1]
277280
m.model.Shares = m.model.Shares[:len(m.model.Shares)-1]
278281
if err := m.model.Save(); err != nil {
@@ -293,7 +296,8 @@ func equal(ref *collaboration.ShareReference, s *collaboration.Share) bool {
293296
return true
294297
}
295298
} else if ref.GetKey() != nil {
296-
if reflect.DeepEqual(*ref.GetKey().Owner, *s.Owner) && reflect.DeepEqual(*ref.GetKey().ResourceId, *s.ResourceId) && reflect.DeepEqual(*ref.GetKey().Grantee, *s.Grantee) {
299+
if (reflect.DeepEqual(*ref.GetKey().Owner, *s.Owner) || reflect.DeepEqual(*ref.GetKey().Owner, *s.Creator)) &&
300+
reflect.DeepEqual(*ref.GetKey().ResourceId, *s.ResourceId) && reflect.DeepEqual(*ref.GetKey().Grantee, *s.Grantee) {
297301
return true
298302
}
299303
}
@@ -306,7 +310,8 @@ func (m *mgr) UpdateShare(ctx context.Context, ref *collaboration.ShareReference
306310
user := user.ContextMustGetUser(ctx)
307311
for i, s := range m.model.Shares {
308312
if equal(ref, s) {
309-
if user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId {
313+
if (user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId) ||
314+
(user.Id.Idp == s.Creator.Idp && user.Id.OpaqueId == s.Creator.OpaqueId) {
310315
now := time.Now().UnixNano()
311316
m.model.Shares[i].Permissions = p
312317
m.model.Shares[i].Mtime = &typespb.Timestamp{
@@ -331,7 +336,8 @@ func (m *mgr) ListShares(ctx context.Context, filters []*collaboration.ListShare
331336
user := user.ContextMustGetUser(ctx)
332337
for _, s := range m.model.Shares {
333338
// TODO(labkode): add check for creator.
334-
if user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId {
339+
if (user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId) ||
340+
(user.Id.Idp == s.Creator.Idp && user.Id.OpaqueId == s.Creator.OpaqueId) {
335341
// no filter we return earlier
336342
if len(filters) == 0 {
337343
ss = append(ss, s)
@@ -358,9 +364,9 @@ func (m *mgr) ListReceivedShares(ctx context.Context) ([]*collaboration.Received
358364
defer m.Unlock()
359365
user := user.ContextMustGetUser(ctx)
360366
for _, s := range m.model.Shares {
361-
if user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId {
367+
if (user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId) ||
368+
(user.Id.Idp == s.Creator.Idp && user.Id.OpaqueId == s.Creator.OpaqueId) {
362369
// omit shares created by me
363-
// TODO(labkode): apply check for s.Creator also.
364370
continue
365371
}
366372
if s.Grantee.Type == provider.GranteeType_GRANTEE_TYPE_USER {

‎pkg/share/manager/memory/memory.go

+20-15
Original file line numberDiff line numberDiff line change
@@ -75,15 +75,16 @@ func (m *manager) Share(ctx context.Context, md *provider.ResourceInfo, g *colla
7575
Nanos: uint32(now % 1000000000),
7676
}
7777

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

8485
// check if share already exists.
8586
key := &collaboration.ShareKey{
86-
Owner: user.Id,
87+
Owner: md.Owner,
8788
ResourceId: md.Id,
8889
Grantee: g.Grantee,
8990
}
@@ -100,7 +101,7 @@ func (m *manager) Share(ctx context.Context, md *provider.ResourceInfo, g *colla
100101
ResourceId: md.Id,
101102
Permissions: g.Permissions,
102103
Grantee: g.Grantee,
103-
Owner: user.Id,
104+
Owner: md.Owner,
104105
Creator: user.Id,
105106
Ctime: ts,
106107
Mtime: ts,
@@ -125,7 +126,8 @@ func (m *manager) getByKey(ctx context.Context, key *collaboration.ShareKey) (*c
125126
m.lock.Lock()
126127
defer m.lock.Unlock()
127128
for _, s := range m.shares {
128-
if key.Owner.Idp == s.Owner.Idp && key.Owner.OpaqueId == s.Owner.OpaqueId &&
129+
if ((key.Owner.Idp == s.Owner.Idp && key.Owner.OpaqueId == s.Owner.OpaqueId) ||
130+
(key.Owner.Idp == s.Creator.Idp && key.Owner.OpaqueId == s.Creator.OpaqueId)) &&
129131
key.ResourceId.StorageId == s.ResourceId.StorageId && key.ResourceId.OpaqueId == s.ResourceId.OpaqueId &&
130132
key.Grantee.Type == s.Grantee.Type && key.Grantee.Id.Idp == s.Grantee.Id.Idp && key.Grantee.Id.OpaqueId == s.Grantee.Id.OpaqueId {
131133
return s, nil
@@ -149,9 +151,9 @@ func (m *manager) get(ctx context.Context, ref *collaboration.ShareReference) (s
149151
}
150152

151153
// check if we are the owner
152-
// TODO(labkode): check for creator also.
153154
user := user.ContextMustGetUser(ctx)
154-
if user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId {
155+
if (user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId) ||
156+
(user.Id.Idp == s.Creator.Idp && user.Id.OpaqueId == s.Creator.OpaqueId) {
155157
return s, nil
156158
}
157159

@@ -174,7 +176,8 @@ func (m *manager) Unshare(ctx context.Context, ref *collaboration.ShareReference
174176
user := user.ContextMustGetUser(ctx)
175177
for i, s := range m.shares {
176178
if equal(ref, s) {
177-
if user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId {
179+
if (user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId) ||
180+
(user.Id.Idp == s.Creator.Idp && user.Id.OpaqueId == s.Creator.OpaqueId) {
178181
m.shares[len(m.shares)-1], m.shares[i] = m.shares[i], m.shares[len(m.shares)-1]
179182
m.shares = m.shares[:len(m.shares)-1]
180183
return nil
@@ -191,7 +194,8 @@ func equal(ref *collaboration.ShareReference, s *collaboration.Share) bool {
191194
return true
192195
}
193196
} else if ref.GetKey() != nil {
194-
if reflect.DeepEqual(*ref.GetKey().Owner, *s.Owner) && reflect.DeepEqual(*ref.GetKey().ResourceId, *s.ResourceId) && reflect.DeepEqual(*ref.GetKey().Grantee, *s.Grantee) {
197+
if (reflect.DeepEqual(*ref.GetKey().Owner, *s.Owner) || reflect.DeepEqual(*ref.GetKey().Owner, *s.Creator)) &&
198+
reflect.DeepEqual(*ref.GetKey().ResourceId, *s.ResourceId) && reflect.DeepEqual(*ref.GetKey().Grantee, *s.Grantee) {
195199
return true
196200
}
197201
}
@@ -204,7 +208,8 @@ func (m *manager) UpdateShare(ctx context.Context, ref *collaboration.ShareRefer
204208
user := user.ContextMustGetUser(ctx)
205209
for i, s := range m.shares {
206210
if equal(ref, s) {
207-
if user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId {
211+
if (user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId) ||
212+
(user.Id.Idp == s.Creator.Idp && user.Id.OpaqueId == s.Creator.OpaqueId) {
208213
now := time.Now().UnixNano()
209214
m.shares[i].Permissions = p
210215
m.shares[i].Mtime = &typespb.Timestamp{
@@ -224,8 +229,8 @@ func (m *manager) ListShares(ctx context.Context, filters []*collaboration.ListS
224229
defer m.lock.Unlock()
225230
user := user.ContextMustGetUser(ctx)
226231
for _, s := range m.shares {
227-
// TODO(labkode): add check for creator.
228-
if user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId {
232+
if (user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId) ||
233+
(user.Id.Idp == s.Creator.Idp && user.Id.OpaqueId == s.Creator.OpaqueId) {
229234
// no filter we return earlier
230235
if len(filters) == 0 {
231236
ss = append(ss, s)
@@ -252,9 +257,9 @@ func (m *manager) ListReceivedShares(ctx context.Context) ([]*collaboration.Rece
252257
defer m.lock.Unlock()
253258
user := user.ContextMustGetUser(ctx)
254259
for _, s := range m.shares {
255-
if user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId {
260+
if (user.Id.Idp == s.Owner.Idp && user.Id.OpaqueId == s.Owner.OpaqueId) ||
261+
(user.Id.Idp == s.Creator.Idp && user.Id.OpaqueId == s.Creator.OpaqueId) {
256262
// omit shares created by me
257-
// TODO(labkode): apply check for s.Creator also.
258263
continue
259264
}
260265
if s.Grantee.Type == provider.GranteeType_GRANTEE_TYPE_USER {

0 commit comments

Comments
 (0)