-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
base: master
Are you sure you want to change the base?
Changes from 2 commits
71d81da
94c8694
0758bc0
4b76fec
ca6794c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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}', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👀 |
||
} | ||
|
||
# Grouping the document tree into LaTeX files. List of tuples | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No relevant diff? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||||||||||||||||||
|
||||||||||||||||||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here |
||||||||||||||||||
|
||||||||||||||||||
/index.php/apps/myapp/api/1.0/resource | ||||||||||||||||||
|
||||||||||||||||||
|
@@ -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 | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are some of the few words I wanted to add... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||
#. 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is former and latter here for you? Browser tab and curl? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||||||||||||||||||
: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) | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
OCS is still protected against CSRF attacks using the OCS-APIRequest header or a CSRF token. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not really, it also checks that the custom header is set (https://github.com/nextcloud/server/blob/94c529409813e03d632662283a5cd302ef8e9781/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php#L235) which is a CSRF check. |
||||||||||||||||||
* - 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 | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
"plain data" is not very descriptive. In fact any response can be used for any of these methods, just OCS handles the DataResponses differently There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
|
There was a problem hiding this comment.
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, theAccept: application/json
header should be used instead as it is standardized.There was a problem hiding this comment.
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.