Use new appstore API #1940

LukasReschke · 2016-10-27T15:49:12Z

This change introduces the new appstore API in Nextcloud.

Todo

App updates

Signed-off-by: Lukas Reschke [email protected]

mention-bot · 2016-10-27T15:49:36Z

@LukasReschke, thanks for your PR! By analyzing the history of the files in this pull request, we identified @DeepDiver1975, @nickvergessen and @rullzer to be potential reviewers.

LukasReschke · 2016-10-27T15:50:04Z

cc @BernhardPosselt @rullzer

BernhardPosselt · 2016-10-27T18:29:34Z

config/config.sample.php

@@ -674,20 +674,6 @@
 'appstoreenabled' => true,

 /**
- * The URL of the appstore to use.
- */
-'appstoreurl' => 'https://api.owncloud.com/v1',


Shouldn't this still be configurable?

I'm not entirely sure on that. So the problem is there are a ton of people out there who actually have no idea what they are doing™️ and just copy the sample config file. I don't really look forward to have around 30 people asking why their appstore doesn't work anymore and because nobody fills out the issue template that's just a waste of time.

So we either have to rename that variable (suggestions welcome) or go with hard-coded for now. I'd be fine with hard-coded for the moment, especially since you also can't easily replace the certificate use for verification. (neither one could before)

Good point about renaming the value, that would relieve us from the need to migrate existing overrides. It's still probably better to keep it configurable for maintainability and flexibility purposes. As for certificate locations: should probably also be configurable but can be added later on.

I need to think about that a bit more. Especially since we also now have a proxy in place which would also be another config value.

BernhardPosselt · 2016-10-27T18:31:09Z

lib/private/App/AppStore/Fetcher/AppFetcher.php

+		$this->fileName = 'apps.json';
+		$this->endpointUrl = sprintf(
+			'https://apps.nextcloud.com/api/v1/platform/%s/apps.json',
+			substr(implode(\OC_Util::getVersion(), '.'), 0, 5)


why substr? looks dangerous

Yeah, pretty much, also this needs at least a comment

OC_Util::getVersion returns the version from config.php such as array(9, 2, 0, 4). But the appstore only takes 3 numbers and ignores the internal patch level.

That said, will add a comment. It's also tested at https://github.com/nextcloud/server/blob/ea32ef300c214c5af4a78f2650801bb856a03395/tests/lib/App/AppStore/Fetcher/AppFetcherTest.php

yeah, but substr(..., 0,5) will break with 11.0.0 which is 0,6 ...
and so on...

Another thing just fyi: you need to pass Nextcloud versions, not ownCloud internal versions. Using 9.2.0 won't yield any compatible apps

Another thing just fyi: you need to pass Nextcloud versions, not ownCloud internal versions. Using 9.2.0 won't yield any compatible apps

Well… It does 🙈

=> https://apps.nextcloud.com/api/v1/platform/9.2.0/apps.json

Also we'll likely just bump that to 11 for 11 so that it then would request the right URL :)

yeah, but substr(..., 0,5) will break with 11.0.0 which is 0,6 ....

True. So let's just read the array elements on it's own.

BernhardPosselt · 2016-10-27T18:34:11Z

lib/private/App/AppStore/Version/VersionParser.php

+					return new Version(substr($matches[0][0], 2), substr($matches[0][1], 2));
+					break;
+				default:
+					throw new \Exception('Version cannot be parsed');


Should probably include the version string that can not be parsed

BernhardPosselt · 2016-10-27T18:36:23Z

lib/private/App/AppStore/Version/VersionParser.php

+
+		// Count the amount of =, if it is one then it's either maximum or minimum
+		// version. If it is two then it is maximum and minimum.
+		if (preg_match_all('/(?:>|<)(?:=|)[0-9.]+/', $versionSpec, $matches)) {


Using a regex to parse a simple version string is maybe a bit overkill. explode() and strpos() may be faster and easier to read

Good point.

BernhardPosselt · 2016-10-27T18:38:00Z

lib/private/Installer.php

+				$x509 = new X509();
+				$x509->loadCA(file_get_contents(__DIR__ . '/../../resources/codesigning/root.crt'));
+				$x509->loadX509($app['certificate']);
+				if($x509->validateSignature() !== true) {


I suppose CRL support will be added later on :)

Actually let's just add that here as well and add tests for this.

BernhardPosselt · 2016-10-27T18:38:49Z

lib/private/Installer.php

+							'app' => 'core',
+						]
+					);
+					return false;


Any reason for this? Should probably throw an exception with your logging string

Same applies to the following lines

BernhardPosselt · 2016-10-27T18:41:58Z

lib/private/legacy/app.php

+				new \OC\App\AppStore\Fetcher\AppFetcher(
+					\OC::$server->getAppDataDir('appstore'),
+					\OC::$server->getHTTPClientService(),
+					new \OC\AppFramework\Utility\TimeFactory(),


You can just query the class from the server e.g. $server->query(\OC\AppFramework\Utility\TimeFactory::class)

Naive question: Any advantage of doing so?

If things change your code won't break

Would go for the public interface however :D

Querying the interface doesn't seem to work. But the other thing works. Replacing.

BernhardPosselt · 2016-10-27T18:43:12Z

settings/Controller/AppSettingsController.php

 		$params['category'] = $category;
 		$params['appstoreEnabled'] = $this->config->getSystemValue('appstoreenabled', true) === true;
 		$this->navigationManager->setActiveEntry('core_apps');

 		$templateResponse = new TemplateResponse($this->appName, 'apps', $params, 'user');
 		$policy = new ContentSecurityPolicy();
-		$policy->addAllowedImageDomain('https://apps.owncloud.com');
+		$policy->addAllowedImageDomain('*');


You can actually use https instead of *

Will also use an image proxy. So we can limit even further. Just waiting for the domain :)

Ref #1939

nickvergessen · 2016-10-28T08:36:16Z

lib/private/App/AppStore/Fetcher/AppFetcher.php

 		$this->endpointUrl = sprintf(
-			'https://apps.nextcloud.com/api/v1/platform/%s/apps.json',
-			substr(implode(\OC_Util::getVersion(), '.'), 0, 5)
+			'https://apps.nextcloud.com/api/v1/platform/%s.%s.%s/apps.json',


%d since it's only numbers? 🙊

This change introduces the new appstore API in Nextcloud. Signed-off-by: Lukas Reschke <[email protected]>

Signed-off-by: Lukas Reschke <[email protected]>

Otherwise this would break with 11.0.0 Signed-off-by: Lukas Reschke <[email protected]>

Signed-off-by: Lukas Reschke <[email protected]>

@nickvergessen

These are only numbers. THX @nickvergessen Signed-off-by: Lukas Reschke <[email protected]>

Signed-off-by: Lukas Reschke <[email protected]>

The other function doesn't work if the appstore is disabled Signed-off-by: Lukas Reschke <[email protected]>

LukasReschke · 2016-10-31T17:15:48Z

Ready for review. Will also add some more tests.

cc @rullzer @nickvergessen

The static version is used nowhere in the code and just decreases coverage Signed-off-by: Lukas Reschke <[email protected]>

Signed-off-by: Lukas Reschke <[email protected]>

rullzer · 2016-11-01T10:17:40Z

Ok so. I have been playing with this for a while now and haven't hit weird errors yet. I'd say lets merge 👍

nickvergessen · 2016-11-02T09:22:04Z

lib/private/legacy/app.php

 	 * @param array $groups (optional) when set, only these groups will have access to the app
 	 * @throws \Exception
 	 * @return void
 	 *
 	 * This function set an app as enabled in appconfig.
 	 */
-	public static function enable($app, $groups = null) {
+	public function enable($appId,


Is this breaking change really required?
Can't you just make that change in the AppManager and leave the old static stuff as a wrapper?

Well. This is private API and the usages in core have been adjusted. If someone relies on private API then they are to blame themselves. I'm not a fan of limiting myself just because some other people may do it majorly wrong. ;)

Also \OC\App\AppManager does a ton of different stuff not related to the appstore at all… I don't really want to pull that into that class at the moment as well. (also since it's public API and thus the expectation is that it will work nearly forever)

stp-ip · 2016-11-02T16:58:21Z

lgtm

* experimental apps where removed with 32cf661 (#1940) * fixes #2183 Signed-off-by: Morris Jobke <[email protected]>

Left over from a refactoring #1940 Signed-off-by: Morris Jobke <[email protected]>

LukasReschke added the 2. developing Work in progress label Oct 27, 2016

LukasReschke added this to the Nextcloud 11.0 milestone Oct 27, 2016

BernhardPosselt reviewed Oct 27, 2016

View reviewed changes

nickvergessen reviewed Oct 28, 2016

View reviewed changes

LukasReschke added 11 commits October 31, 2016 17:17

Use new appstore API

32cf661

This change introduces the new appstore API in Nextcloud. Signed-off-by: Lukas Reschke <[email protected]>

Fix tests

8d83e23

Signed-off-by: Lukas Reschke <[email protected]>

Make non-static

ca7f6de

Signed-off-by: Lukas Reschke <[email protected]>

Read array elements instead of substr

067fb18

Otherwise this would break with 11.0.0 Signed-off-by: Lukas Reschke <[email protected]>

Use substr and explode instead of a regex

89fc435

Signed-off-by: Lukas Reschke <[email protected]>

%d instead %s

01c5668

These are only numbers. THX @nickvergessen Signed-off-by: Lukas Reschke <[email protected]>

Replace with exception instead of boolean return value

0e2aee2

Signed-off-by: Lukas Reschke <[email protected]>

Add support for CRL

3e6dd86

Signed-off-by: Lukas Reschke <[email protected]>

Query the timefactory instead of creating it

df7fd2b

Signed-off-by: Lukas Reschke <[email protected]>

Add update support

8acb54a

Signed-off-by: Lukas Reschke <[email protected]>

Proxy images through usercontent.apps.nextcloud.com

a685957

Signed-off-by: Lukas Reschke <[email protected]>

LukasReschke force-pushed the new-appstore branch from 5848ac7 to a685957 Compare October 31, 2016 16:17

Use findAppInDirectories

d805df7

The other function doesn't work if the appstore is disabled Signed-off-by: Lukas Reschke <[email protected]>

LukasReschke added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 31, 2016

LukasReschke changed the title ~~[WIP] Use new appstore API~~ Use new appstore API Oct 31, 2016

LukasReschke added 3 commits October 31, 2016 18:42

Move to non-static version

086d43f

The static version is used nowhere in the code and just decreases coverage Signed-off-by: Lukas Reschke <[email protected]>

Remove non-required getter

1a676ba

Signed-off-by: Lukas Reschke <[email protected]>

Add tests for installer method

0eeef26

Signed-off-by: Lukas Reschke <[email protected]>

nickvergessen reviewed Nov 2, 2016

View reviewed changes

stp-ip merged commit e81d04c into master Nov 2, 2016

ChristophWurst deleted the new-appstore branch November 2, 2016 17:17

ChristophWurst mentioned this pull request Nov 3, 2016

use correct internal server version nextcloud/mail#180

Merged

MorrisJobke added a commit that referenced this pull request Nov 17, 2016

Remove settings pane in app management

6d5cc6e

* experimental apps where removed with 32cf661 (#1940) * fixes #2183 Signed-off-by: Morris Jobke <[email protected]>

MorrisJobke mentioned this pull request Nov 17, 2016

Remove settings pane in app management #2188

Merged

MorrisJobke mentioned this pull request Feb 27, 2018

Add "Search" category nextcloud/appstore#551

Closed

MorrisJobke added a commit that referenced this pull request Apr 23, 2018

Remove a check for an unused variable

4fd697e

Left over from a refactoring #1940 Signed-off-by: Morris Jobke <[email protected]>

MorrisJobke mentioned this pull request Apr 23, 2018

Fix undefined variable - found by Phan #9271

Merged

MorrisJobke added a commit that referenced this pull request May 29, 2018

Remove a check for an unused variable

57489f1

Left over from a refactoring #1940 Signed-off-by: Morris Jobke <[email protected]>

MorrisJobke mentioned this pull request Nov 19, 2018

Missing infomration about: appstore.experimental.enabled nextcloud/documentation#931

Closed

Use new appstore API #1940

Use new appstore API #1940

Conversation

LukasReschke commented Oct 27, 2016 • edited Loading

Todo

mention-bot commented Oct 27, 2016

LukasReschke commented Oct 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukasReschke commented Oct 31, 2016

rullzer commented Nov 1, 2016

Choose a reason for hiding this comment

LukasReschke Nov 2, 2016 • edited Loading

Choose a reason for hiding this comment

LukasReschke Nov 2, 2016 • edited Loading

Choose a reason for hiding this comment

stp-ip commented Nov 2, 2016

LukasReschke commented Oct 27, 2016 •

edited

Loading

LukasReschke Nov 2, 2016 •

edited

Loading

LukasReschke Nov 2, 2016 •

edited

Loading