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

Fix sharing with uids in numerical format #37336

Merged
merged 3 commits into from
May 6, 2020
Merged

Conversation

karakayasemi
Copy link
Contributor

@karakayasemi karakayasemi commented May 4, 2020

Description

Php does not allow numeric string as array key and converts these strings to integer. Share autocompletion api returns numerical uids as integer because of the desribed behavior of php.

When autcomplete share api response directly used in ownCloud clients without type checking, it leds to problem when sharing with numerical uids. This PR fixes api response.

Related Issue

How Has This Been Tested?

  • Acceptance tests added.

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

@codecov
Copy link

codecov bot commented May 4, 2020

Codecov Report

Merging #37336 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #37336   +/-   ##
=========================================
  Coverage     64.54%   64.55%           
- Complexity    19217    19218    +1     
=========================================
  Files          1268     1268           
  Lines         75086    75104   +18     
  Branches       1331     1331           
=========================================
+ Hits          48466    48484   +18     
  Misses        26228    26228           
  Partials        392      392           
Flag Coverage Δ Complexity Δ
#javascript 54.14% <ø> (ø) 0.00 <ø> (ø)
#phpunit 65.71% <100.00%> (+<0.01%) 19218.00 <0.00> (+1.00)
Impacted Files Coverage Δ Complexity Δ
...es_sharing/lib/Controller/Share20OcsController.php 93.86% <100.00%> (+0.40%) 210.00 <0.00> (+1.00)
...files_sharing/lib/Controller/ShareesController.php 90.63% <100.00%> (+0.03%) 105.00 <0.00> (ø)
lib/private/Share20/DefaultShareProvider.php 97.74% <0.00%> (-0.16%) 120.00% <0.00%> (ø%)
settings/routes.php 100.00% <0.00%> (ø) 0.00% <0.00%> (ø%)
lib/private/OCS/Provider.php 100.00% <0.00%> (ø) 5.00% <0.00%> (ø%)
apps/files/appinfo/routes.php 100.00% <0.00%> (ø) 0.00% <0.00%> (ø%)
apps/files/lib/Capabilities.php 100.00% <0.00%> (ø) 2.00% <0.00%> (ø%)
apps/federation/appinfo/routes.php 100.00% <0.00%> (ø) 0.00% <0.00%> (ø%)
apps/dav/lib/Files/Xml/FilterRequest.php 0.00% <0.00%> (ø) 13.00% <0.00%> (ø%)
apps/federatedfilesharing/appinfo/routes.php 100.00% <0.00%> (ø) 0.00% <0.00%> (ø%)
... and 7 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 e5ab8b0...2effe65. Read the comment docs.

@phil-davis
Copy link
Contributor

This fixes the webUI by always sending it string UID - good, the webUI client works.
But if some other client still makes a mess of string UIDs and sends back an API request to shareWith an integer then it still breaks:

curl --user jim:jim http://172.17.0.1:8080/ocs/v2.php/apps/files_sharing/api/v1/shares?format=json -d '{"shareType": 0, "shareWith": 123, "permissions": 31, "path": "/faaa"}' -X POST -H "Content-Type: application/json"
{"ocs":{"meta":{"status":"failure","statuscode":500,"message":"","totalitems":"","itemsperpage":""},"data":[]}}

IMO when also want to, when we receive an incoming API request, either:

  1. "stringify" whatever is in shareWith and continue to process the share request, or;
  2. If shareWith is not a string, then catch the exception and respond with some 4nn error code (that will let the API client know that their request is not valid, without giving them a 500)

@karakayasemi
Copy link
Contributor Author

@phil-davis according to IShare interface description, it is not throwing any exception. Since Share20OcsController.php has already request parameter validation, I extended these validations with is_string check instead of catching exception.

@phil-davis phil-davis self-requested a review May 5, 2020 13:29
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.

LGTM

@phil-davis
Copy link
Contributor

I added skip tags to the changed acceptance test scenario, because that test scenario will not pass on old ownCloud releases.

@phil-davis phil-davis merged commit a43d942 into master May 6, 2020
@delete-merged-branch delete-merged-branch bot deleted the fix-numeric-uid-share branch May 6, 2020 06:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't share with user that only has numbers as name
3 participants