Use OcsController and routes instead of API::register and static methods #37272

VicDeo · 2020-04-16T16:26:11Z

Description

Legacy classes removed
a separate file for ocs routes removed
ocs routes are registered in core using $application->registerRoutes instead of API::register
these routes use non-static controller
use DI for this controller
proper unit tests

Related Issue

Fixes #34664
Partially covers
#12454

How Has This Been Tested?

with curl

* set a key
* test: curl http://login:passwd@oc/core/ocs/v1.php/privatedata/setattribute/testy/123  --data "value=foobar"
	 
* read keys
* test: curl http://login:passwd@oc/core/ocs/v1.php/privatedata/getattribute
          curl http://login:passwd@oc/core/ocs/v1.php/privatedata/getattribute/testy
          curl http://login:passwd@oc/core/ocs/v1.php/privatedata/getattribute/testy/123

* delete a key
* test: curl http://login:passwd@oc/core/ocs/v1.php/privatedata/deleteattribute/testy/123 --data "post=1"

Case 2:

php occ  user:setting admin core domains --value '["http:\/\/abcd.com"]'
curl -u admin:admin http://localhost/owncloud/ocs/v1.php/config -H "Origin: http://abcd.com" -v

should contain
Access-Control-Allow-Origin: http://abcd.com

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Database schema changes (next release will require increase of minor version instead of patch)
Breaking change (fix or feature that would cause existing functionality to change)
Technical debt
Tests only (no source changes)

Checklist:

codecov · 2020-04-20T15:20:38Z

Codecov Report

Merging #37272 (bb14b4c) into master (f7bcdce) will decrease coverage by 0.47%.
The diff coverage is 84.61%.

@@             Coverage Diff              @@
##             master   #37272      +/-   ##
============================================
- Coverage     65.22%   64.74%   -0.48%     
+ Complexity    19445    19405      -40     
============================================
  Files          1286     1282       -4     
  Lines         76031    75793     -238     
  Branches       1336     1336              
============================================
- Hits          49590    49072     -518     
+ Misses        26441    26327     -114     
- Partials          0      394     +394

Flag	Coverage Δ	Complexity Δ
javascript	`54.06% <ø> (-5.23%)`	`0.00 <ø> (ø)`
phpunit	`65.92% <84.61%> (+0.04%)`	`0.00 <15.00> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
core/routes.php	`50.00% <ø> (ø)`	`0.00 <0.00> (ø)`
lib/private/Route/Router.php	`68.55% <ø> (-0.20%)`	`54.00 <0.00> (ø)`
core/Controller/OcsController.php	`84.61% <84.61%> (ø)`	`15.00 <15.00> (?)`
core/js/oc-backbone.js	`50.00% <0.00%> (-50.00%)`	`0.00% <0.00%> (ø%)`
apps/comments/js/app.js	`66.66% <0.00%> (-33.34%)`	`0.00% <0.00%> (ø%)`
core/js/oc-requesttoken.js	`75.00% <0.00%> (-25.00%)`	`0.00% <0.00%> (ø%)`
core/js/systemtags/systemtagsmappingcollection.js	`47.36% <0.00%> (-21.06%)`	`0.00% <0.00%> (ø%)`
core/js/systemtags/systemtagmodel.js	`72.72% <0.00%> (-18.19%)`	`0.00% <0.00%> (ø%)`
core/js/sharescollection.js	`42.85% <0.00%> (-14.29%)`	`0.00% <0.00%> (ø%)`
apps/comments/js/commentcollection.js	`72.72% <0.00%> (-13.64%)`	`0.00% <0.00%> (ø%)`
... and 65 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 35a5eca...0a84437. Read the comment docs.

jvillafanez · 2020-05-06T12:37:13Z

It's difficult for me to justify having sql statements in a controller. I'd rather split the responsibilities in order to have one class focused on storing the info and the controller to build the response. Having an easy interface with the DB will also help to test the controller very intensively.

VicDeo · 2020-06-17T10:07:58Z

IMO Working with DB in a controller that is declared as controller explicitly is less critical than using classes in lib/private/OCS/ as controllers.

Currently lib/private/OCS/PrivateData.php and all other classes under lib/private/OCS/ are directly hooked up to API endpoints. So they are used as controllers de facto.

phil-davis

Acceptance test changes look good. Hopefully CI will agree.
@DeepDiver1975 or some dev, please review the code again.

jvillafanez · 2020-11-19T08:23:22Z

In addition to Corby's comment, I think we should include a comment somewhere stating that the code in the controller has been moved from a different place.

Honestly, if I hadn't any context, I'd say the code is quite bad to the point of make me wonder why this code exists. A comment in the controller would help to understand that we've just moved the code (barely touching it) and we opted to keep the backwards-compatibility. This hint should be enough to check the history if someone is interested.

VicDeo · 2020-11-19T08:57:57Z

@C0rby

The only thing I would check is if the annotation @NoCSRFRequired is necessary here and if not, remove the annotations.

This will fail existing acceptance tests as none of these endpoints checked CSRF token in past

VicDeo · 2020-11-19T09:20:09Z

@C0rby @DeepDiver1975 I've got your concerns after the checking API auth flow.
What if I check HTTP_OCS_APIREQUEST header presence directly in the controller actions to keep the previous behavior?

C0rby · 2020-11-19T09:45:16Z

I just saw in the description that there is a CORS header issue. So is there still work to do? @DeepDiver1975

@C0rby @DeepDiver1975 I've got your concerns after the checking API auth flow.
What if I check HTTP_OCS_APIREQUEST header presence directly in the controller actions to keep the previous behavior?

The HTTP_OCS_APIREQUEST header shouldn't be checked anymore.
It was removed sometime ago and isn't sent by all clients. #38019 is providing a CSRF check which doesn't require the header HTTP_OCS_APIREQUEST header.

VicDeo · 2020-11-19T10:54:19Z

@C0rby

It was removed sometime ago and isn't sent by all clients

not completely ;)
it is still in use https://github.com/owncloud/core/blob/master/lib/private/legacy/api.php#L378

Here I'm migrating multiple legacy API endpoints into a single controller (and remove their dependence on prehistoric OC_API class)
The only intention for me was to fix #34664 - legacy API does NOT send CORS with the response. Now it does send CORS headers
But as you see the prev (legacy) implementation depended on HTTP_OCS_APIREQUEST header. That's why I suggested to check the same header here - just to keep backward compatibility for these endpoints.

jvillafanez · 2020-11-24T07:59:08Z

Anything left to do here? I'm ok approving the current changes

VicDeo · 2020-12-15T14:23:53Z

These endpoints don't need HTTP_OCS_APIREQUEST header anymore but require CSRF token instead.
On the one hand I consider it to be a breaking change on the other hand I wonder does someone still use this API.

I guess some acceptance tests will fail with the last commit.

C0rby · 2020-12-15T14:46:53Z

The CSRF token is only checked on requests using cookie authorization.

#38019 has been merged

VicDeo · 2021-01-20T10:12:52Z

Let's merge it finally

sonarcloud · 2021-01-20T11:28:35Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

VicDeo added the 2 - Developing label Apr 16, 2020

VicDeo self-assigned this Apr 16, 2020

VicDeo force-pushed the ocs-to-routes branch from 4936408 to f081f02 Compare April 16, 2020 17:38

VicDeo changed the title ~~Ocs to routes~~ Use OcsController and routes instead of API::register and static methods Apr 16, 2020

VicDeo force-pushed the ocs-to-routes branch 4 times, most recently from 7f6cf8e to a00def3 Compare April 20, 2020 12:38

VicDeo requested a review from phil-davis April 20, 2020 14:32

VicDeo force-pushed the ocs-to-routes branch 2 times, most recently from aa2be47 to e1e87a5 Compare April 24, 2020 13:06

VicDeo added 3 - To Review and removed 2 - Developing labels Apr 24, 2020

VicDeo added this to the development milestone Apr 24, 2020

VicDeo requested a review from DeepDiver1975 April 24, 2020 13:11

VicDeo force-pushed the ocs-to-routes branch 4 times, most recently from 3b243e5 to 4b4102b Compare April 30, 2020 13:01

VicDeo force-pushed the ocs-to-routes branch 2 times, most recently from f69da89 to 3e29f25 Compare May 6, 2020 09:52

VicDeo force-pushed the ocs-to-routes branch 2 times, most recently from bf70b74 to 04fc3e9 Compare May 14, 2020 12:58

VicDeo force-pushed the ocs-to-routes branch from 04fc3e9 to aa79ef2 Compare June 17, 2020 08:10

VicDeo force-pushed the ocs-to-routes branch from aa79ef2 to 1928eb5 Compare July 31, 2020 14:24

VicDeo force-pushed the ocs-to-routes branch from 1928eb5 to bb14b4c Compare September 16, 2020 11:14

VicDeo force-pushed the ocs-to-routes branch from bb14b4c to bc3222e Compare October 8, 2020 20:24

phil-davis reviewed Nov 18, 2020

View reviewed changes

phil-davis requested a review from DeepDiver1975 November 18, 2020 10:57

VicDeo force-pushed the ocs-to-routes branch from 602e659 to 47df8c9 Compare November 19, 2020 09:11

VicDeo force-pushed the ocs-to-routes branch from 47df8c9 to 71758a7 Compare November 23, 2020 11:17

VicDeo force-pushed the ocs-to-routes branch from 71758a7 to 01609e3 Compare December 15, 2020 13:58

This comment has been minimized.

Sign in to view

VicDeo force-pushed the ocs-to-routes branch from 65e333e to 1c2b44e Compare January 19, 2021 08:18

VicDeo requested a review from jvillafanez January 20, 2021 10:12

VicDeo added 4 commits January 20, 2021 13:13

Move legacy OCS endpoints into a single OCSController

51f764f

Inject DB connection. Use upsert

ce0ec20

Update acceptance test and unit tests. Add docs

6ddbf58

Check CSRF token in OCS API endpoints

0a84437

VicDeo force-pushed the ocs-to-routes branch from 1c2b44e to 0a84437 Compare January 20, 2021 10:15

jvillafanez approved these changes Jan 21, 2021

View reviewed changes

VicDeo merged commit 6f1420f into master Jan 21, 2021

delete-merged-branch bot deleted the ocs-to-routes branch January 21, 2021 09:43

This was referenced Jan 26, 2021

Investigate nightly CI cors fails owncloud/encryption#232

Closed

[tests-only] skip refactored tests on old oC10 #38345

Merged

[tests-only] Refactor examples table tags related to PRs 38345 and 37272 #38358

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use OcsController and routes instead of API::register and static methods #37272

Use OcsController and routes instead of API::register and static methods #37272

VicDeo commented Apr 16, 2020 •

edited

Loading

codecov bot commented Apr 20, 2020 •

edited

Loading

jvillafanez commented May 6, 2020

VicDeo commented Jun 17, 2020 •

edited

Loading

phil-davis left a comment

jvillafanez commented Nov 19, 2020

VicDeo commented Nov 19, 2020

VicDeo commented Nov 19, 2020 •

edited

Loading

C0rby commented Nov 19, 2020

VicDeo commented Nov 19, 2020 •

edited

Loading

jvillafanez commented Nov 24, 2020

VicDeo commented Dec 15, 2020 •

edited

Loading

C0rby commented Dec 15, 2020

This comment has been minimized.

VicDeo commented Jan 20, 2021

sonarcloud bot commented Jan 20, 2021

Use OcsController and routes instead of API::register and static methods #37272

Use OcsController and routes instead of API::register and static methods #37272

Conversation

VicDeo commented Apr 16, 2020 • edited Loading

Description

Related Issue

How Has This Been Tested?

Case 2:

Types of changes

Checklist:

codecov bot commented Apr 20, 2020 • edited Loading

Codecov Report

jvillafanez commented May 6, 2020

VicDeo commented Jun 17, 2020 • edited Loading

phil-davis left a comment

Choose a reason for hiding this comment

jvillafanez commented Nov 19, 2020

VicDeo commented Nov 19, 2020

VicDeo commented Nov 19, 2020 • edited Loading

C0rby commented Nov 19, 2020

VicDeo commented Nov 19, 2020 • edited Loading

jvillafanez commented Nov 24, 2020

VicDeo commented Dec 15, 2020 • edited Loading

C0rby commented Dec 15, 2020

This comment has been minimized.

VicDeo commented Jan 20, 2021

sonarcloud bot commented Jan 20, 2021

VicDeo commented Apr 16, 2020 •

edited

Loading

codecov bot commented Apr 20, 2020 •

edited

Loading

VicDeo commented Jun 17, 2020 •

edited

Loading

VicDeo commented Nov 19, 2020 •

edited

Loading

VicDeo commented Nov 19, 2020 •

edited

Loading

VicDeo commented Dec 15, 2020 •

edited

Loading