Make OCS routes work together with the AppFramework

LukasReschke commented 9 years ago

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 https://github.com/owncloud/core/pull/12452#issuecomment-64597352

PVince81 commented 9 years ago

CC @Raydiation

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

LukasReschke commented 9 years ago

From https://github.com/owncloud/core/pull/12452#issuecomment-65210669:

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:

You have to opt-in instead of opt-out, thus if a developer forgets it there is unexpected behaviour.
This is bad again for testability.
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 commented 9 years ago

:+1:

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 commented 9 years ago

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 commented 9 years ago

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

butonic commented 9 years ago

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 commented 9 years ago

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 https://github.com/owncloud/core/issues/12454#issuecomment-65211607

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 commented 9 years ago

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 commented 9 years ago

Discussion here about trying and use WebDAV a little bit more, as yet another alternative: https://github.com/owncloud/core/issues/12543 Also see discussion here about REST vs WebDAV: https://github.com/owncloud/core/issues/12504#issuecomment-65129333

PVince81 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 :see_no_evil: )

finally - still talking about the future - we should really consider the WebDAV ideas which have been raised by @evert in https://github.com/owncloud/core/issues/12504#issuecomment-65129333 - 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 :question: )

PVince81 commented 9 years ago

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

DeepDiver1975 commented 9 years ago

8.1 as we reached feature freeze

PVince81 commented 9 years ago

@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 commented 9 years ago

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 ?

BernhardPosselt commented 9 years ago

The routing should redirect ocs/v1.php/apps/files/(.) to index.php/apps/files/ocs/(.) :)

BernhardPosselt commented 9 years ago

Either by mod_rewrite or http redirect. That way you can explicitly whitelist transitioned apps while still keeping compability for everyone else

PVince81 commented 9 years ago

CC @tomneedham who knows more about OCS than I do

BernhardPosselt commented 9 years ago

Ok, the API in general seems to be a bit full of weirdness like merging responses etc so I dont think you can fully transition, you will always need to keep the old stuff around (e.g. capabilities thingy)

So what you probably want is either of the following two things:

Keep the old API but don't invest in it further, mark it as deprecated. Then create a RESTful API using the app framework and you're done -> gradually transitioning away from OCS
Deal with the funky stuff like capabilities but move 95% to the nice and testable appframework controller api. You can do that by adding custom redirects/url rewrites and register a Responder for both xml and json that wraps onto an OCSResponse class (which itself can be done for you in an OCSController class that you inherit from).

Since ocs uses this method to transform an array to json/xml we can wrap it in a response very easily: https://github.com/owncloud/core/blob/master/lib/private/ocs.php#L124

Its also side effect free, meaning the static call wont hurt and we can test it properly.

So how would a controller with the ocs response look like:

<?php
use OCP\AppFramework\OCSController;

class ShareController extends OCSController {

    /**
     * @NoAdminRequired
     * @CORS
     * @NoCSRFRequired
     */
    public function getSharesOfUser($user) {
        $actualData = $this->service->get_data();
        return [
            'status' => 'blubb',
            'statuscode' => 550,
            'message' => 'message',
            'data' => $actualData,
            'tag' => '',
            'tagattribute' => '',
            'dimension' => -1,
            'itemscount' => 45,
            'itemsperpage' => 5
        ];
    }

}

Since OCS does not use HTTP status codes you dont have to wrap it in a dataresponse and can simply return an array.

BernhardPosselt commented 9 years ago

Here you go https://github.com/owncloud/core/pull/13921

Implementation for the thing outline in my previous post

PVince81 commented 9 years ago

@Raydiation thanks a lot.

@DeepDiver1975 regarding the question of the route, it was discussed here: https://github.com/owncloud/core/issues/12454#issuecomment-73020084 Basically we could move the endpoint to "index.php/ocs/..." and have a rewrite rule or a redirect for the old format that was based on "ocs.php".

DeepDiver1975 commented 9 years ago

well - there is still the logic in ocs.php regarding the error handling - somewhat related to https://github.com/owncloud/core/issues/11834

BernhardPosselt commented 9 years ago

Error handling? Maybe use a middleware implementing the afterException method

rullzer commented 9 years ago

Any status on which road we will follow? Probably getting this into 8.1 is not really feasable anymore. But 8.2 would be a good idea so we can start migrating stuff to the appframework.

MorrisJobke commented 9 years ago

Nothing for 8.1 -> moved to 8.2

PVince81 commented 9 years ago

Any update ? Move to 9.0 ? (we're past feature freeze)

DeepDiver1975 commented 9 years ago

I don't see anything with this respect for 8.2

rullzer commented 8 years ago

@BernhardPosselt the capabilities issue has been resolved. As we now have a capabilities manager that handles all this for us.

PVince81 commented 8 years ago

Did we make any progress on this ?

We're past feature freeze, moving to 9.1

@cmonteroluque

rullzer commented 8 years ago

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

DeepDiver1975 commented 8 years ago

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

nice - please open pr! THX

DeepDiver1975 commented 8 years ago

@cmonteroluque @PVince81 setting 9.2?

rullzer commented 8 years ago

Ok will open WIP PR for it :)

ghost commented 8 years ago

nod, thanks

PVince81 commented 7 years ago

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

DeepDiver1975 commented 7 years ago

@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 commented 7 years ago

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

DeepDiver1975 commented 7 years ago

from a perf pov we shall fix this

PVince81 commented 7 years ago

@DeepDiver1975 any estimate ?

DeepDiver1975 commented 7 years ago

8d+

PVince81 commented 7 years ago

Move to 10.1 then ?

PVince81 commented 7 years ago

backlog then

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

PVince81 commented 6 years ago

[ ] files_external
[x] files_sharing, removed with https://github.com/owncloud/core/pull/33180 and https://github.com/owncloud/core/pull/37022
[ ] provisioning_api https://github.com/owncloud/core/pull/37382
[ ] testing app
[ ] activity app

owncloud / core

Make OCS routes work together with the AppFramework #12454