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.

Sign up for GitHub

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

Jump to bottom

fix(caldav): Do not load IMipPlugin before user auth and session is c… #45098

Merged
merged 5 commits into from
May 4, 2024
Merged

fix(caldav): Do not load IMipPlugin before user auth and session is c… #45098

merged 5 commits into from
May 4, 2024

Conversation

SebastianKrupinski
Copy link
Contributor

Summary

Do not load IMipPlugin before user is authenticated and user session is initilized

github-advanced-security[bot]
github-advanced-security bot found potential problems Apr 29, 2024
View reviewed changes
apps/dav/lib/Server.php
@@ -176,12 +177,10 @@

// calendar plugins
if ($this->requestIsForSubtree(['calendars', 'public-calendars', 'system-calendars', 'principals'])) {
$this->server->addPlugin(new DAV\Sharing\Plugin($authBackend, \OC::$server->getRequest(), \OC::$server->getConfig()));

Check notice

Code scanning / Psalm

DeprecatedMethod Note

The method OC\Server::getRequest has been marked as deprecated
apps/dav/lib/Server.php
@@ -176,12 +177,10 @@

// calendar plugins
if ($this->requestIsForSubtree(['calendars', 'public-calendars', 'system-calendars', 'principals'])) {
$this->server->addPlugin(new DAV\Sharing\Plugin($authBackend, \OC::$server->getRequest(), \OC::$server->getConfig()));

Check notice

Code scanning / Psalm

DeprecatedMethod Note

The method OC\Server::getConfig has been marked as deprecated
apps/dav/lib/Server.php
@@ -304,6 +302,19 @@
\OC::$server->getCommentsManager(),
$userSession
));
if (\OC::$server->getConfig()->getAppValue('dav', 'sendInvitations', 'yes') === 'yes') {

Check notice

Code scanning / Psalm

DeprecatedMethod Note

The method OC\Server::getConfig has been marked as deprecated
apps/dav/lib/Server.php
@@ -304,6 +302,19 @@
\OC::$server->getCommentsManager(),
$userSession
));
if (\OC::$server->getConfig()->getAppValue('dav', 'sendInvitations', 'yes') === 'yes') {

Check notice

Code scanning / Psalm

DeprecatedMethod Note

The method OCP\IConfig::getAppValue has been marked as deprecated
apps/dav/lib/Server.php Fixed Show fixed Hide fixed
apps/dav/lib/Server.php Fixed Show fixed Hide fixed
github-advanced-security[bot]
github-advanced-security bot found potential problems Apr 30, 2024
View reviewed changes
apps/dav/lib/CalDAV/Schedule/IMipPlugin.php
$senderName = $senderName->getValue() ?? null;
// Due to a bug in sabre, the senderName property for an iTIP message can actually also be a VObject Property
// If the iTIP message senderName is null or empty use the user session name as the senderName
if (($iTipMessage->senderName instanceof Parameter) && !empty(trim($iTipMessage->senderName->getValue()))) {

Check notice

Code scanning / Psalm

PossiblyNullArgument Note

Argument 1 of trim cannot be null, possibly null value provided
apps/dav/lib/CalDAV/Schedule/IMipPlugin.php Fixed Show fixed Hide fixed
tcitworld
tcitworld reviewed Apr 30, 2024
View reviewed changes
apps/dav/lib/CalDAV/Schedule/IMipPlugin.php Outdated
$senderName = $iTipMessage->senderName;
}
else {
$senderName = $this->userSession->getUser()->getDisplayName();
Copy link
Member

Choose a reason for hiding this comment

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

Might be an opportunity to trim $senderName afterwards for all cases.

apps/dav/lib/CalDAV/Schedule/IMipPlugin.php Outdated
$senderName = $iTipMessage->senderName;
}
else {
$senderName = $this->userSession->getUser()->getDisplayName();
Copy link
Member

Choose a reason for hiding this comment

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

Even though this should be safe when registered through dav server, it might not be when used through apps/dav/appinfo/v1/caldav.php (very legacy stuff, but still). I'd add a check to see if getUser() isn't null (and psalm would be happy).

@SebastianKrupinski
Copy link
Contributor Author

@tcitworld thank you for the input. I will apply your recommendations.

@tcitworld
Copy link
Member

OCA\DAV\Tests\unit\CalDAV\Schedule\IMipPluginTest needs to be changed with the new constructor.

tcitworld
tcitworld approved these changes May 3, 2024
View reviewed changes
Copy link
Member

@tcitworld tcitworld left a comment

Choose a reason for hiding this comment

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

Lint fix, otherwise ✅

github-advanced-security[bot]
github-advanced-security bot found potential problems May 3, 2024
View reviewed changes
apps/dav/lib/CalDAV/Schedule/IMipPlugin.php
// Due to a bug in sabre, the senderName property for an iTIP message can actually also be a VObject Property
// If the iTIP message senderName is null or empty use the user session name as the senderName
if (($iTipMessage->senderName instanceof Parameter) && !empty(trim($iTipMessage->senderName->getValue()))) {
$senderName = trim($iTipMessage->senderName->getValue());

Check notice

Code scanning / Psalm

PossiblyNullArgument Note

Argument 1 of trim cannot be null, possibly null value provided
apps/dav/lib/CalDAV/Schedule/IMipPlugin.php
} elseif (is_string($iTipMessage->senderName) && !empty(trim($iTipMessage->senderName))) {
$senderName = trim($iTipMessage->senderName);
} elseif ($this->userSession->getUser() !== null) {
$senderName = trim($this->userSession->getUser()->getDisplayName());

Check notice

Code scanning / Psalm

PossiblyNullReference Note

Cannot call method getDisplayName on possibly null value
@SebastianKrupinski SebastianKrupinski merged commit c17fcc4 into master May 4, 2024
157 checks passed
@SebastianKrupinski SebastianKrupinski deleted the fix/issue-45081 branch May 4, 2024 13:36
@welcome Welcome
Copy link

welcome bot commented May 4, 2024

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

@SebastianKrupinski SebastianKrupinski added this to the Nextcloud 30 milestone Jul 23, 2024
@blizzz blizzz mentioned this pull request Jul 24, 2024
Merged
@susnux
Copy link
Contributor

susnux commented Sep 2, 2024

@SebastianKrupinski could we backport this to stable29 ?

@SebastianKrupinski SebastianKrupinski mentioned this pull request Sep 2, 2024
Closed
5 tasks
@SebastianKrupinski
Copy link
Contributor Author

@SebastianKrupinski could we backport this to stable29 ?

I have no issue back porting it... Let me just ask if there is any reason we shoudn't

@ChristophWurst
Copy link
Member

Go :shipit:

@SebastianKrupinski
Copy link
Contributor Author

/backport to stable29

@SebastianKrupinski
Copy link
Contributor Author

/backport to stable28

@nextcloud nextcloud deleted a comment from backportbot bot Sep 2, 2024
@nextcloud nextcloud deleted a comment from backportbot bot Sep 2, 2024
@SebastianKrupinski
Copy link
Contributor Author

Backport for 29 #47790

Backport for 28 #47791

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

@tcitworld tcitworld tcitworld approved these changes

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@miaulalala miaulalala Awaiting requested review from miaulalala

Assignees
No one assigned
Labels
backport-request
Projects
None yet
Milestone
Nextcloud 30
Development

Successfully merging this pull request may close these issues.

[Bug]: From Name is not setting properly in Calendar invitations created from Thunderbird
4 participants
@SebastianKrupinski @tcitworld @susnux @ChristophWurst