X Tutup
Skip to content

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

Merged
VicDeo merged 4 commits intomasterfrom
ocs-to-routes
Jan 21, 2021
Merged

Use OcsController and routes instead of API::register and static methods#37272
VicDeo merged 4 commits intomasterfrom
ocs-to-routes

Conversation

@VicDeo
Copy link
Member

@VicDeo VicDeo commented Apr 16, 2020 •
edited
Loading

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:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

@VicDeo VicDeo self-assigned this Apr 16, 2020
@VicDeo VicDeo changed the title Ocs to routes Use OcsController and routes instead of API::register and static methods Apr 16, 2020
@VicDeo VicDeo force-pushed the ocs-to-routes branch 4 times, most recently from 7f6cf8e to a00def3 Compare April 20, 2020 12:38
@VicDeo VicDeo requested a review from phil-davis April 20, 2020 14:32
@codecov
Copy link

codecov bot commented Apr 20, 2020 •
edited
Loading

Codecov Report

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

Impacted file tree graph

@@             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.

@VicDeo VicDeo force-pushed the ocs-to-routes branch 2 times, most recently from aa2be47 to e1e87a5 Compare April 24, 2020 13:06
@VicDeo VicDeo added this to the development milestone Apr 24, 2020
@VicDeo VicDeo requested a review from DeepDiver1975 April 24, 2020 13:11
@VicDeo VicDeo force-pushed the ocs-to-routes branch 4 times, most recently from 3b243e5 to 4b4102b Compare April 30, 2020 13:01
@VicDeo VicDeo force-pushed the ocs-to-routes branch 2 times, most recently from f69da89 to 3e29f25 Compare May 6, 2020 09:52
@jvillafanez
Copy link
Member

jvillafanez commented May 6, 2020

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 VicDeo force-pushed the ocs-to-routes branch 2 times, most recently from bf70b74 to 04fc3e9 Compare May 14, 2020 12:58
@VicDeo
Copy link
Member Author

VicDeo commented Jun 17, 2020 •
edited
Loading

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
phil-davis reviewed Nov 18, 2020
View reviewed changes
Copy link
Contributor

@phil-davis phil-davis left a comment

Choose a reason for hiding this comment

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

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

@jvillafanez
Copy link
Member

jvillafanez commented Nov 19, 2020

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
Copy link
Member Author

VicDeo commented Nov 19, 2020

@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
Copy link
Member Author

VicDeo commented Nov 19, 2020 •
edited
Loading

@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
Copy link
Contributor

C0rby commented Nov 19, 2020

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
Copy link
Member Author

VicDeo commented Nov 19, 2020 •
edited
Loading

@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
Copy link
Member

jvillafanez commented Nov 24, 2020

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

@VicDeo
Copy link
Member Author

VicDeo commented Dec 15, 2020 •
edited
Loading

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
Copy link
Contributor

C0rby commented Dec 15, 2020

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

@sonarqubecloud

This comment has been minimized.

Sign in to view
@VicDeo VicDeo requested a review from jvillafanez January 20, 2021 10:12
@VicDeo
Copy link
Member Author

VicDeo commented Jan 20, 2021

Let's merge it finally

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jan 20, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@VicDeo VicDeo merged commit 6f1420f into master Jan 21, 2021
@delete-merged-branch delete-merged-branch bot deleted the ocs-to-routes branch January 21, 2021 09:43
This was referenced Jan 26, 2021
Closed
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@phil-davis phil-davis phil-davis left review comments

@jvillafanez jvillafanez jvillafanez approved these changes

@DeepDiver1975 DeepDiver1975 Awaiting requested review from DeepDiver1975

Assignees

@VicDeo VicDeo

Labels

3 - To Review

Projects

None yet

Milestone

development

Development

Successfully merging this pull request may close these issues.

ocs config endpoint does not return CORS headers

7 participants

@VicDeo @jvillafanez @micbar @phil-davis @mmattel @C0rby @DeepDiver1975
X Tutup