-
Notifications
You must be signed in to change notification settings - Fork 118
feat(sharing): Allow custom share tokens for public links #3348
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -207,17 +207,21 @@ public function newShare(int $formId, int $shareType, string $shareWith = '', ar | |||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Update permissions of a share | ||||||
| * Update properties of a share | ||||||
| * | ||||||
| * @param int $formId of the form | ||||||
| * @param int $shareId of the share to update | ||||||
| * @param array<string, mixed> $keyValuePairs Array of key=>value pairs to update. | ||||||
| * @return DataResponse<Http::STATUS_OK, int, array{}> | ||||||
| * @throws OCSBadRequestException Share doesn't belong to given Form | ||||||
| * @throws OCSBadRequestException Invalid permission given | ||||||
| * @throws OCSBadRequestException Invalid share token | ||||||
| * @throws OCSBadRequestException Share hash exists, please retry | ||||||
| * @throws OCSForbiddenException Custom public share tokens are not allowed | ||||||
| * @throws OCSForbiddenException This form is not owned by the current user | ||||||
| * @throws OCSForbiddenException Empty keyValuePairs, will not update | ||||||
| * @throws OCSForbiddenException Not allowed to update other properties than permissions | ||||||
| * @throws OCSForbiddenException Not allowed to update token on non-link share | ||||||
| * @throws OCSForbiddenException Not allowed to update unknown properties | ||||||
| * @throws OCSNotFoundException Could not find share | ||||||
| * | ||||||
| * 200: the id of the updated share | ||||||
|
|
@@ -226,7 +230,7 @@ public function newShare(int $formId, int $shareType, string $shareWith = '', ar | |||||
| #[NoAdminRequired()] | ||||||
| #[ApiRoute(verb: 'PATCH', url: '/api/v3/forms/{formId}/shares/{shareId}')] | ||||||
| public function updateShare(int $formId, int $shareId, array $keyValuePairs): DataResponse { | ||||||
| $this->logger->debug('Updating share: {shareId} of form {formId}, permissions: {permissions}', [ | ||||||
| $this->logger->debug('Updating share: {shareId} of form {formId}, values: {keyValuePairs}', [ | ||||||
| 'formId' => $formId, | ||||||
| 'shareId' => $shareId, | ||||||
| 'keyValuePairs' => $keyValuePairs | ||||||
|
|
@@ -256,22 +260,64 @@ public function updateShare(int $formId, int $shareId, array $keyValuePairs): Da | |||||
| throw new OCSForbiddenException('Empty keyValuePairs, will not update'); | ||||||
| } | ||||||
|
|
||||||
| //Don't allow to change other properties than permissions | ||||||
| if (count($keyValuePairs) > 1 || !array_key_exists('permissions', $keyValuePairs)) { | ||||||
| $this->logger->debug('Not allowed to update other properties than permissions'); | ||||||
| throw new OCSForbiddenException('Not allowed to update other properties than permissions'); | ||||||
| $allowedKeys = ['permissions', 'token']; | ||||||
| foreach (array_keys($keyValuePairs) as $key) { | ||||||
| if (!in_array($key, $allowedKeys, true)) { | ||||||
| $this->logger->debug('Not allowed to update other properties than permissions or token'); | ||||||
| throw new OCSForbiddenException('Not allowed to update other properties than permissions or token'); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| if (!$this->validatePermissions($keyValuePairs['permissions'], $formShare->getShareType())) { | ||||||
| if (array_key_exists('permissions', $keyValuePairs) && !$this->validatePermissions($keyValuePairs['permissions'], $formShare->getShareType())) { | ||||||
| throw new OCSBadRequestException('Invalid permission given'); | ||||||
| } | ||||||
|
|
||||||
| if (array_key_exists('token', $keyValuePairs)) { | ||||||
| if (!$this->configService->getAllowCustomPublicToken()) { | ||||||
| $this->logger->debug('Custom public share tokens are not allowed.'); | ||||||
| throw new OCSForbiddenException('Custom public share tokens are not allowed.'); | ||||||
| } | ||||||
|
|
||||||
| if ($formShare->getShareType() !== IShare::TYPE_LINK) { | ||||||
| $this->logger->debug('Not allowed to update token on non-link share'); | ||||||
| throw new OCSForbiddenException('Not allowed to update token on non-link share'); | ||||||
| } | ||||||
|
|
||||||
| if (!is_string($keyValuePairs['token'])) { | ||||||
| throw new OCSBadRequestException('Invalid share token'); | ||||||
| } | ||||||
|
|
||||||
| $token = $keyValuePairs['token']; | ||||||
| if (!array_key_exists('permissions', $keyValuePairs) && $token === $formShare->getShareWith()) { | ||||||
| return new DataResponse($formShare->getId()); | ||||||
| } | ||||||
|
|
||||||
| if ($token !== $formShare->getShareWith()) { | ||||||
| $this->validatePublicShareToken($token); | ||||||
|
|
||||||
| try { | ||||||
| $existingShare = $this->shareMapper->findPublicShareByHash($token); | ||||||
| if ($existingShare->getId() !== $formShare->getId()) { | ||||||
| $this->logger->debug('Share hash already exists.'); | ||||||
| throw new OCSBadRequestException('Share hash exists, please retry.'); | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should not leak that information
Suggested change
|
||||||
| } | ||||||
| } catch (DoesNotExistException $e) { | ||||||
| // Just continue, this is what we expect to happen (share hash not existing yet). | ||||||
| } | ||||||
|
|
||||||
| $formShare->setShareWith($token); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| $this->formsService->obtainFormLock($form); | ||||||
|
|
||||||
| $formShare->setPermissions($keyValuePairs['permissions']); | ||||||
| if (array_key_exists('permissions', $keyValuePairs)) { | ||||||
| $formShare->setPermissions($keyValuePairs['permissions']); | ||||||
| } | ||||||
| $formShare = $this->shareMapper->update($formShare); | ||||||
|
|
||||||
| if (in_array($formShare->getShareType(), [IShare::TYPE_USER, IShare::TYPE_GROUP, IShare::TYPE_USERGROUP, IShare::TYPE_CIRCLE], true)) { | ||||||
| if (array_key_exists('permissions', $keyValuePairs) | ||||||
| && in_array($formShare->getShareType(), [IShare::TYPE_USER, IShare::TYPE_GROUP, IShare::TYPE_USERGROUP, IShare::TYPE_CIRCLE], true)) { | ||||||
| if (in_array(Constants::PERMISSION_RESULTS, $keyValuePairs['permissions'], true)) { | ||||||
| $userFolder = $this->rootFolder->getUserFolder($form->getOwnerId()); | ||||||
| $uploadedFilesFolderPath = $this->formsService->getFormUploadedFilesFolderPath($form); | ||||||
|
|
@@ -421,4 +467,22 @@ private function validatePermissions(array $permissions, int $shareType): bool { | |||||
| } | ||||||
| return true; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * @throws OCSBadRequestException If token does not satisfy basic safety checks | ||||||
| */ | ||||||
| private function validatePublicShareToken(string $token): void { | ||||||
| if ($token !== trim($token)) { | ||||||
| throw new OCSBadRequestException('Invalid share token'); | ||||||
| } | ||||||
|
|
||||||
| $tokenLength = strlen($token); | ||||||
| if ($tokenLength < Constants::PUBLIC_SHARE_TOKEN_MIN_LENGTH || $tokenLength > Constants::PUBLIC_SHARE_TOKEN_MAX_LENGTH) { | ||||||
| throw new OCSBadRequestException('Invalid share token'); | ||||||
| } | ||||||
|
|
||||||
| if (preg_match('/^[a-zA-Z0-9]+$/', $token) !== 1) { | ||||||
| throw new OCSBadRequestException('Invalid share token'); | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -37,7 +37,10 @@ public function getAllowPermitAll(): bool { | |||||||||||||
| public function getAllowPublicLink(): bool { | ||||||||||||||
| return json_decode($this->config->getAppValue($this->appName, Constants::CONFIG_KEY_ALLOWPUBLICLINK, 'true')); | ||||||||||||||
| } | ||||||||||||||
| public function getAllowShowToAll() : bool { | ||||||||||||||
| public function getAllowCustomPublicToken(): bool { | ||||||||||||||
| return json_decode($this->config->getAppValue($this->appName, Constants::CONFIG_KEY_ALLOWCUSTOMPUBLICTOKEN, 'false')); | ||||||||||||||
| } | ||||||||||||||
|
Comment on lines
+40
to
+42
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please use
Suggested change
|
||||||||||||||
| public function getAllowShowToAll(): bool { | ||||||||||||||
| return json_decode($this->config->getAppValue($this->appName, Constants::CONFIG_KEY_ALLOWSHOWTOALL, 'true')); | ||||||||||||||
| } | ||||||||||||||
| private function getUnformattedCreationAllowedGroups(): array { | ||||||||||||||
|
|
@@ -57,6 +60,7 @@ public function getAppConfig(): array { | |||||||||||||
| return [ | ||||||||||||||
| Constants::CONFIG_KEY_ALLOWPERMITALL => $this->getAllowPermitAll(), | ||||||||||||||
| Constants::CONFIG_KEY_ALLOWPUBLICLINK => $this->getAllowPublicLink(), | ||||||||||||||
| Constants::CONFIG_KEY_ALLOWCUSTOMPUBLICTOKEN => $this->getAllowCustomPublicToken(), | ||||||||||||||
| Constants::CONFIG_KEY_ALLOWSHOWTOALL => $this->getAllowShowToAll(), | ||||||||||||||
| Constants::CONFIG_KEY_CREATIONALLOWEDGROUPS => $this->getCreationAllowedGroups(), | ||||||||||||||
| Constants::CONFIG_KEY_RESTRICTCREATION => $this->getRestrictCreation(), | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -32,6 +32,13 @@ | |||||||||||||||||||||||||||||
| @update:modelValue="onAllowPublicLinkChange"> | ||||||||||||||||||||||||||||||
| {{ t('forms', 'Allow sharing by link') }} | ||||||||||||||||||||||||||||||
| </NcCheckboxRadioSwitch> | ||||||||||||||||||||||||||||||
| <NcCheckboxRadioSwitch | ||||||||||||||||||||||||||||||
| ref="switchAllowCustomPublicShareTokens" | ||||||||||||||||||||||||||||||
| v-model="appConfig.allowCustomPublicShareTokens" | ||||||||||||||||||||||||||||||
| type="switch" | ||||||||||||||||||||||||||||||
| @update:modelValue="onAllowCustomPublicShareTokensChange"> | ||||||||||||||||||||||||||||||
| {{ t('forms', 'Allow custom public share tokens') }} | ||||||||||||||||||||||||||||||
| </NcCheckboxRadioSwitch> | ||||||||||||||||||||||||||||||
|
Comment on lines
35
to
+41
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||||||||
| <NcCheckboxRadioSwitch | ||||||||||||||||||||||||||||||
| ref="switchAllowPermitAll" | ||||||||||||||||||||||||||||||
| v-model="appConfig.allowPermitAll" | ||||||||||||||||||||||||||||||
|
|
@@ -115,6 +122,13 @@ export default { | |||||||||||||||||||||||||||||
| el.loading = false | ||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| async onAllowCustomPublicShareTokensChange(newVal) { | ||||||||||||||||||||||||||||||
| const el = this.$refs.switchAllowCustomPublicShareTokens | ||||||||||||||||||||||||||||||
| el.loading = true | ||||||||||||||||||||||||||||||
| await this.saveAppConfig('allowCustomPublicShareTokens', newVal) | ||||||||||||||||||||||||||||||
| el.loading = false | ||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
Comment on lines
+125
to
131
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||||||||
| async onAllowPermitAllChange(newVal) { | ||||||||||||||||||||||||||||||
| const el = this.$refs.switchAllowPermitAll | ||||||||||||||||||||||||||||||
| el.loading = true | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be consistens with file sharing we should use
1here, but we need to add bruteforce protection on the PageController