Skip to content

feat(share): add public share-review API under OCP\Share\ShareReview#61543

Open
AndyScherzinger wants to merge 5 commits into
masterfrom
feat/noid/shareReviewDeleteEvent
Open

feat(share): add public share-review API under OCP\Share\ShareReview#61543
AndyScherzinger wants to merge 5 commits into
masterfrom
feat/noid/shareReviewDeleteEvent

Conversation

@AndyScherzinger

@AndyScherzinger AndyScherzinger commented Jun 23, 2026

Copy link
Copy Markdown
Member

Summary

Introduces OCP\Share\Events\ShareReviewAccessCheckEvent as the canonical authorization gate event for ShareReview sources. The event carries the source name and share ID for listener context, implements deny-wins semantics, and stops propagation immediately on denial.
This is a discussed and agreed on measure to safeguard auditing-triggered share deletions throughout various apps using a "check back event mechanism", so apps can implement their support for this (any app that has shares basically, of any kind)

Docs at nextcloud/documentation#15223

Assisted-by: Claude Code:claude-sonnet-4-6
and
Assisted-by: Claude Code:claude-fable-5

TODO

Checklist

AI (if applicable)

  • The content of this PR was partly or fully generated using AI

@AndyScherzinger AndyScherzinger added this to the Nextcloud 35 milestone Jun 23, 2026
@AndyScherzinger AndyScherzinger requested a review from a team as a code owner June 23, 2026 14:45
@AndyScherzinger AndyScherzinger requested review from Altahrim and ArtificialOwl and removed request for a team June 23, 2026 14:45
@AndyScherzinger AndyScherzinger added the 2. developing Work in progress label Jun 23, 2026
@AndyScherzinger AndyScherzinger marked this pull request as draft June 23, 2026 14:48
@AndyScherzinger

Copy link
Copy Markdown
Member Author

/backport to stable34

@CarlSchwan CarlSchwan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to run build/autoloaderchecker.sh

Comment thread lib/public/Share/ShareReview/Events/ShareReviewAccessCheckEvent.php
@AndyScherzinger AndyScherzinger force-pushed the feat/noid/shareReviewDeleteEvent branch 6 times, most recently from c4ea0a9 to af59b39 Compare June 23, 2026 17:03
@AndyScherzinger AndyScherzinger marked this pull request as ready for review June 23, 2026 17:44
@AndyScherzinger AndyScherzinger added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jun 25, 2026
Comment thread lib/public/Share/ShareReview/Events/ShareReviewAccessCheckEvent.php Outdated
@AndyScherzinger AndyScherzinger force-pushed the feat/noid/shareReviewDeleteEvent branch 2 times, most recently from f306220 to d45d8e1 Compare July 2, 2026 21:42
@AndyScherzinger

Copy link
Copy Markdown
Member Author

@provokateurin @CarlSchwan worked on addressing your review comments.

@AndyScherzinger AndyScherzinger changed the title feat(share): add ShareReviewAccessCheckEvent to OCP public API feat(share): add public share-review API under OCP\Share\ShareReview Jul 2, 2026
@AndyScherzinger AndyScherzinger force-pushed the feat/noid/shareReviewDeleteEvent branch from d45d8e1 to e46b015 Compare July 3, 2026 16:59
@AndyScherzinger AndyScherzinger requested a review from susnux July 6, 2026 18:17

@provokateurin provokateurin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the explanation to the event docs.

Comment thread lib/public/Share/ShareReview/IShareReviewSource.php Outdated
Comment thread lib/public/Share/ShareReview/IShareReviewSource.php Outdated
Comment thread tests/lib/Share20/ShareReview/RegisterShareReviewSourceEventTest.php Outdated
Introduces OCP\Share\Events\ShareReviewAccessCheckEvent as the canonical
authorization gate event for ShareReview sources. The event carries the
source name and share ID for listener context, implements deny-wins
semantics, and stops propagation immediately on denial.

Assisted-by: ClaudeCode:claude-sonnet-4-6
Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
…re\ShareReview

Assisted-by: Claude Code:claude-fable-5
Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
…ssCheckEvent

Assisted-by: Claude Code:claude-fable-5
Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
Assisted-by: ClaudeCode:claude-fable-5
Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
Assisted-by: ClaudeCode:claude-fable-5
Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
@AndyScherzinger AndyScherzinger force-pushed the feat/noid/shareReviewDeleteEvent branch from e46b015 to fa57f02 Compare July 7, 2026 12:51
Comment on lines +49 to +51
public readonly string $time = '1970-01-01 01:00:00',
public readonly string $action = '',
public readonly ?int $timestamp = null,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should not be two fields for the creation time.

Also their names are not descriptive enough, they should be called creationTime/creationTimestamp and one has a default value and the other doesn't.

If you keep it like this, you can expect that they will be used incorrectly, because they're confusing 🙈

public readonly int $type,
public readonly string $recipient,
public readonly int $permissions = 1,
public readonly string $time = '1970-01-01 01:00:00',

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a proper ISO8601 string.

* @param string $object Name or title of the shared object, such as a
* file path or report name.
* @param string $initiator User ID of the initiator.
* @param int $type {@see \OCP\Share\IShare} type of the share.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @param int $type {@see \OCP\Share\IShare} type of the share.
* @param IShare::TYPE_* $type {@see \OCP\Share\IShare} type of the share.

* @param string $initiator User ID of the initiator.
* @param int $type {@see \OCP\Share\IShare} type of the share.
* @param string $recipient User ID of the owner or the token of a link.
* @param int $permissions Permissions level of the share.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @param int $permissions Permissions level of the share.
* @param int-mask-of<Constants::PERMISSION_*> $permissions Permissions level of the share.

public readonly string $time = '1970-01-01 01:00:00',
public readonly string $action = '',
public readonly ?int $timestamp = null,
public readonly bool $password = false,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public readonly bool $password = false,
public readonly bool $hasPassword = false,

* @param int $type {@see \OCP\Share\IShare} type of the share.
* @param string $recipient User ID of the owner or the token of a link.
* @param int $permissions Permissions level of the share.
* @param string $time Creation time of the share.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @param string $time Creation time of the share.
* @param string $creationTime Creation time of the share as ISO8601 string.

* @param int|null $timestamp Optional creation Unix timestamp, used for sorting.
* @param bool $password Whether the share is password protected. Never
* the password itself.
* @param string|null $expiration Optional expiration date displayed for the share.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @param string|null $expiration Optional expiration date displayed for the share.
* @param string|null $expirationDate Optional expiration date displayed for the share as ISO8601 string.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants