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

christianlupus · 2024-10-03T18:34:15Z

☑️ Resolves

The statement about OCS to be legacy is removed
A comparison with (plain) REST is made

🖼️ Screenshots

T.B.D.

Signed-off-by: Christian Wolf <[email protected]>

christianlupus · 2024-10-03T18:41:58Z

@provokateurin I started to rephrase the "do not use OCS" in this PR. Does this sound reasonable or am I going completely the wrong direction? I know I need to eventually write a few more words and I for sure have to check the table entries (and wanted to explain them a bit more).

What do you think, makes this sense? Is this to be discussed with some guys from the core team before merging (as it reverts some basic strategic statement)?

provokateurin

Thanks a lot, this is already super helpful!
The RST syntax for the table really messed with my brain, I need to take a closer look at it in proper table shape.

developer_manual/basics/controllers.rst

provokateurin · 2024-10-03T19:26:49Z

developer_manual/conf.py

@@ -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}',


provokateurin · 2024-10-03T19:27:41Z

developer_manual/digging_deeper/rest_apis.rst

-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>`_.


No relevant diff?

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

provokateurin · 2024-10-03T19:27:54Z

developer_manual/digging_deeper/rest_apis.rst

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


provokateurin · 2024-10-03T19:28:53Z

developer_manual/digging_deeper/rest_apis.rst

+The following combinations of attributes might be useful for various scenarios:
+
+#. Plain frontend route: No special attribute related to CSRF or CORS
+#. Plain Frontend with CRSF checks disabled: Add the ``#[NoCSRFRequired]`` attribute


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

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

developer_manual/digging_deeper/rest_apis.rst

provokateurin · 2024-10-03T19:41:06Z

developer_manual/digging_deeper/rest_apis.rst

+The following combinations of attributes might be useful for various scenarios:
+
+#. Plain frontend route: No special attribute related to CSRF or CORS
+#. Plain Frontend with CRSF checks disabled: Add the ``#[NoCSRFRequired]`` attribute


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

developer_manual/digging_deeper/rest_apis.rst

provokateurin · 2024-10-03T19:42:13Z

developer_manual/digging_deeper/rest_apis.rst

+#. Plain frontend route: No special attribute related to CSRF or CORS
+#. Plain Frontend with CRSF checks disabled: Add the ``#[NoCSRFRequired]`` attribute
+#. REST route with CORS enabled: Add the ``#[CORS]`` attribute
+#. OCS-based route


There is another case with OCS+CSRF

Co-authored-by: Kate <[email protected]> Signed-off-by: Christian Wolf <[email protected]>

Signed-off-by: Christian Wolf <[email protected]>

christianlupus · 2024-10-07T22:05:41Z

I tried to address the points discussed here. I decided that the CORS should be better put in a separate section as it seems orthogonal to the original question (what should I use Controller or OCSController?).

For simpler reading, I put screenshots of the current state here:

provokateurin

Looks pretty good now!

developer_manual/basics/controllers.rst

provokateurin · 2024-10-08T07:03:16Z

developer_manual/digging_deeper/rest_apis.rst

+#. Plain frontend route: ``Controller`` class
+#. Plain Frontend with CRSF checks disabled: ``Controller`` class and ``#[NoCSRFRequired]`` attribute on the method
+#. OCS-based route: ``OCSController`` class
+#. OCS-based route with CSRF disabled: ``OCSController`` class and ``#[NoCSRFRequired]`` attribute on the method


I think this case doesn't exist, NoCSRFRequired should not have an effect on OCSController, but I will have to check that in the code to be sure.

I was thinking back and forth on this as well. In the end, I decided to keep the case as there is a certain difference (at least from the implementation):

For methods with NoCSRFRequired, this section is doing nothing.

For the case where the attribute is not given, the strict cookie check boils down to a check on the OCS-APIREQUEST header or no valid session token.

With the header in place (as the OCS controller generally expects), the second part is also skipped: The CSRF check is hardcoded to true, thus isInvalidCSRFRequired returns false.

When the header is not set, you enter the second if block and will check the inner if. The check isinstance OCSController is obviously true. The isValidOCSRequest is somewhat redundant as it can only be reached with the header unset. Alternatively, a bearer authorization header is sufficient to make the CSRF checker happy (even without OCS header and without NoCSRFRequired attribute).

I was adding this case as a reaction on your statement

There is another case with OCS+CSRF

In my opinion the last case could be dropped (and was never planned in the first round) but I do not see clearly the motivation of it, to be honest. Did I misunderstand your statement above?

In my opinion the last case could be dropped (and was never planned in the first round) but I do not see clearly the motivation of it, to be honest. Did I misunderstand your statement above?

I messed up, I meant OCS+CORS...
Currently the CORS cases are completely missing in the list.

Yeah, okay, then that makes more sense. I can read the cors posts and extend by the OCS+cors as well.

I will add a warning about cors in general similar to csrf of you do not mind.

If you have any idea how to describe the case Access from external website more precisely or otherwise better, feel free to comment 😉.

Signed-off-by: Christian Wolf <[email protected]>

christianlupus · 2024-10-09T22:47:26Z

Updated text:

github-actions · 2024-10-18T02:39:11Z

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

christianlupus added 2 commits October 3, 2024 20:31

Started to fix unclear statements about OCS and REST

71d81da

Signed-off-by: Christian Wolf <[email protected]>

Fix some sphinx build watrnings

94c8694

Signed-off-by: Christian Wolf <[email protected]>

christianlupus added 2. developing manual: developer labels Oct 3, 2024

provokateurin requested changes Oct 3, 2024

View reviewed changes

christianlupus and others added 2 commits October 4, 2024 00:04

Apply suggestions from code review

0758bc0

Co-authored-by: Kate <[email protected]> Signed-off-by: Christian Wolf <[email protected]>

Fix OCS description after some research

4b76fec

Signed-off-by: Christian Wolf <[email protected]>

christianlupus mentioned this pull request Oct 7, 2024

REST or OCS #12157

Open

provokateurin requested changes Oct 8, 2024

View reviewed changes

Fix text about responders

ca6794c

Signed-off-by: Christian Wolf <[email protected]>

christianlupus force-pushed the fix/clarification-rest-ocs branch from 08e4aab to ca6794c Compare October 8, 2024 22:33

Adding CORS back into the mix

d5f5e94

Signed-off-by: Christian Wolf <[email protected]>

github-actions bot added the feedback-requested label Oct 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

christianlupus commented Oct 3, 2024

christianlupus commented Oct 3, 2024

provokateurin left a comment

provokateurin Oct 3, 2024

provokateurin Oct 3, 2024

christianlupus Oct 3, 2024

provokateurin Oct 3, 2024

provokateurin Oct 3, 2024

christianlupus Oct 3, 2024

provokateurin Oct 3, 2024

provokateurin Oct 3, 2024

christianlupus commented Oct 7, 2024

provokateurin left a comment

provokateurin Oct 8, 2024

christianlupus Oct 8, 2024

provokateurin Oct 9, 2024

christianlupus Oct 9, 2024

christianlupus Oct 9, 2024

christianlupus commented Oct 9, 2024

github-actions bot commented Oct 18, 2024

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

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

Are you sure you want to change the base?

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

Conversation

christianlupus commented Oct 3, 2024

☑️ Resolves

🖼️ Screenshots

christianlupus commented Oct 3, 2024

provokateurin left a 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

christianlupus commented Oct 7, 2024

provokateurin left a 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

christianlupus commented Oct 9, 2024

github-actions bot commented Oct 18, 2024