Skip to content

Conversation

@starypatyk
Copy link
Contributor

When working on nextcloud/server#34918, I have noticed that after applying changes proposed in that PR, incorrect permissions are returned for room shares.

This seems to be caused by the fact that the database query issued in RoomShareProvider::getSharesInFolder returnes two columns named permissions - one from the oc_shares table and a second one from oc_filecache. Which column is then used by the PHP code probably depends on the database driver used and maybe some random factors. In my case (MariaDB) the wrong column was consistently used.

In this PR I propose to change the SELECT part of the query to match other methods in the class - getSharesByIds and getSharedWith.

This change also means that the node data will be stored in the cache: https://github.com/nextcloud/spreed/blob/master/lib/Share/RoomShareProvider.php#L313, but I do not know what side effects this might introduce.

@nickvergessen
Copy link
Member

Looks good

@nickvergessen nickvergessen added feature: api 🛠️ OCS API for conversations, chats and participants feature: upload & shares & voice 📤🎙️ Sharing files into a chat and audio recordings labels Nov 5, 2022
@nickvergessen
Copy link
Member

When working on nextcloud/server#34918, I have noticed that after applying changes proposed in that PR, incorrect permissions are returned for room shares.

So another server PR breaking things? :P

Can we for the fun of it send a PR to talk to run integration tests against it? (Replace CORE_BRANCH: master with your PR in .drone.yml? (That being said, you didn't push the branch to the org, so it won't work for now)

@nickvergessen nickvergessen added this to the 💟 Next Major (26) milestone Nov 7, 2022
@starypatyk
Copy link
Contributor Author

@nickvergessen

So another server PR breaking things? :P

I would rather say exposing an existing issue... 😉
BTW - it started here: nextcloud/android#10783

Can we for the fun of it send a PR to talk to run integration tests against it? (Replace CORE_BRANCH: master with your PR in .drone.yml? (That being said, you didn't push the branch to the org, so it won't work for now)

As I understand I do not have permissions to push to nextcloud/server or nextcloud/spreed directly. It seems however, that changing CORE_BRANCH: master to CORE_BRANCH: refs/pull/34918/head in .drone.yml might do the trick.

@nickvergessen
Copy link
Member

As I understand I do not have permissions to push to nextcloud/server or nextcloud/spreed directly.

You are a member of our GitHub Organisation and therefor should have permissions to push branches just fine.

@nickvergessen
Copy link
Member

Added a PR to test that branch at #8312

@nickvergessen
Copy link
Member

Confirmed broken in #8312
Pushed your fix there

@nickvergessen
Copy link
Member

Fixes it as per #8312

So 👍

@nickvergessen nickvergessen merged commit 6654233 into nextcloud:master Nov 8, 2022
@starypatyk starypatyk deleted the bugfix/noid/permissions_get_shares_in_folder branch November 8, 2022 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review bug feature: api 🛠️ OCS API for conversations, chats and participants feature: upload & shares & voice 📤🎙️ Sharing files into a chat and audio recordings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants