Skip to content
This repository has been archived by the owner on Jan 18, 2021. It is now read-only.

Edit user share response might be missing file ids #332

Closed
3 tasks
PVince81 opened this issue Jul 2, 2020 · 10 comments · Fixed by cs3org/reva#958
Closed
3 tasks

Edit user share response might be missing file ids #332

PVince81 opened this issue Jul 2, 2020 · 10 comments · Fixed by cs3org/reva#958
Assignees
Labels
bug Something isn't working QA-team

Comments

@PVince81
Copy link
Contributor

PVince81 commented Jul 2, 2020

See https://github.com/cs3org/reva/blob/master/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/shares.go#L883

It seems to be missing a call to stat() and addFileInfo().
Maybe we should refactor that logic of userShare2ShareData to automatically do the stat to avoid forgetting in further places...

  • reproduce issue to confirm
  • fix
  • find matching API tests or add new ones

@individual-it FYI might be nice to add tests beforehand

@PVince81 PVince81 added bug Something isn't working QA-team labels Jul 2, 2020
@PVince81
Copy link
Contributor Author

PVince81 commented Jul 6, 2020

Steps

  1. Create a folder "test"
  2. Share "test" with "marie"
  3. Open the network console
  4. Edit the share
  5. Change the permission to any value
  6. Click "Save"
  7. Check the matching PUT request

Expected result

Ids are present

Actual result

Many fields are missing values like "item_source", "file_source", "file_target".

<?xml version="1.0" encoding="UTF-8"?>
<ocs>
  <meta>
    <status>ok</status>
    <statuscode>100</statuscode>
    <message>OK</message>
  </meta>
  <data>
    <id>4db43c34-183e-45e0-b859-712cac6436e4</id>
    <share_type>0</share_type>
    <uid_owner>einstein</uid_owner>
    <displayname_owner>Einstein</displayname_owner>
    <permissions>15</permissions>
    <stime>1594022584</stime>
    <parent/>
    <expiration/>
    <token/>
    <uid_file_owner>einstein</uid_file_owner>
    <displayname_file_owner>Einstein</displayname_file_owner>
    <additional_info_owner/>
    <additional_info_file_owner/>
    <state>0</state>
    <path/>
    <item_type/>
    <mimetype/>
    <storage_id/>
    <storage>0</storage>
    <item_source/>
    <file_source/>
    <file_parent/>
    <file_target/>
    <share_with>marie</share_with>
    <share_with_displayname>Curie</share_with_displayname>
    <share_with_additional_info/>
    <mail_send>0</mail_send>
    <name/>
  </data>
</ocs>

I believe we already have some tests that indirectly use share updates when testing.
We should add one that directly tests those fields.

I wonder if maybe we already have some but they might be disabled for other reasons.

@individual-it individual-it self-assigned this Jul 6, 2020
@individual-it
Copy link
Member

diff of oc10 response (left) and OCIS response (right) when changing permissions of a share

<?xml version="1.0" encoding="UTF-8"?>				<?xml version="1.0" encoding="UTF-8"?>
<ocs>								<ocs>
   <meta>							   <meta>
      <status>ok</status>					      <status>ok</status>
      <statuscode>100</statuscode>				      <statuscode>100</statuscode>
      <message />					      |	      <message>OK</message>
      <totalitems />					      <
      <itemsperpage />					      <
   </meta>							   </meta>
   <data>							   <data>
      <id>391</id>					      |	      <id>bcac9575-e47a-45be-99ee-d9b7e10000c8</id>
      <share_type>0</share_type>				      <share_type>0</share_type>
      <uid_owner>Alice</uid_owner>				      <uid_owner>Alice</uid_owner>
      <displayname_owner>Alice Hansen</displayname_owner>	      <displayname_owner>Alice Hansen</displayname_owner>
      <permissions>31</permissions>				      <permissions>31</permissions>
      <stime>1594023692</stime>				      |	      <stime>1594024221</stime>
      <parent />						      <parent />
      <expiration />						      <expiration />
      <token />							      <token />
      <uid_file_owner>Alice</uid_file_owner>			      <uid_file_owner>Alice</uid_file_owner>
      <displayname_file_owner>Alice Hansen</displayname_file_	      <displayname_file_owner>Alice Hansen</displayname_file_
      <additional_info_owner />					      <additional_info_owner />
      <additional_info_file_owner />				      <additional_info_file_owner />
      <path>/Alice-folder</path>			      |	      <state>0</state>
      <item_type>folder</item_type>			      |	      <path />
      <mimetype>httpd/unix-directory</mimetype>		      |	      <item_type />
      <storage_id>home::Alice</storage_id>		      |	      <mimetype />
      <storage>556</storage>				      |	      <storage_id />
      <item_source>2147537010</item_source>		      |	      <storage>0</storage>
      <file_source>2147537010</file_source>		      |	      <item_source />
      <file_parent>2147536991</file_parent>		      |	      <file_source />
      <file_target>/Alice-folder</file_target>		      |	      <file_parent />
							      >	      <file_target />
      <share_with>Brian</share_with>				      <share_with>Brian</share_with>
      <share_with_displayname>Brian Murphy</share_with_displa	      <share_with_displayname>Brian Murphy</share_with_displa
      <share_with_additional_info />				      <share_with_additional_info />
      <mail_send>0</mail_send>					      <mail_send>0</mail_send>
      <attributes />					      |	      <name />
   </data>							   </data>
</ocs>								</ocs>

@individual-it
Copy link
Member

individual-it commented Jul 6, 2020

test PR (WIP) owncloud/core#37656

@PVince81
Copy link
Contributor Author

PVince81 commented Jul 9, 2020

@PVince81
Copy link
Contributor Author

PVince81 commented Jul 9, 2020

and now to enable the tests again... or do we need to wait for the ocis PR update as well ??

@individual-it
Copy link
Member

We can enable the tests anytime, in cs3org/reva owncloud/ocis and owncloud/ocis-reva the tests are all pinned to a specific commit id, so as long as that is not advanced it will not use the new (enabled) tests

@PVince81
Copy link
Contributor Author

PVince81 commented Jul 9, 2020

Yes... but if someone feels the need to advance it for another ticket 💥

anyway, let's take the risk and update the test right now

@PVince81
Copy link
Contributor Author

PVince81 commented Jul 9, 2020

damn, first need to fix the issue... I mixed this one up with the oder "update share ticket" where we are done

@PVince81
Copy link
Contributor Author

PVince81 commented Jul 9, 2020

@individual-it seems none of the tests in owncloud/core#37656 were tagged with this issue 332 ?

@PVince81
Copy link
Contributor Author

PVince81 commented Jul 9, 2020

PR for the fix: cs3org/reva#958

@PVince81 PVince81 self-assigned this Jul 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working QA-team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants