Skip to content
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

fix(calendar): Fix getting the permissions of the user #49731

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nickvergessen
Copy link
Member

Checklist

@nickvergessen nickvergessen added this to the Nextcloud 31 milestone Dec 9, 2024
@nickvergessen nickvergessen added bug 3. to review Waiting for reviews feature: caldav Related to CalDAV internals labels Dec 9, 2024
@nickvergessen nickvergessen self-assigned this Dec 9, 2024
@nickvergessen nickvergessen added the pending documentation This pull request needs an associated documentation update label Dec 9, 2024
*/
public function getPermissions(): int;
public function getPermissions(?string $principal = null): int;
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to open this box? I assume getkey, getUri, getDisplayName and getDiplayColor will have the same problem, as the values may be different for the sharer and the sharees? 🙊

Copy link
Member Author

@nickvergessen nickvergessen Dec 9, 2024

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I get that we need to fix it. I'm just wondering if we should take a different approach. If you got the calendar objects through \OCP\Calendar\IManager::getCalendarsForPrincipal I'd expect the permissions to already be specific to the principal passed there, and not having the need to specify the principal another time later on.

Copy link
Contributor

@miaulalala miaulalala Dec 9, 2024

Choose a reason for hiding this comment

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

You could add a new method on the IManager that only returns writable calendars, and filter directly in this method.

Copy link
Member Author

Choose a reason for hiding this comment

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

If you got the calendar objects through \OCP\Calendar\IManager::getCalendarsForPrincipal I'd expect the permissions to already be specific to the principal passed there

Also makes sense. Should we add a new method which does that, or add a new method which has the current (unexpected) behaviour, or not have the old behaviour anymore at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

You could add a new method on the IManager that only returns writable calendars, and filter directly in this method.

If getPermissions is correct this should not be needed anymore as the OCP consumer can easily filter.

Copy link
Member Author

@nickvergessen nickvergessen Dec 11, 2024

Choose a reason for hiding this comment

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

So, fixed the permissions. ✅

For the color and the name, I would do a DB query in CalendarProvider::getCalendars against the oc_properties table, in lack of another good abstraction (I tried OCA\DAV\DAV\CustomPropertiesBackend but that requires a PropFind object, which is failing to generate in OCS requests with Could not resolve Sabre\DAV\ICollection! Class can not be instantiated).

Or does someone have better ideas that can be implemented until end of the week?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you get the calendars via the IManager, isn't that (color, displayName) already contained within the calendarInfo?

Copy link
Member

Choose a reason for hiding this comment

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

Color and name need properties lookup. Let's tackle that separately when we need it.

@nickvergessen nickvergessen force-pushed the bugfix/noid/allow-to-get-permissions-of-a-principal branch from be795ab to 4fd84e4 Compare December 11, 2024 07:40
@nickvergessen nickvergessen changed the title fix(calendar): Allow to get the permissions of a dedicated user fix(calendar): Fix getting the permissions of the user Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug feature: caldav Related to CalDAV internals pending documentation This pull request needs an associated documentation update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Schedule meetings inside Talk
3 participants