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

Clarify the description of REST vs OCS in accordance to sugestions discussed #12264

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 2 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
15 changes: 12 additions & 3 deletions developer_manual/basics/controllers.rst
Original file line number Diff line number Diff line change
Expand Up @@ -732,10 +732,15 @@ The following policy for instance allows images, audio and videos from other dom
OCS
^^^

.. note:: This is purely for compatibility reasons. If you are planning to offer an external API, go for a :doc:`../digging_deeper/rest_apis` instead.
In order to simplify exchange of data between the Nextcloud backend and any client (be it the web frontend or whatever else), the OCS API has been introduced.
Here, JSON and XML responders have been prepared and are installed without additional effort.

In order to ease migration from OCS API routes to the App Framework, an additional controller and response have been added. To migrate your API you can use the **OCP\\AppFramework\\OCSController** base class and return your data in the form of a DataResponse in the following way:
.. note::
The usage of OCS is closely related to the usage of :doc:`../digging_deeper/rest_apis`.
Unless you have a clear use-case, it is advised to use OCS over pure REST.
A more detailed description can be found in :ref:`ocs-vs-rest`.

To use OCS in your API you can use the **OCP\\AppFramework\\OCSController** base class and return your data in the form of a **DataResponse** in the following way:

.. code-block:: php

Expand All @@ -759,7 +764,7 @@ In order to ease migration from OCS API routes to the App Framework, an addition

The format parameter works out of the box, no intervention is required.
Copy link
Member

Choose a reason for hiding this comment

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

I know you didn't change this, but this sentence is a bit unclear. It talks about the ?format=json parameter, but doesn't really explain it. IMO using the format parameter is not good, the Accept: application/json header should be used instead as it is standardized.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to address this in the latest commit.
grafik


In order to make routing work for OCS routes you need to add a separate 'ocs' entry to the routing table of your app.
In order to make routing work for OCS routes you need to add a separate 'ocs' entry to the routing table in ``appinf/routes.php`` of your app.
Inside these are normal routes.

.. code-block:: php
Expand All @@ -778,6 +783,10 @@ Inside these are normal routes.

Now your method will be reachable via ``<server>/ocs/v2.php/apps/<APPNAME>/api/v1/shares``

.. versionadded:: 29
You can use the attribute ``ApiRoute`` as described in :doc:`Routing <routing>` instead of the entry in ``appinfo/routed.php`` as an alternative.
christianlupus marked this conversation as resolved.
Show resolved Hide resolved


Handling errors
^^^^^^^^^^^^^^^

Expand Down
2 changes: 1 addition & 1 deletion developer_manual/conf.py
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@
#'pointsize': '10pt',

# Additional stuff for the LaTeX preamble.
'preamble': '\extrafloats{100}\maxdeadcycles=500\DeclareUnicodeCharacter{274C}{\sffamily X}',
'preamble': '\\extrafloats{100}\\maxdeadcycles=500\\DeclareUnicodeCharacter{274C}{\\sffamily X}',
Copy link
Member

Choose a reason for hiding this comment

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

👀

}

# Grouping the document tree into LaTeX files. List of tuples
Expand Down
69 changes: 67 additions & 2 deletions developer_manual/digging_deeper/rest_apis.rst
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@

.. sectionauthor:: Bernhard Posselt <dev@bernhard-posselt.com>

Offering a RESTful API is not different from creating a :doc:`route <../basics/routing>` and :doc:`controllers <../basics/controllers>` for the web interface. It is recommended though to inherit from ApiController and add **@CORS** annotations to the methods so that `web applications will also be able to access the API <https://developer.mozilla.org/en-US/docs/Web/HTTP/Access_control_CORS>`_.
Offering a RESTful API is not different from creating a :doc:`route <../basics/routing>` and :doc:`controllers <../basics/controllers>` for the web interface.
It is recommended though to inherit from ApiController and add **@CORS** annotations to the methods so that `web applications will also be able to access the API <https://developer.mozilla.org/en-US/docs/Web/HTTP/Access_control_CORS>`_.
Comment on lines -9 to +10
Copy link
Member

Choose a reason for hiding this comment

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

No relevant diff?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just styling to put every sentence on it's own line. I can drop that as well.


.. code-block:: php

Expand Down Expand Up @@ -44,7 +45,8 @@
)


Keep in mind that multiple apps will likely depend on the API interface once it is published and they will move at different speeds to react to changes implemented in the API. Therefore it is recommended to version the API in the URL to not break existing apps when backwards incompatible changes are introduced::
Keep in mind that multiple apps will likely depend on the API interface once it is published and they will move at different speeds to react to changes implemented in the API.
Therefore it is recommended to version the API in the URL to not break existing apps when backwards incompatible changes are introduced::
Comment on lines -47 to +49
Copy link
Member

Choose a reason for hiding this comment

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

Same here


/index.php/apps/myapp/api/1.0/resource

Expand Down Expand Up @@ -79,3 +81,66 @@
}

}

.. _ocs-vs-rest:

Relation of REST and OCS
------------------------

There is a close relationship between REST APIs and :ref:`OCS <ocscontroller>`.
Both provide a way to transmit data between the backend of the app in the Nextcloud server and some frontend.

The following combinations of attributes might be useful for various scenarios:

#. Plain frontend route: No special attribute related to CSRF or CORS
christianlupus marked this conversation as resolved.
Show resolved Hide resolved
#. Plain Frontend with CRSF checks disabled: Add the ``#[NoCSRFRequired]`` attribute
Copy link
Member

Choose a reason for hiding this comment

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

CSRF checks should only be disabled when absolutely necessary, this line should have a big warning

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are some of the few words I wanted to add...

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#. Plain Frontend with CRSF checks disabled: Add the ``#[NoCSRFRequired]`` attribute
#. Plain Frontend with CRSF checks disabled: ``Controller`` class and ``#[NoCSRFRequired]`` attribute on the method

#. REST route with CORS enabled: Add the ``#[CORS]`` attribute
christianlupus marked this conversation as resolved.
Show resolved Hide resolved
#. OCS-based route
christianlupus marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

There is another case with OCS+CSRF


There are different ways a clients might interact with your APIs.
These ways depend on your API configuration (what you allow) and on which route the request is finally made.

- *Access from web frontend* means the user is browses the Nextcloud web frontend with a browser.
- *Access from an external app* indicates that the user is not using the normal browser (as logged in) but directly navigates a certain URL.
This can be in a browser tab or an external program (like an Android app or simply a curl command line).
Copy link
Member

Choose a reason for hiding this comment

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

These two cases are different, as the CSRF checks want to prevent the former while the later should usually be allowed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is former and latter here for you? Browser tab and curl?

Copy link
Member

Choose a reason for hiding this comment

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

Yes

- *Access from external website* means that the user browses some third party web site and *magically* data from your app appears.
Technically, the other website would embed/load/use images, JSON data, or other resources from a URL pointing to the Nextcloud server.
Copy link
Member

Choose a reason for hiding this comment

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

This use-case is relatively niche and I think the general advice is to avoid it (and CORS altogether) as it opens unintended security issues if you are not careful. I think this should have some warnings as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add some words here as well


.. list-table:: Comparision of different API types

Check failure on line 109 in developer_manual/digging_deeper/rest_apis.rst

View workflow job for this annotation

GitHub Actions / Check spelling

Comparision ==> Comparison
:header-rows: 1
:align: center

* - Description
- 1 (plain)
- 2 (no CSRF)
- 3 (CORS)
- 4 (OCS)
* - URL prefix (relative to server)
- ``/apps/<appid>/``
- ``/apps/<appid>/``
- ``/apps/<appid>/``
- ``/ocs/v2.php/apps/<appid>/``
* - Access from web frontend
- yes
- yes (CSRF risk)
- yes (CSRF risk)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe explain that for CORS one needs to disable CSRF and thus automatically poses a security risk through CSRF attacks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good point.

- yes
* - Access from external app
- --- (CSRF protection blocks)
- yes
- yes
- yes
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- yes
- no

OCS is still protected against CSRF attacks using the OCS-APIRequest header or a CSRF token.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't this line disabling the csrf checks for OCS when done with a bearer token?

Copy link
Member

Choose a reason for hiding this comment

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

* - Access from external web page
- ---
- ---
- yes
- yes
christianlupus marked this conversation as resolved.
Show resolved Hide resolved
* - Transmitted data type
- plain data
- plain data
- plain data
- encapsulated data
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- plain data
- plain data
- plain data
- encapsulated data
- plain data
- plain data
- plain data
- encapsulated data (JSON or XML)

"plain data" is not very descriptive. In fact any response can be used for any of these methods, just OCS handles the DataResponses differently

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should be a bit more verbose on this, I see


As a rule of thumb one can conclude that OCS provides a good way to handle most use cases.
christianlupus marked this conversation as resolved.
Show resolved Hide resolved
The only exception to this is if you want to provide an API for external usage where you have to comply with an externally defined API scheme.
Here, the encapsulation introduced in OCS might be in your way.
christianlupus marked this conversation as resolved.
Show resolved Hide resolved