Make OCS routes work together with the AppFramework

[LukasReschke]

Mhm. Would be great to incorporate either OCS with the AppFramework somehow or switch to REST routes powered by AppFramework controllers. That way we would be able to use all advantages of the AppFramework also in OCS routes... - Let me file a technical debt issue about that.

From #12452 (comment)

[PVince81]

CC @Raydiation

Having such integration could also be a necessary step to allow smoother migration to the app framework eventually.

[LukasReschke]

From #12452 (comment):

I'd like to bring up a few objections against adding yet another OCS API. And as usual use this opportunity to cheer for the AppFramework. While I know that there are some people with objections against it, this is really the way to go.

If we don't begin now with implementing new functionality in a clean way we will all know what happens: This code will stay in for ages and potential pollute other code as well with smell.
(Please note that this goes not against the code that @schiesbn wrote, this are all drawbacks caused by the usage of OCS)

---

Permission checks and functions are decoupled

The OCS API does it's permission checks at the route definition as following:

OC_API::register(
    'get',
    '/cloud/user',
    array('OC_OCS_Cloud', 'getCurrentUser'),
    'core',
    OC_API::USER_AUTH
);

From a developers and auditor's point of view this makes the work even harder, because you have to find the correct routing file and then look-up which permissions are required to call the function.

If you use the AppFramework the permission checks are implemented as built-in middleware using annotations and always require the highest possible permissions. (opt-out instead of opt-in)

The following function could only get executed by admins:

<?php
namespace OCA\MyApp\Controller;

use \OCP\IRequest;
use \OCP\AppFramework\Controller;

class PageController extends Controller {

    /**
     * Following function can be accessed by admins only
     */
    public function freeForAdmins() {

   }

    /**
     * Following function can be accessed by any authenticated user
     * @NoAdminRequired
     */
    public function freeForUsers() {

   }

    /**
     * Following function can be accessed by everybody, even not logged-in users
     * @PublicPage
     */
    public function freeForAll() {

   }

}

This kind of annotation makes it more clear who can access a function and thus reduces the risk of bugs and makes the code cleaner to read.

---

POST/GET parameters cannot be injected as function parameter and have no annotations

From a clean code PoV you really don't want to use globals such as $_POST or $_GET directly. Instead you want to add these globals as function parameters instead, let me give you an example, this is the currently existing code:

/**
 * get all shares
 *
 * @param array $params option 'file' to limit the result to a specific file/folder
 * @return \OC_OCS_Result share information
 */
public static function getAllShares($params) {
    if (isset($_GET['shared_with_me']) && $_GET['shared_with_me'] !== 'false') {
        return self::getFilesSharedWithMe();
    }
    // if a file is specified, get the share for this file
    if (isset($_GET['path'])) {
        $params['itemSource'] = self::getFileId($_GET['path']);
        $params['path'] = $_GET['path'];
        $params['itemType'] = self::getItemType($_GET['path']);
        if ( isset($_GET['reshares']) && $_GET['reshares'] !== 'false' ) {
            $params['reshares'] = true;
        } else {
            $params['reshares'] = false;
        }
        if (isset($_GET['subfiles']) && $_GET['subfiles'] !== 'false') {
            return self::getSharesFromFolder($params);
        }
        return self::collectShares($params);
    }
}

As you can see you have no chance at all to see which global parameters (``$_GET['shared_with_me'], $_GET['path']`, `$_GET['reshares']` and `$_GET['subfiles']`) are existant. This is very bad as this leads to bad testability and furthermore is prone to introduce bugs in the future. Also it's not quite clear what types the variables are. (are they booleans/integers/strings/arrays/etc...?)

Instead with the AppFramework some reflection magic will allow you to add the parameters directly to the functions definition. You can also define the type and it will automatically verify and convert it for you. (or fallback to a default sane value such as "" or 0)

/**
 * get all shares
 * @PublicPage
 * 
 * @param array $params option 'file' to limit the result to a specific file/folder
 * @param bool $sharedWithMe
 * @param string $path
 * @param bool $reshares
 * @param bool $subfiles
 * @return \OC_OCS_Result share information
 */
public function getAllShares($params, $sharedWithMe = false, $path, $reshares = false, $subfiles = false) {
    if ($sharedWithMe === true) {
        return self::getFilesSharedWithMe();
    }
    // if a file is specified, get the share for this file
    if (!empty($path)) {
        $params['itemSource'] = self::getFileId($path);
        $params['path'] = $path;
        $params['itemType'] = self::getItemType($path);
        $params['reshares'] = $reshares;
        if ($subfiles === true) {
            return self::getSharesFromFolder($params);
        }
        return self::collectShares($params);
    }
}

Using this way the code gets way cleaner to read and easier to test. Also you can easily enforce a type, specifiy defaults, etc...

---

Custom middlewares are not possible

If some kinds of permission checks are required it is better to implement this as middleware instead of having to copy the same code over and over again.

This is also the case here were the following code snippet is there multiple times in different functions:

if (!$this->isS2SEnabled(true)) {
    return \OC_OCS_Result(null, 503, 'Server does not support server-to-server sharing');
}

Implementing this code is bad for multiple reasons:

1. You have to opt-in instead of opt-out, thus if a developer forgets it there is unexpected behaviour.
2. This is bad again for testability.
3. DRY

So, the proper way with the AppFramework would be to use a middleware (registration is left out of here, but would only be a single line within the applications App container definition):

class IsSharingEnabledMiddleware extends Middleware {
    private $isSharingEnabled;
    private $reflector;

    /**
     * @param bool $isSharingEnabled
     * @param ControllerMethodReflector $reflector
     */
    public function __construct($isSharingEnabled, ControllerMethodReflector $reflector) {
        $this->isSharingEnabled = $isSharingEnabled;
        $this->reflector = $reflector;
    }

    public function beforeController($controller, $methodName){
        if(!$isSharingEnabled && !$this->reflector->hasAnnotation('WorksEvenWithoutSharing')) {
            throw new SecurityException('Sharing is not enabled', Http::STATUS_UNAUTHORIZED);
        }
    }
}

With that code now an exception is thrown (that can be catched using AfterException) if sharing is not enabled and the function has not the annotation WorksEvenWithoutSharing. Thus it defaults to "check the permission" but can be changed to "not check the permission".

---

AppFramework middlewares are not used

ownCloud comes with a few built-in middlewares to enhance security and performance. For example all sessions are closed by the middleware unless the controller has a UseSession annotation. This leads to major performance improvements and app developers don't have to do anything.

This allows us to improve performance without adjusting every application. - If you don't use the AppFramework, your app will simply not be able to use this.

---

Routes are decoupled at ocs/routes.php instead of apps/files_sharing/appinfo/routes.php

This is a nightmare when it comes to finding the correct entry-point. Also it calls static functions and is way less easier to understand than proper AppFramework routes:

s2s = new \OCA\Files_Sharing\API\Server2Server();
OC_API::register('post',
        '/cloud/shares',
        array($s2s, 'createShare'),
        'files_sharing',
        OC_API::GUEST_AUTH
);
OC_API::register('post',
        '/cloud/shares/{id}/accept',
        array($s2s, 'acceptShare'),
        'files_sharing',
        OC_API::GUEST_AUTH
);
OC_API::register('post',
        '/cloud/shares/{id}/decline',
        array($s2s, 'declineShare'),
        'files_sharing',
        OC_API::GUEST_AUTH
);
OC_API::register('post',
        '/cloud/shares/{id}/unshare',
        array($s2s, 'unshare'),
        'files_sharing',
        OC_API::GUEST_AUTH
);

vs.

$application = new Application();
$application->registerRoutes($this, array(
    'routes' => array(
        array('name' => 'server2server#createShare', 'url' => '/create', 'verb' => 'POST'),
        array('name' => 'server2server#acceptShare', 'url' => '/{id}/accept', 'verb' => 'POST'),
        array('name' => 'server2server#declineShare', 'url' => '/{id}/decline', 'verb' => 'POST'),
        array('name' => 'server2server#unshare', 'url' => '/{id}/unshare', 'verb' => 'POST')
    )
));

---

Static functions are the hell

I think the title says enough… - With the AppFramework not even the controllers are static…

---

No versioning possible of API

There is no sane versioning possible if we go the OCS route! This means that the API is never allowed to change, which is a major compatibility hell!

With the AppFramework you're usually going to create routes such as /v1-2/create allowing you to have multiple endpoints for compatbility routes.

---

I know, this is a controverse point but I'd really advocate to make OCS compatible with the AppFramework.

But then again: Why do we really need OCS? - Why don't we just use REST routes such as the AppFramework offers?

I know the URLs won't then look ocs/v1.php/share/create but instead index.php/apps/files_sharing/api/s2s/create. - But it is a new API and unless we have some REALLY good reason to stay with OCS for NEW code we should consider switching.

(Fun fact: That v1.php is an hard-coded PHP file in core)

\cc @Raydiation @MorrisJobke @DeepDiver1975 @craigpg

[butonic]

👍

I started an updated version of the OCS and tried to make it more RESTful, adding curl examples: https://gist.github.com/butonic/9821d5769772e2e3f5ff I still need to finish it and add JSON versions of the examples. I did not get past the FAN entry point, but if we want to clean up our API, we might as well update the OCS API. cc @karlitschek

[LukasReschke]

I started an updated version of the OCS and tried to make it more RESTful, adding curl examples: https://gist.github.com/butonic/9821d5769772e2e3f5ff I still need to finish it and add JSON versions of the examples.

Can we use the accept header for the content-type instead of ?json? - The AppFramework supports automatic converting for DataResponse.

[BernhardPosselt]

It falls back to JSON if you dont supply a content-type that is supported fyi

[butonic]

Can we use the accept header for the content-type instead of ?json?

That is one of the things that I meant by "more RESTful".

[schiessle]

I started an updated version of the OCS and tried to make it more RESTful

I think that's the right approach, thanks @butonic for the initiative. Both the API and the implementation in ownCloud can be improved (but that's most likely true for every API and implementation, that's how software advances over time). But I don't think it is a bad decision in general to use OCS.

The App Framework adds a lot of great stuff to ownCloud, thanks to everyone who worked on it! But in my opinions we also added some problems with the App Framework, for example two different APIs (OCS vs REST-style API from the frame work) and two different and incompatible hook systems. Ideally we would have solved this issues before we added the App Framework to core. But that's how it is. Now let's look forward and improve what we have. IMHO the App Framework should just provide a API for OCS instead of inventing a complete new API and OCS should be improved like started by Jörn.

That's just my 2 cents.

Just some comments to #12454 (comment)

• You can define OCS routes in apps/files_sharing/appinfo/routes.php. Just look at the file and you will find OCS routes in it.
• You can also add versions to OCS routes to have the possibility to change the API in the future. It is done for the local share API as you will see if you look at apps/files_sharing/appinfo/routes.php

[BernhardPosselt]

IMHO the App Framework should just provide a API for OCS instead of inventing a new complete new API and OCS should be improved like started by Jörn.

Very easy: create an OCSResponse and do the transformation in the render method. You can even "catch" Http::STATUS_X headers in the render method and transform them to the appropriate xml tag

[PVince81]

Discussion here about trying and use WebDAV a little bit more, as yet another alternative: #12543
Also see discussion here about REST vs WebDAV: #12504 (comment)

[PVince81]

Since we're neither removing OCS nor the app framework, I think integrating both is a step in the right direction.

@Raydiation another part that seems to be missing is being able to map the routes to start with "/ocs/v1.php/..." (it didn't work when I tried so)

[LukasReschke]

Since we're neither removing OCS nor the app framework, I think integrating both is a step in the right direction.

Which was also pointed out pretty much at the top of the issue description…

Mhm. Would be great to incorporate either OCS with the AppFramework somehow or switch to REST routes powered by AppFramework controllers

[karlitschek]

And option is to integrate OCS in the app framework. And it is also possible to tweak some of the behavior like the return codes. But OCS won´t go away. Same for WebDAV by the way. ;-)

[DeepDiver1975]

And option is to integrate OCS in the app framework. And it is also possible to tweak some of the behavior like the return codes. But OCS won´t go away. Same for WebDAV by the way. ;-)

• OCS from an protocol point of view has to continue to exist - it is a public available, released and used API
• but we can make the developers life easier to integrate the programming logic into app framework (reasoning is above - testing etc.)
• looking a bit more into the future we still have to come up with an more REST-ish version of OCS to fit today's expectations of developers (we already had long discussions about this - hopefully we can walk this path some day 🙈 )
• finally - still talking about the future - we should really consider the WebDAV ideas which have been raised by @evert in Replace WebDAV with REST #12504 (comment) - which was leading to some promissing new ideas (maybe pure webdav with required extension is enough from an ownCloud Public Http API point of view ❓ )

[PVince81]

Setting to OC 8 as this bridge is needed for the OCS endpoints of the metadata app.

[DeepDiver1975]

8.1 as we reached feature freeze

[PVince81]

@Raydiation would you be interested in helping with this ?
This is needed so we can port the apps with existing OCS endpoints (ex: files_sharing) to use the app framework.

One of the challenges is that some routes can return multiple values, like "cloud/getcapabilities". It seems that it would call this route for every app and gather the results in a multi-status response.

[PVince81]

Had a discussion with @Raydiation about possible approaches.

One trouble is that we cannot simply make "ocs/v1.php" use the same router, there would be conflicts.

So the suggested approach is to either:

1. Make ocs/v1.php/... a redirect to index.php/ocs/v1/.... This assuming that all API clients support HTTP redirects
   or
2. Add a rewrite rule in .htaccess that rewrites ocs/v1.php/... to index.php/ocs/v1/...

The OCS route can then be used like any other app framework route.

What do you guys think ?

[PVince81]

Did we make any progress on this ?

We're past feature freeze, moving to 9.1

@cmonteroluque

[DeepDiver1975]

There is a WIP branch in https://github.com/owncloud/core/tree/ocs_appframework ! Pretty clean even ;)

nice - please open pr! THX

[DeepDiver1975]

@cmonteroluque @PVince81 setting 9.2?

[PVince81]

@DeepDiver1975 move to backlog or was this done already ? I remember something about OCS routes but it might be something else.

[DeepDiver1975]

@DeepDiver1975 move to backlog or was this done already ? I remember something about OCS routes but it might be something else.

I'll have a look

[DeepDiver1975]

63 APIs are still using the old registration mechanism - grep for API::register

[DeepDiver1975]

from a perf pov we shall fix this

[PVince81]

@DeepDiver1975 any estimate ?

[DeepDiver1975]

8d+

[PVince81]

Move to 10.1 then ?

[PVince81]

backlog then

if this is important it needs to be scheduled before other tech debt tickets

[PVince81]

• files_external
• files_sharing, removed with Rewire notify by email #33180 and Show pending remote shares at the 'Shared with you' tab #37022
• provisioning_api Use OcsController in provisioning_api routes #37382
• testing app
• activity app