Public Shares CRUD, File Public Shares Manager #681

refs · 2020-04-23T10:24:09Z

What?

Other Tasks

Do not return expired shares to Phoenix.
Persistent shares manager.

Description

The file manager, as per commit 8503ee3 needs an existing file to be used as storage, as the revad process doesn't have write permissions(1).

The db file is thread safe, all operations lock a mutex, so goroutines consuming the file are sure to be using an up-to-date version.

Example file contents:

{
  "KTBfUQmYrEXy": "{\"id\":{\"opaqueId\":\"KTBfUQmYrEXy\"},\"token\":\"VjldgRWfpudX\",\"resourceId\":{\"storageId\":\"1284d238-aa92-42ce-bdc4-0b0000009162\",\"opaqueId\":\"19897125-ddd9-4939-be1d-12d86c503569\"},\"permissions\":{\"permissions\":{\"getPath\":true,\"getQuota\":true,\"initiateFileDownload\":true,\"listGrants\":true,\"listContainer\":true,\"listFileVersions\":true,\"listRecycle\":true,\"stat\":true}},\"owner\":{\"opaqueId\":\"einstein\"},\"creator\":{\"idp\":\"https://localhost:9200\",\"opaqueId\":\"einstein\"},\"ctime\":{\"seconds\":\"1587720640\",\"nanos\":939334000},\"mtime\":{\"seconds\":\"1587720755\",\"nanos\":187107000},\"expiration\":{\"seconds\":\"1587859200\"},\"displayName\":\"empty\"}",
  "jKoqDYLhZYDU": "{\"id\":{\"opaqueId\":\"jKoqDYLhZYDU\"},\"token\":\"orOSByQWpIvl\",\"resourceId\":{\"storageId\":\"1284d238-aa92-42ce-bdc4-0b0000009162\",\"opaqueId\":\"621502b8-9ed1-43c9-9a4e-b4ecf2711752\"},\"permissions\":{\"permissions\":{\"getPath\":true,\"getQuota\":true,\"initiateFileDownload\":true,\"listGrants\":true,\"listContainer\":true,\"listFileVersions\":true,\"listRecycle\":true,\"stat\":true}},\"owner\":{\"opaqueId\":\"einstein\"},\"creator\":{\"idp\":\"https://localhost:9200\",\"opaqueId\":\"einstein\"},\"ctime\":{\"seconds\":\"1587975510\",\"nanos\":771457000},\"mtime\":{\"seconds\":\"1587975510\",\"nanos\":771457000},\"displayName\":\"demo1\"}",
  "uJcxqIvypjUo": "{\"id\":{\"opaqueId\":\"uJcxqIvypjUo\"},\"token\":\"RozaxRnMYtjQ\",\"resourceId\":{\"storageId\":\"1284d238-aa92-42ce-bdc4-0b0000009162\",\"opaqueId\":\"19897125-ddd9-4939-be1d-12d86c503569\"},\"permissions\":{\"permissions\":{\"getPath\":true,\"getQuota\":true,\"initiateFileDownload\":true,\"listGrants\":true,\"listContainer\":true,\"listFileVersions\":true,\"listRecycle\":true,\"stat\":true}},\"owner\":{\"opaqueId\":\"einstein\"},\"creator\":{\"idp\":\"https://localhost:9200\",\"opaqueId\":\"einstein\"},\"ctime\":{\"seconds\":\"1587720648\",\"nanos\":135337000},\"mtime\":{\"seconds\":\"1587975296\",\"nanos\":942731000},\"expiration\":{\"seconds\":\"1588204800\"},\"displayName\":\"3\"}"
}

Share contents comparison (link.PublicShare) is achieved by string comparison, rather than reflection (no reflect.DeepEqual) or a comparison interface. This was a design choice meant to be replaced by https://pkg.go.dev/github.com/google/go-cmp/cmp for comparing protobuf struct.

Operations require a small overhead:

read file contents onto a map[string]interface{}
1.1 keys are PublicShares OpaqueID
1.2 contents are serialized json (using jsonpb, deprecated and should use ptorojson instead)
range over the shares
use jsonpb Unmarshaler and unmarshal v's data onto a *link.PublicShare struct

From this point on, a PublicShare struct is available to be used.

Write / Update operations follow the same approach, only an extra step that Marshals the contents of the *link.PublicShare onto the file on a new key or overriding an existing one.

(1) this is an assumption by trial and error. Need solid proof.

refs · 2020-04-28T21:33:04Z

internal/grpc/services/gateway/publicshareprovider.go

@@ -43,6 +43,61 @@ func (s *svc) CreatePublicShare(ctx context.Context, req *link.CreatePublicShare
 		return nil, err
 	}

+	// Grants are not accessible through a link.PublicShare, in order for a service to access a grant (link.Grant) this needs to be


WIP. We need to persist the grant for basic authentication password checking. Still need to figure out whether to commit it to the storage or find another way

Hi @refs I don't think we need to commit the grant to storage for a public share, keeping it outside the storage is good enough. User accessing a public link they cannot access the storage directly anyway. What can be stored in the storage is a reference to the public link ID as metadata.

That makes a clean separation of concerns. Currently the password is stored on a link.Grant, that is required at the moment of a PublicShare creation, but the PublicShare itself does not include a Grant, by design. So in short, where would you think it would be a good way to store this? We were thinking on the public shares auth provider.

/cc @butonic

Bump. This is the only opinionated bit that needs to be resolved before having a functional persistent public shares storage

For the time being I'm running with an in-memory map for the lack of decision here. This should NOT be adopted in production, but as a proof of concept does the job on checking Basic credentials with the stored public share grants.

labkode · 2020-05-26T09:25:46Z

@refs we need to extend the interface to add a password parameter:
https://github.com/cs3org/reva/blob/master/pkg/publicshare/publicshare.go#L36

PVince81 · 2020-05-27T08:22:59Z

Some test results:

BUG: copy link to clipboard is missing the host name

PVince81 · 2020-05-27T09:14:48Z

BUG: some regression introduced by ocdav: Implement PROPFIND infinity depth #758 because the PROPFIND there is working on paths instead of refs, so doesn't work cross-storage. => ocdav: use reference instead of path in PROPFIND #778

PVince81 · 2020-05-27T09:35:45Z

fix for cross-storage propfind is here: #778

PVince81 · 2020-05-27T13:25:10Z

I've updated #778 but the propfind is still broken, but in another way

Use original ref for Depth 1 which works fine. Depth infinity will have its own logic which currently only works within a single storage.

PVince81 · 2020-05-27T15:07:01Z

@refs I got stuck with even more issues when resolving by resource id in the PROPFIND.

To get us unblocked, I've raised another PR #779 to restore the Depth-1 logic to what it was before the regression. The Depth-infinity logic is separate now.

PVince81 · 2020-05-27T15:10:03Z

@refs please merge refs#1 to receive the fix into your branch.

I could only test with curl but not with public shares as my setup is not working yet. Please verify

refs · 2020-05-27T17:36:10Z

@refs please merge refs#1 to receive the fix into your branch.

I could only test with curl but not with public shares as my setup is not working yet. Please verify

@PVince81 public links behave as expected with that provisional patch 👍

refs · 2020-05-28T07:25:07Z

@labkode I think we can merge as it is so this doesn't grow larger, and I'll be addressing the remaining issues in the upcoming weeks :)

labkode · 2020-05-28T11:23:56Z

internal/grpc/services/publicstorageprovider/publicstorageprovider.go

@@ -58,7 +59,10 @@ func (s *service) Close() error {
 	return nil
 }

-func (s *service) UnprotectedEndpoints() []string { return []string{} }
+func (s *service) UnprotectedEndpoints() []string {
+	// return []string{"/cs3.sharing.link.v1beta1.LinkAPI/GetPublicShareByToken"}


I think needs to be unprotected

labkode · 2020-05-28T11:25:58Z

internal/grpc/services/publicstorageprovider/publicstorageprovider.go

 	}

-	publicShareResponse, err := s.gateway.GetPublicShareByToken(
+	publicShareResponse, err := driver.GetPublicShare(


This was correct, the method is GetPublicShareByToken

refs added 14 commits April 20, 2020 11:45

set permissions from phoenix

c3e9ef4

CRUD operations on public shares

7ac248a

WIP persistent shares storage

41676b3

json public share manager

bee35ad

fix filter, add locally scoped var

5f8a0df

liberate mutex on defer

a7bb4e6

correct time parsing on creation

9b6cc77

hide expired public shares

b7499d5

use correct timestamp on create and update

613e636

add comment for future generations

9cdada6

get shares by token working

c1cb793

defer mutex unlock, refactor GetShare

bb642fa

use mutex on get public share

2ad8bf1

code style

8503ee3

refs changed the title ~~CRUD on Public Shares~~ CRUD on Public Shares, File Public Shares Manager Apr 27, 2020

refs changed the title ~~CRUD on Public Shares, File Public Shares Manager~~ Public Shares CRUD, File Public Shares Manager Apr 27, 2020

refs added 4 commits April 27, 2020 13:39

Merge branch 'master' into public-shares

6e53187

address linter

ac9ab44

navigation working

6c84f2a

folder navigation on a public share is now supported

6c118b2

refs commented Apr 28, 2020

View reviewed changes

refs mentioned this pull request Apr 28, 2020

Public Links owncloud/ocis-reva#133

Closed

refs added 5 commits April 29, 2020 08:19

remove spurious lines

a8fe9ee

attempt to create db file if not exists

da5a7d2

base64 encode password

b160fcd

in memory grant manager as a proof of concept

52c9d2d

remove spurious code

20dfb8b

refs marked this pull request as ready for review May 8, 2020 09:54

refs requested a review from labkode May 8, 2020 10:03

make use of err, get rid of ineffectual assignment

8bfcc03

refs added 3 commits May 26, 2020 10:40

use a db file for link.Grant

9b6adc7

fix linter

455e6da

Merge branch 'master' into public-shares

58e0b25

fix propfind stack

21dbf29

PVince81 mentioned this pull request May 27, 2020

ocdav: use reference instead of path in PROPFIND #778

Closed

defer usage of ref

a07b579

Fix PROPFIND with Depth 1

39f3fc5

Use original ref for Depth 1 which works fine. Depth infinity will have its own logic which currently only works within a single storage.

PVince81 mentioned this pull request May 27, 2020

Separate PROPFIND logic for Depth 1 and infinity #779

Merged

Merge pull request #1 from PVince81/public-shares

7fd06cf

Merge branch 'master' into public-shares

72fba4b

refs requested a review from labkode May 28, 2020 07:24

labkode and others added 6 commits May 28, 2020 10:17

publicshare: commit grant

68c1932

Merge pull request #2 from labkode/refs-hugo

f395902

adapt db json file marshal / unmarshal

df39963

remove deadcode

d6eaa1c

remove yet more dead code

91bf637

remove unused config parameter

e42564b

labkode reviewed May 28, 2020

View reviewed changes

labkode merged commit f599518 into cs3org:master May 28, 2020

PVince81 mentioned this pull request Jan 18, 2021

Troubles with Depth:infinity and cross-storage owncloud/ocis#1301

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Public Shares CRUD, File Public Shares Manager #681

Public Shares CRUD, File Public Shares Manager #681

refs commented Apr 23, 2020 •

edited

Loading

refs Apr 28, 2020

labkode Apr 29, 2020

refs Apr 29, 2020

refs May 4, 2020

refs May 8, 2020

labkode commented May 26, 2020 •

edited

Loading

PVince81 commented May 27, 2020

PVince81 commented May 27, 2020 •

edited

Loading

PVince81 commented May 27, 2020

PVince81 commented May 27, 2020

PVince81 commented May 27, 2020

PVince81 commented May 27, 2020

refs commented May 27, 2020

refs commented May 28, 2020

labkode May 28, 2020

labkode May 28, 2020

Public Shares CRUD, File Public Shares Manager #681

Public Shares CRUD, File Public Shares Manager #681

Conversation

refs commented Apr 23, 2020 • edited Loading

What?

Other Tasks

Description

refs Apr 28, 2020

Choose a reason for hiding this comment

labkode Apr 29, 2020

Choose a reason for hiding this comment

refs Apr 29, 2020

Choose a reason for hiding this comment

refs May 4, 2020

Choose a reason for hiding this comment

refs May 8, 2020

Choose a reason for hiding this comment

labkode commented May 26, 2020 • edited Loading

PVince81 commented May 27, 2020

PVince81 commented May 27, 2020 • edited Loading

PVince81 commented May 27, 2020

PVince81 commented May 27, 2020

PVince81 commented May 27, 2020

PVince81 commented May 27, 2020

refs commented May 27, 2020

refs commented May 28, 2020

labkode May 28, 2020

Choose a reason for hiding this comment

labkode May 28, 2020

Choose a reason for hiding this comment

refs commented Apr 23, 2020 •

edited

Loading

labkode commented May 26, 2020 •

edited

Loading

PVince81 commented May 27, 2020 •

edited

Loading