feat: add permissions for public link shares#2483
Conversation
07381c4 to
9754375
Compare
35509c9 to
7a3163c
Compare
|
|
||
| if ($userId === '') { | ||
| return true; | ||
| return $this->isCli || $this->isPublicContext; |
There was a problem hiding this comment.
i wonder if this breaks existing shares 🤔
There was a problem hiding this comment.
It should not break existing shares, anything going through PublicRowOCSController, so also the existing getRows() will have isPublicContext set to true. I just wanted to be more explicit and safe here, in what cases a empty userId should be allowed.
Was there any other case than CLI and isPublicContext when a empty userId should bypass the permission check? We can of course also revert this line and always return true here again, just to be sure.
I think we should think about a way to refactor this later and maybe use something else than an empty userId to bypass the permission checks.
7a3163c to
c826307
Compare
There was a problem hiding this comment.
When a public share has delete permissions, the guest is able to delete rows from other tables 🙀 😱
To see, create a public share, open in an incognito/private tab and run in the console:
const TOKEN = location.pathname.match(/\/s\/([A-Za-z0-9]{16})\/?$/)?.[1] || 'TOKEN_HERE'
const ROW_ID = 16 // row from some other table
const res = await fetch(
`${location.origin}/ocs/v2.php/apps/tables/api/2/public/${TOKEN}/rows/${ROW_ID}?format=json`,
{
method: 'DELETE',
headers: {
'OCS-APIRequest': 'true',
...(window.OC?.requestToken ? { requesttoken: OC.requestToken } : {}),
},
},
)
4936a9a to
94530a7
Compare
94530a7 to
6374a90
Compare
|
Hi @enjeck and thank you for catching the delete issue, I updated the PR. For now to keep it consistent with the |
c61cf0c to
bdceadf
Compare
blizzz
left a comment
There was a problem hiding this comment.
What we discussed earlier is good, with a non-blocking remark.
| $this->logger->error($e->getMessage(), ['exception' => $e]); | ||
| throw new InternalError(get_class($this) . ' - ' . __FUNCTION__ . ': ' . $e->getMessage()); | ||
| } | ||
|
|
There was a problem hiding this comment.
theoretically both table and view can be null. Not from the calling Controller, given it is internally called after validation there is no much pressure… but it leaves a funny feeling in the tummy. #paranoia
There was a problem hiding this comment.
As a defensive fix, I now added a simple check to the create, update and delete in the PublicRowOCSController to be sure they can not call the service with both tableId and viewId being null.
d6ab44d to
1634366
Compare
|
cypress: it is always the last three cases that fail. So far. |
a259ec6 to
8252979
Compare
Signed-off-by: Benjamin Frueh <benjamin.frueh@gmail.com>
8252979 to
10d8429
Compare
@blizzz The toast message "Row successfully created." was stacking up ~5 times and covering elements required for cypress tests. The test opens the create row modal, fills and sends it in fast succession. I now added to the cypress helper that any success toast from creating a row will be closed before continue. Cypress should pass again with that change now. Before this PR, the success toast was only shown when the "Add more" option is active in the create modal. I changed this with the newly introduced form mode (create only permission), to always give feedback even if no added row is visually shown without the read permission. |
nope, link sharing was introduced in 2.0 only |
Adds view, create, update and delete permissions for public table shares.
public-<token>for thecreated_byandlast_edit_byvalue.Closes #2389