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

feat: hide caldav server settings if no app uses the caldav backend #46510

Merged
merged 1 commit into from
Jul 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions apps/dav/lib/Settings/CalDAVSettings.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
namespace OCA\DAV\Settings;

use OCA\DAV\AppInfo\Application;
use OCP\App\IAppManager;
use OCP\AppFramework\Http\TemplateResponse;
use OCP\AppFramework\Services\IInitialState;
use OCP\IConfig;
Expand All @@ -21,6 +22,7 @@ class CalDAVSettings implements IDelegatedSettings {
private $initialState;

private IURLGenerator $urlGenerator;
private IAppManager $appManager;

private const defaults = [
'sendInvitations' => 'yes',
Expand All @@ -36,10 +38,11 @@ class CalDAVSettings implements IDelegatedSettings {
* @param IConfig $config
* @param IInitialState $initialState
*/
public function __construct(IConfig $config, IInitialState $initialState, IURLGenerator $urlGenerator) {
public function __construct(IConfig $config, IInitialState $initialState, IURLGenerator $urlGenerator, IAppManager $appManager) {
$this->config = $config;
$this->initialState = $initialState;
$this->urlGenerator = $urlGenerator;
$this->appManager = $appManager;
}

public function getForm(): TemplateResponse {
Expand All @@ -51,10 +54,11 @@ public function getForm(): TemplateResponse {
return new TemplateResponse(Application::APP_ID, 'settings-admin-caldav');
}

/**
* @return string
*/
public function getSection() {
public function getSection(): ?string {
if (!$this->appManager->isBackendRequired(IAppManager::BACKEND_CALDAV)) {
return null;
}

return 'groupware';
}

Expand Down
20 changes: 19 additions & 1 deletion apps/dav/tests/unit/Settings/CalDAVSettingsTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
namespace OCA\DAV\Tests\Unit\DAV\Settings;

use OCA\DAV\Settings\CalDAVSettings;
use OCP\App\IAppManager;
use OCP\AppFramework\Http\TemplateResponse;
use OCP\AppFramework\Services\IInitialState;
use OCP\IConfig;
Expand All @@ -24,6 +25,9 @@ class CalDAVSettingsTest extends TestCase {
/** @var IURLGenerator|MockObject */
private $urlGenerator;

/** @var IAppManager|MockObject */
private $appManager;

private CalDAVSettings $settings;

protected function setUp(): void {
Expand All @@ -32,7 +36,8 @@ protected function setUp(): void {
$this->config = $this->createMock(IConfig::class);
$this->initialState = $this->createMock(IInitialState::class);
$this->urlGenerator = $this->createMock(IURLGenerator::class);
$this->settings = new CalDAVSettings($this->config, $this->initialState, $this->urlGenerator);
$this->appManager = $this->createMock(IAppManager::class);
$this->settings = new CalDAVSettings($this->config, $this->initialState, $this->urlGenerator, $this->appManager);
}

public function testGetForm(): void {
Expand Down Expand Up @@ -65,10 +70,23 @@ public function testGetForm(): void {
}

public function testGetSection(): void {
$this->appManager->expects(self::once())
->method('isBackendRequired')
->with(IAppManager::BACKEND_CALDAV)
->willReturn(true);
$this->assertEquals('groupware', $this->settings->getSection());
}

public function testGetSectionWithoutCaldavBackend(): void {
$this->appManager->expects(self::once())
->method('isBackendRequired')
->with(IAppManager::BACKEND_CALDAV)
->willReturn(false);
$this->assertEquals(null, $this->settings->getSection());
}

public function testGetPriority(): void {
$this->assertEquals(10, $this->settings->getPriority());
}

}
12 changes: 12 additions & 0 deletions lib/private/App/AppManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -875,4 +875,16 @@ public function setDefaultApps(array $defaultApps): void {

$this->config->setSystemValue('defaultapp', join(',', $defaultApps));
}

public function isBackendRequired(string $backend): bool {
foreach ($this->appInfos as $appInfo) {
foreach ($appInfo['dependencies']['backend'] as $appBackend) {
if ($backend === $appBackend) {
return true;
}
}
}

return false;
}
}
10 changes: 9 additions & 1 deletion lib/private/App/InfoParser.php
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
}

libxml_use_internal_errors(true);
$xml = simplexml_load_string(file_get_contents($file));

Check failure on line 38 in lib/private/App/InfoParser.php

View workflow job for this annotation

GitHub Actions / static-code-analysis-security

TaintedFile

lib/private/App/InfoParser.php:38:50: TaintedFile: Detected tainted file handling (see https://psalm.dev/255)

if ($xml === false) {
libxml_clear_errors();
Expand Down Expand Up @@ -113,6 +113,12 @@
if (!array_key_exists('personal-section', $array['settings'])) {
$array['settings']['personal-section'] = [];
}
if (!array_key_exists('dependencies', $array)) {
$array['dependencies'] = [];
}
if (!array_key_exists('backend', $array['dependencies'])) {
$array['dependencies']['backend'] = [];
}

if (array_key_exists('types', $array)) {
if (is_array($array['types'])) {
Expand Down Expand Up @@ -177,10 +183,12 @@
if (isset($array['settings']['personal-section']) && !is_array($array['settings']['personal-section'])) {
$array['settings']['personal-section'] = [$array['settings']['personal-section']];
}

if (isset($array['navigations']['navigation']) && $this->isNavigationItem($array['navigations']['navigation'])) {
$array['navigations']['navigation'] = [$array['navigations']['navigation']];
}
if (isset($array['dependencies']['backend']) && !is_array($array['dependencies']['backend'])) {
$array['dependencies']['backend'] = [$array['dependencies']['backend']];
}

if ($this->cache !== null) {
$this->cache->set($fileCacheKey, json_encode($array));
Expand Down
15 changes: 15 additions & 0 deletions lib/public/App/IAppManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,11 @@
* @since 8.0.0
*/
interface IAppManager {
/**
* @since 30.0.0
*/
public const BACKEND_CALDAV = 'caldav';

/**
* Returns the app information from "appinfo/info.xml".
*
Expand Down Expand Up @@ -261,4 +266,14 @@ public function getDefaultApps(): array;
* @since 28.0.0
*/
public function setDefaultApps(array $defaultApps): void;

/**
* Check whether the given backend is required by at least one app.
*
* @param self::BACKEND_* $backend Name of the backend, one of `self::BACKEND_*`
* @return bool True if at least one app requires the backend
*
* @since 30.0.0
*/
public function isBackendRequired(string $backend): bool;
Copy link
Member

Choose a reason for hiding this comment

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

}
7 changes: 7 additions & 0 deletions resources/app-info-shipped.xsd
Original file line number Diff line number Diff line change
Expand Up @@ -575,6 +575,8 @@
maxOccurs="1"/>
<xs:element name="architecture" type="architecture" minOccurs="0"
maxOccurs="unbounded"/>
<xs:element name="backend" type="backend" minOccurs="0"
maxOccurs="unbounded"/>
</xs:sequence>
</xs:complexType>

Expand Down Expand Up @@ -757,4 +759,9 @@
</xs:restriction>
</xs:simpleType>

<xs:simpleType name="backend">
<xs:restriction base="xs:string">
<xs:enumeration value="caldav"/>
</xs:restriction>
</xs:simpleType>
</xs:schema>
8 changes: 8 additions & 0 deletions resources/app-info.xsd
Copy link
Member

Choose a reason for hiding this comment

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

It's a bit problematic that we don't have CI in place to confirm this, but https://github.com/nextcloud/appstore/blob/master/nextcloudappstore/api/v1/release/info.xsd needs to be kept in sync.

Copy link
Member

Choose a reason for hiding this comment

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

Original file line number Diff line number Diff line change
Expand Up @@ -563,6 +563,8 @@
maxOccurs="1"/>
<xs:element name="architecture" type="architecture" minOccurs="0"
maxOccurs="unbounded"/>
<xs:element name="backend" type="backend" minOccurs="0"
maxOccurs="unbounded"/>
</xs:sequence>
</xs:complexType>

Expand Down Expand Up @@ -737,4 +739,10 @@
value="[a-zA-Z_][0-9a-zA-Z_]*(\\[a-zA-Z_][0-9a-zA-Z_]*)*"/>
</xs:restriction>
</xs:simpleType>

<xs:simpleType name="backend">
<xs:restriction base="xs:string">
<xs:enumeration value="caldav"/>
</xs:restriction>
</xs:simpleType>
</xs:schema>
5 changes: 4 additions & 1 deletion tests/data/app/expected-info.json
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,10 @@
"min-version": "7.0.1",
"max-version": "8"
}
}
},
"backend": [
"caldav"
]
},
"repair-steps": {
"install": [],
Expand Down
5 changes: 3 additions & 2 deletions tests/data/app/navigation-one-item.json
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@
"min-version": "16",
"max-version": "16"
}
}
},
"backend": []
},
"background-jobs": [
"OCA\\Activity\\BackgroundJob\\EmailNotification",
Expand Down Expand Up @@ -82,4 +83,4 @@
"uninstall": []
},
"two-factor-providers": []
}
}
5 changes: 3 additions & 2 deletions tests/data/app/navigation-two-items.json
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@
"min-version": "16",
"max-version": "16"
}
}
},
"backend": []
},
"background-jobs": [
"OCA\\Activity\\BackgroundJob\\EmailNotification",
Expand Down Expand Up @@ -88,4 +89,4 @@
"uninstall": []
},
"two-factor-providers": []
}
}
1 change: 1 addition & 0 deletions tests/data/app/valid-info.xml
Original file line number Diff line number Diff line change
Expand Up @@ -34,5 +34,6 @@
<lib>curl</lib>
<os>Linux</os>
<owncloud min-version="7.0.1" max-version="8" />
<backend>caldav</backend>
</dependencies>
</info>
48 changes: 48 additions & 0 deletions tests/lib/App/AppManagerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -885,4 +885,52 @@ public function testGetDefaultAppForUser($defaultApps, $userDefaultApps, $userAp

$this->assertEquals($expectedApp, $this->manager->getDefaultAppForUser(null, $withFallbacks));
}

public static function isBackendRequiredDataProvider(): array {
return [
// backend available
[
'caldav',
['app1' => ['caldav']],
true,
],
[
'caldav',
['app1' => [], 'app2' => ['foo'], 'app3' => ['caldav']],
true,
],
// backend not available
[
'caldav',
['app3' => [], 'app1' => ['foo'], 'app2' => ['bar', 'baz']],
false,
],
// no app available
[
'caldav',
[],
false,
],
];
}

/**
* @dataProvider isBackendRequiredDataProvider
*/
public function testIsBackendRequired(
string $backend,
array $appBackends,
bool $expected,
): void {
$appInfoData = array_map(
static fn (array $backends) => ['dependencies' => ['backend' => $backends]],
$appBackends,
);

$reflection = new \ReflectionClass($this->manager);
$property = $reflection->getProperty('appInfos');
$property->setValue($this->manager, $appInfoData);

$this->assertEquals($expected, $this->manager->isBackendRequired($backend));
}
}
Loading