Allow full paths targets #1605
Allow full paths targets #1605
Conversation
|
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
There was a problem hiding this comment.
@labkode Path needs to contain the full path, while the base is expected as FileTarget
Also, can you remove this line? https://github.com/cs3org/reva/blob/master/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/shares.go#L641
internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/shares.go
Outdated
Show resolved
Hide resolved
internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/shares.go
Show resolved
Hide resolved
internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/shares.go
Outdated
Show resolved
Hide resolved
|
The share That being said, the current implementation is indeed wrong because it only returns the basename. Some more details on how the In oc10 the path is built as $result['path'] = $userFolder->getRelativePath($shareNode->getPath());whereas the file_target is the mount point of the recipient $result['file_target'] = $share->getTarget();getTarget directly comes from the db and returns the recipients mount point. In the web ui the relevant code is in apps/files_sharing/js/sharedfilelist.js if (self._sharedWithUser) {
// Whether the list shows the files shared with the user (true) or
file.shareOwner = share.displayname_owner;
file.shareState = share.state;
file.name = OC.basename(share.file_target);
file.path = OC.dirname(share.file_target);
file.permissions = share.permissions;
if (file.path) {
file.extraData = share.file_target;
}
}
else {
// the files that the user shared with others (false).
if (share.share_type !== OC.Share.SHARE_TYPE_LINK) {
file.share.targetDisplayName = share.share_with_displayname;
}
file.name = OC.basename(share.path);
file.path = OC.dirname(share.path);
file.permissions = OC.PERMISSION_ALL;
if (file.path) {
file.extraData = share.path;
}
}
I'll dig deeper after lunch... |
|
@butonic that explains it. We are returning a full path. I think nor the server nor the UI should to any assumptions and only show the basename and when clicking use the file_target wherever it points to. |
|
Note about tests: if, after discussion, it is agreed to have some behaviour in reva/OCIS that is "a bit" different to oC10, then we can adjust for that in the tests by:
If the new behaviour is made a config option, then the existing oC10 API tests will pass. And "local" acceptance tests can be added that run with the config option, and describe/test the new behaviour. |
|
let me throw in my 2 cents. We have the requirement that ownCloud web and all other clients are 100% compatible with ocis backend and oc10 backend. If we do introduce a "slightly" different behavior of the APIs, we need to do extensive research to not break anything, or maybe also follow with oc10 to keep compatibility. |
|
In owncloud/web the same code looks like this: if (incomingShares) {
resource.resourceOwner = {
username: share.uid_file_owner,
displayName: share.displayname_file_owner
}
resource.owner = [
{
username: share.uid_owner,
displayName: share.displayname_owner,
avatar: await getAvatarSrc(share.uid_file_owner, server, token)
}
]
resource.status = share.state
resource.name = path.basename(share.file_target)
resource.path = share.file_target
resource.isReceivedShare = () => true
} else {
resource.sharedWith = share.sharedWith
resource.shareOwner = share.uid_owner
resource.shareOwnerDisplayname = share.displayname_owner
resource.name = path.basename(share.path)
resource.path = share.path
// permissions irrelevant here
resource.isReceivedShare = () => false
}So both oc10 and ocis/web do not need the The desktop client only uses the Next thing is the reva ocs
To conclude:
|
|
@mbarz the question is if tho oc10 web ui needs to work with ocis? |
|
Regarding tests, this is an example that we can use to clarify why they fail:
We already determined that the ocs handler implementation for When listing shares with others (and links)
When listing 🤔 totally different beast. This needs to list the mount points of the shared file in the
In a global namespace there is no neccessity for a A virtual But determining where to sync can also be done by listing the storage spaces a user has access to. Every share creates its own storage space and the storage registry can cache the etag for it ... For the time being, as well as for older desktop clients, we can provide a |
|
@butonic @micbar my addition: in current OCIS master the shared with me functionality is half-broken. If I share a file/folder with a user, the recipient can click on it but you get an error message. Only after accepting the share you can list the contents. By having the full path exposed (global namespace) you can still access the share. Our config already reflects the global namespace: |
|
@butonic @phil-davis Can we figure out how to make this work? If I got you right @butonic the change is not conflicting with the Classic APIs. |
|
This is caused by jailing users into the /home folder: on it
|
|
@labkode @ishank011 labkode#183 should fix the tests by using paths from the Share if it is configured. |
|
@ishank011 continued in #1691? |
|
@butonic nope, I just tried to see if your commit passes the CI. Refer labkode#183 (comment) and labkode#183 (comment) |
|
trying to reproduce with: but running into panics. AFAICT we need diff --git a/internal/grpc/services/gateway/storageprovider.go b/internal/grpc/services/gateway/storageprovider.go
index 90a5a042..6052b926 100644
--- a/internal/grpc/services/gateway/storageprovider.go
+++ b/internal/grpc/services/gateway/storageprovider.go
@@ -1225,6 +1225,23 @@ func (s *svc) statOnProvider(ctx context.Context, req *provider.StatRequest, res
*e = errors.Wrap(err, "gateway: error calling ListContainer")
return
}
+
+ if r.Status.Code != rpc.Code_CODE_OK {
+ switch r.Status.Code {
+ case rpc.Code_CODE_NOT_FOUND:
+ *e = errtypes.NotFound(newPath)
+ case rpc.Code_CODE_PERMISSION_DENIED:
+ *e = errtypes.PermissionDenied(newPath)
+ case rpc.Code_CODE_INVALID_ARGUMENT, rpc.Code_CODE_FAILED_PRECONDITION, rpc.Code_CODE_OUT_OF_RANGE:
+ *e = errtypes.BadRequest(newPath)
+ case rpc.Code_CODE_UNIMPLEMENTED:
+ *e = errtypes.NotSupported(newPath)
+ default:
+ *e = errtypes.InternalError("gateway: error stating target reference")
+ }
+ return
+ }
+
if res == nil {
res = &provider.ResourceInfo{}
}
diff --git a/pkg/storage/utils/decomposedfs/lookup.go b/pkg/storage/utils/decomposedfs/lookup.go
index 0de4106b..f4d2b4ae 100644
--- a/pkg/storage/utils/decomposedfs/lookup.go
+++ b/pkg/storage/utils/decomposedfs/lookup.go
@@ -71,6 +71,9 @@ func (lu *Lookup) NodeFromPath(ctx context.Context, fn string) (*node.Node, erro
if err != nil {
return nil, err
}
+ } else {
+ // allow listing and stating root
+ n.Exists = true
}
return n, nil
diff --git a/pkg/storage/utils/decomposedfs/node/permissions.go b/pkg/storage/utils/decomposedfs/node/permissions.go
index ea3e5cae..913cc949 100644
--- a/pkg/storage/utils/decomposedfs/node/permissions.go
+++ b/pkg/storage/utils/decomposedfs/node/permissions.go
@@ -37,7 +37,8 @@ var NoPermissions *provider.ResourcePermissions = &provider.ResourcePermissions{
// NoOwnerPermissions defines permissions for nodes that don't have an owner set, eg the root node
var NoOwnerPermissions *provider.ResourcePermissions = &provider.ResourcePermissions{
- Stat: true,
+ Stat: true,
+ ListContainer: true,
}
// OwnerPermissions defines permissions for nodes owned by the user |
|
deprecated by #1739 |
Visually it allows to click on a share item in the "Shared with me" tab (accepted or not) and access it in the owner's path.