owncloud / ocis

:atom_symbol: ownCloud Infinite Scale Stack
https://doc.owncloud.com/ocis/next/
Apache License 2.0
1.4k stars 183 forks source link

Deleting password of public link share when enforce password is enabled returns 996 ocs code #6619

Closed PrajwolAmatya closed 10 months ago

PrajwolAmatya commented 1 year ago

Describe the bug

Removing the password of a public link share when the config OCIS_SHARING_PUBLIC_WRITEABLE_SHARE_MUST_HAVE_PASSWORD is set to true.

Steps to reproduce

Steps to reproduce the behavior:

  1. Set OCIS_SHARING_PUBLIC_WRITEABLE_SHARE_MUST_HAVE_PASSWORD=true
  2. Upload a file testfile.txt
  3. create a public link share with edit permission curl -k -X POST https://localhost:9200/ocs/v2.php/apps/files_sharing/api/v1/shares -d 'shareType=3&path=/testfile.txt&permissions=3&password=1234' -u <username>:<password> -v
  4. Try to remove the password of the public link curl -k -X PUT https://localhost:9200/ocs/v2.php/apps/files_sharing/api/v1/shares/<share-id> -d 'password=' -u <username>:<password> -v

Expected behavior

The response should return 400 ocs code.

Actual behavior

The response is returning 996 ocs code.

Actual Response

> PUT /ocs/v2.php/apps/files_sharing/api/v1/shares/OVSRqusgeRzOcPH HTTP/1.1
> Host: localhost:9200
> Authorization: Basic YWRtaW46YWRtaW4=
> User-Agent: curl/7.81.0
> Accept: */*
> Content-Length: 9
> Content-Type: application/x-www-form-urlencoded
> 
* TLSv1.2 (IN), TLS header, Supplemental data (23):
* TLSv1.3 (IN), TLS handshake, Newsession Ticket (4):
* TLSv1.2 (IN), TLS header, Supplemental data (23):
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< Content-Length: 227
< Content-Type: text/xml; charset=utf-8
< Date: Mon, 26 Jun 2023 07:20:26 GMT
< Ocs-Api-Version: 1
< Vary: Origin
< 
<?xml version="1.0" encoding="UTF-8"?>
* Connection #0 to host localhost left intact
<ocs><meta><status>error</status><statuscode>996</statuscode><message>Error sending update request to public link provider: the public share needs to have a password</message></meta></ocs>
2403905 commented 1 year ago

@PrajwolAmatya When the frontend sends such a request it also includes the "permissions" value. If you call curl -k -X PUT https://localhost:9200/ocs/v2.php/apps/files_sharing/api/v1/shares/<share-id> -d 'permissions=3&password=' -u <username>:<password> -v the API returns 400 <ocs><meta><status>error</status><statuscode>400</statuscode><message>missing required password</message></meta></ocs>

2403905 commented 1 year ago

@PrajwolAmatya But if you think it is the critical spot I'll fix it.

PrajwolAmatya commented 1 year ago

I think while updating the link, only the required attribute should be used, i.e., password=. Since we are updating the public link and while updating the link, setting the permission to 3 should not be necessary in my opinion and only the required attribute value should be used. Referenced this docs: https://lukasreschke.github.io/OpenCloudMeshSpecification/#update-an-existing-share https://docs.nextcloud.com/server/latest/developer_manual//client_apis/OCS/ocs-share-api.html#update-share @2403905

SwikritiT commented 11 months ago

If I'm not wrong, the current behavior of the server is that the password can be removed only by people with specific roles like Admin user and the password is enforced in links by default is this issue relevant anymore? We also have some test coverage as a part of this issue https://github.com/owncloud/ocis/issues/7823 cc @2403905 @PrajwolAmatya @ScharfViktor @saw-jan

saw-jan commented 10 months ago

If you are sure about the tests coverage then maybe we can close this one. Scenario that needs to be covered: https://github.com/owncloud/ocis/pull/6465/files

SwikritiT commented 10 months ago

If you are sure about the tests coverage then maybe we can close this one. Scenario that needs to be covered: https://github.com/owncloud/ocis/pull/6465/files

I think this is covered here , so the PR can also be closed https://github.com/owncloud/ocis/blob/55f3ef0aff4c2ee58e4cf7b8a38391b43c0c576f/tests/acceptance/features/coreApiSharePublicLink1/changingPublicLinkShare.feature#L136

https://github.com/owncloud/ocis/blob/55f3ef0aff4c2ee58e4cf7b8a38391b43c0c576f/tests/acceptance/features/apiSpacesShares/shareSpacesViaLink.feature#L254

SwikritiT commented 10 months ago

I think while updating the link, only the required attribute should be used, i.e., password=. Since we are updating the public link and while updating the link, setting the permission to 3 should not be necessary in my opinion and only the required attribute value should be used. Referenced this docs: https://lukasreschke.github.io/OpenCloudMeshSpecification/#update-an-existing-share https://docs.nextcloud.com/server/latest/developer_manual//client_apis/OCS/ocs-share-api.html#update-share @2403905

this was covered and fixed as a part of this issue https://github.com/owncloud/ocis/issues/7821

saw-jan commented 10 months ago

I am closing this ticket as fixed.

CC @2403905 @ScharfViktor