Skip to content

Hash event UID to make sure it's not too long for PushProvider notifications - #19252

Merged
nickvergessen merged 1 commit into
masterfrom
bug/fix-notification-object-id
Feb 25, 2020
Merged

Hash event UID to make sure it's not too long for PushProvider notifications#19252
nickvergessen merged 1 commit into
masterfrom
bug/fix-notification-object-id

Conversation

@tcitworld

@tcitworldtcitworld commented Feb 1, 2020

Copy link
Copy Markdown
Member

Notifications object ID is limited to 64 characters, event UID is not. A Sha256 hash is limited to 64 characters.

@tcitworld
tcitworldforce-pushed the bug/fix-notification-object-id branch from 782bfa3 to 79ed9fcCompareFebruary 1, 2020 14:43
@kesselb

kesselb commented Feb 1, 2020

Copy link
Copy Markdown
Contributor

Should fix #18688 and #17758?

@tcitworld

tcitworld commented Feb 1, 2020

Copy link
Copy Markdown
MemberAuthor

At least #17758, not sure about #18688

@tcitworld
Hash event UID to make sure it's not too long for PushProvider
41fa057
notifications
Signed-off-by: Thomas Citharel <tcit@tcit.fr>
@tcitworld
tcitworldforce-pushed the bug/fix-notification-object-id branch from 79ed9fc to 41fa057CompareFebruary 1, 2020 17:14
@tcitworldtcitworld added this to the Nextcloud 19 milestone Feb 5, 2020
@tcitworld

tcitworld commented Feb 5, 2020

Copy link
Copy Markdown
MemberAuthor

/backport to stable18

@tcitworld

tcitworld commented Feb 5, 2020

Copy link
Copy Markdown
MemberAuthor

/backport to stable17

rullzer
rullzer reviewed Feb 6, 2020
View reviewed changes
Comment threadapps/dav/lib/CalDAV/Reminder/NotificationProvider/PushProvider.php
$eventDetails['calendar_displayname'] = $calendarDisplayName;
$eventUUID = (string) $vevent->UID;
// Empty Notification ObjectId will be catched by OC\Notification\Notification
$eventUUIDHash = $eventUUID ? hash('sha256', $eventUUID, false) : '';

@rullzerrullzerFeb 6, 2020

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.

just curious. By why sha256?

@kesselbkesselbFeb 6, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Notifications object ID is limited to 64 characters, event UID is not. A Sha256 hash is limited to 64 characters.

Seems to be the reason.

@rullzerrullzerFeb 6, 2020

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.

right. fair enough

@tcitworldtcitworldFeb 6, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

And as it's happening through a background task the performance difference shouldn't be an issue.

@nickvergessen

nickvergessen commented Feb 6, 2020

Copy link
Copy Markdown
Member

THe thing is, that basically means we need to be able to resolve sha256 in the future for direct links to calendar events
// cc @georgehrke

@tcitworld

tcitworld commented Feb 6, 2020•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

THe thing is, that basically means we need to be able to resolve sha256 in the future for direct links to calendar events

@nickvergessen Wouldn't we use setLink in that case ? Then the link is already constructed before the notification is rendered.

@szaimen

szaimen commented Feb 22, 2020

Copy link
Copy Markdown
Contributor

Any progres here? :)

@nickvergessen

nickvergessen commented Feb 25, 2020

Copy link
Copy Markdown
Member

Wouldn't we use setLink in that case ? Then the link is already constructed before the notification is rendered.

link field is also limited to 4k chars. anyway. In worst case the calendar app has to do a mapping: "if event not found and uid.length === 64 check somewhere else for a reverse match"

@nickvergessen

nickvergessen commented Feb 25, 2020

Copy link
Copy Markdown
Member

/backport to stable18

@nickvergessen

nickvergessen commented Feb 25, 2020

Copy link
Copy Markdown
Member

/backport to stable17

@nickvergessen
nickvergessen merged commit e620673 into masterFeb 25, 2020
@nickvergessen
nickvergessen deleted the bug/fix-notification-object-id branch February 25, 2020 16:04
@backportbot-nextcloudbackportbot-nextcloudBot mentioned this pull request Feb 25, 2020
Merged
@backportbot-nextcloud

backportbot-nextcloudBot commented Feb 25, 2020

Copy link
Copy Markdown

backport to stable18 in #19639

@backportbot-nextcloudbackportbot-nextcloudBot mentioned this pull request Feb 25, 2020
Merged
@backportbot-nextcloud

backportbot-nextcloudBot commented Feb 25, 2020

Copy link
Copy Markdown

backport to stable17 in #19640

@tcitworldtcitworld mentioned this pull request Mar 27, 2020
Closed
@kesselbkesselb mentioned this pull request Jan 15, 2021
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kesselbkesselbkesselb left review comments
@rullzerrullzerrullzer approved these changes
@nickvergessennickvergessennickvergessen approved these changes
@georgehrkegeorgehrkeAwaiting requested review from georgehrke

Assignees

No one assigned

Labels

3. to reviewWaiting for reviewsbugfeature: dav

Projects

None yet

Milestone

Nextcloud 19

Development

Successfully merging this pull request may close these issues.

6 participants

@tcitworld@kesselb@nickvergessen@szaimen@rullzer@MorrisJobke