Convert shareWith to lower case by sharidas · Pull Request #35084 · owncloud/core

sharidas · 2019-04-23T17:04:00Z

Convert shareWith to lower case when share is created.
This would help to make it consistent across oC.

Signed-off-by: Sujith H sharidasan@owncloud.com

Description

Convert shareWith to lower case. The share API does not convert it to lower case, and as a result, it would cause inconsistency: if the API is used by passing shareWith with a mix of upper and lower case. The share would be inaccessible. This was found with guest user.

Related Issue

Fixes <issue_link>

Motivation and Context

Convert shareWith to lower case in the Share20OcsController::createShare(). If its not converted the share might be inaccessible to the user. This was found with guest user.

How Has This Been Tested?

Create guest user
Set the guest user password by accessing the link from email
The guest share is accessible from the webUI by logging in as the guest user.

Screenshots (if appropriate):

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:

Open tasks:

Backport (if applicable set "backport-request" label and remove when the backport was done)

codecov · 2019-04-24T05:45:27Z

Codecov Report

Merging #35084 into master will decrease coverage by 17.07%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##             master   #35084       +/-   ##
=============================================
- Coverage     65.37%   48.29%   -17.08%     
=============================================
  Files          1215      109     -1106     
  Lines         70532    10580    -59952     
  Branches       1295     1295               
=============================================
- Hits          46112     5110    -41002     
+ Misses        24046     5096    -18950     
  Partials        374      374

Flag	Coverage Δ	Complexity Δ
#javascript	`52.85% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`38.17% <ø> (-28.65%)`	`0 <ø> (-18626)`

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Files/Storage/DAV.php	`59.45% <0%> (-21.64%)`	`0% <0%> (ø)`
apps/updatenotification/templates/admin.php
lib/private/Encryption/Keys/Storage.php
lib/private/App/CodeChecker/NodeVisitor.php
lib/private/RedisFactory.php
apps/dav/lib/Avatars/AvatarNode.php
...s/dav/appinfo/Migrations/Version20170202213905.php
apps/dav/lib/Upload/ChunkLocationProvider.php
apps/files/lib/AppInfo/Application.php
apps/systemtags/list.php
... and 1097 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 dcab5a4...e909d72. Read the comment docs.

codecov · 2019-04-24T05:45:28Z

Codecov Report

Merging #35084 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master   #35084      +/-   ##
============================================
+ Coverage     65.45%   65.45%   +<.01%     
- Complexity    18631    18633       +2     
============================================
  Files          1215     1215              
  Lines         70545    70547       +2     
  Branches       1296     1296              
============================================
+ Hits          46174    46177       +3     
+ Misses        23994    23993       -1     
  Partials        377      377

Flag	Coverage Δ	Complexity Δ
#javascript	`53.49% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`66.83% <100%> (ø)`	`18633 <0> (+2)`	⬆️

Impacted Files	Coverage Δ	Complexity Δ
...es_sharing/lib/Controller/Share20OcsController.php	`85.89% <100%> (+0.04%)`	`205 <0> (+2)`	⬆️
lib/private/Server.php	`86.68% <0%> (+0.11%)`	`253% <0%> (ø)`	⬇️

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 8f3d6c8...64c5103. Read the comment docs.

sharidas · 2019-04-25T07:23:44Z

The drone failure https://drone.owncloud.com/owncloud/core/16994/275, when I try to reproduce in my instance

sujith@sujith-ownCloud  /tmp/test  ➦ c7502860 ●  phpdbg -d memory_limit=4096M -rr  ./lib/composer/bin/phpunit --configuration tests/phpunit-autotest.xml
[Welcome to phpdbg, the interactive PHP debugger, v0.5.0]
To get help using phpdbg type "help" and press enter
[Please report bugs to <http://bugs.php.net/report.php>]
[PHP Warning:  require_once(/tmp/test/apps/dav/tests/unit/Connector/Sabre/CopyEtagHeaderPluginTest.php): failed to open stream: Too many open files in /tmp/test/tests/apps.php on line 23]
[PHP Stack trace:]
[PHP   1. {main}() /tmp/test/lib/composer/phpunit/phpunit/phpunit:0]
[PHP Fatal error:  require_once(): Failed opening required '/tmp/test/apps/dav/tests/unit/Connector/Sabre/CopyEtagHeaderPluginTest.php' (include_path='/tmp/test/apps:/tmp/test/apps-external') in /tmp/test/tests/apps.php on line 23]
[PHP Stack trace:]
[PHP   1. {main}() /tmp/test/lib/composer/phpunit/phpunit/phpunit:0]
[PHP Warning:  include(/tmp/test/lib/private/TempManager.php): failed to open stream: Too many open files in /tmp/test/lib/composer/composer/ClassLoader.php on line 444]
[PHP Stack trace:]
[PHP   1. {main}() /tmp/test/lib/composer/phpunit/phpunit/phpunit:0]
[PHP   2. spl_autoload_call() /tmp/test/lib/private/Server.php:580]
[PHP Warning:  include(/tmp/test/lib/private/TempManager.php): failed to open stream: Too many open files in /tmp/test/lib/composer/composer/ClassLoader.php on line 444]
[PHP Stack trace:]
[PHP   1. {main}() /tmp/test/lib/composer/phpunit/phpunit/phpunit:0]
[PHP   2. spl_autoload_call() /tmp/test/lib/private/Server.php:580]
[PHP Warning:  include(): Failed opening '/tmp/test/lib/private/TempManager.php' for inclusion (include_path='/tmp/test/apps:/tmp/test/apps-external') in /tmp/test/lib/composer/composer/ClassLoader.php on line 444]
[PHP Stack trace:]
[PHP   1. {main}() /tmp/test/lib/composer/phpunit/phpunit/phpunit:0]
[PHP   2. spl_autoload_call() /tmp/test/lib/private/Server.php:580]
[PHP Fatal error:  Uncaught Error: Class 'OC\TempManager' not found in /tmp/test/lib/private/Server.php:580
Stack trace:
#0 /tmp/test/lib/composer/pimple/pimple/src/Pimple/Container.php(118): OC\Server->OC\{closure}(Object(OC\Server))
#1 /tmp/test/lib/private/AppFramework/Utility/SimpleContainer.php(108): Pimple\Container->offsetGet('TempManager')
#2 /tmp/test/lib/private/ServerContainer.php(86): OC\AppFramework\Utility\SimpleContainer->query('TempManager')
#3 /tmp/test/lib/private/Server.php(1409): OC\ServerContainer->query('TempManager')
#4 /tmp/test/lib/base.php(667): OC\Server->getTempManager()
#5 /tmp/test/lib/private/Shutdown/ShutDownManager.php(61): OC::{closure}()
#6 [internal function]: OC\Shutdown\ShutDownManager->run()
#7 {main}
  thrown in /tmp/test/lib/private/Server.php on line 580]
[PHP Stack trace:]
[PHP   1. {main}() /tmp/test/lib/composer/phpunit/phpunit/phpunit:0]
 ✘ sujith@sujith-ownCloud  /tmp/test  ➦ c7502860 ● 

Most likely phpdbg is not working. Because this is not only with object storage enabled + testing enabled, even if I disable object storage and run the command the result remains same. After doing research most likely I suspect http://git.php.net/?p=php-src.git;a=commitdiff;h=b8b880932e95ff1e575a04a6988bc0cf00b89842;hp=2e9dccef78d169be9a4a37c813e0b5d624bd5ac7 is missing in my instance :( ( Verified from the source of the package installed ).

Convert shareWith to lower case when share is created. This would help to make it consistent across oC. Signed-off-by: Sujith H <sharidasan@owncloud.com>

VicDeo

Looks good

phil-davis · 2019-04-25T22:13:23Z

@sharidas you removed the unit test case testLowerCaseShareWithCreateShare()
Did you manage to find out why it was causing the phpdbg out of memory errors?
(it would be nice if we understood which code in that test case was exploding the unit test framework, so that we don't use lots of time again in future with this sort of test fail)

sharidas · 2019-04-26T04:49:09Z

@phil-davis No, I couldn't :(

PVince81 · 2019-04-26T09:12:52Z

let's wait for @micbar to do some smoke test

micbar · 2019-05-02T07:39:35Z

Manual Testing

1 Test uppercase Username will be converted to lowercase

curl -X POST \
  http://localhost/ocs/v1.php/apps/files_sharing/api/v1/shares \
  -H 'Authorization: Basic YWRtaW46YWRtaW4=' \
  -d '{
	"name" : "Test Link",
	"path" : "welcome.txt",
	"shareType" : "0",
	"permissions" : "15",
	"expireDate" : "",
	"shareWith" : "TEST1"
}'

Response:

<?xml version="1.0"?>
<ocs>
    <meta>
        <status>ok</status>
        <statuscode>100</statuscode>
        <message/>
        <totalitems></totalitems>
        <itemsperpage></itemsperpage>
    </meta>
    <data>
        <id>5</id>
        <share_type>0</share_type>
        <uid_owner>admin</uid_owner>
        <displayname_owner>admin</displayname_owner>
        <permissions>3</permissions>
        <stime>1556781451</stime>
        <parent/>
        <expiration/>
        <token/>
        <uid_file_owner>admin</uid_file_owner>
        <displayname_file_owner>admin</displayname_file_owner>
        <path>/welcome.txt</path>
        <item_type>file</item_type>
        <mimetype>text/plain</mimetype>
        <storage_id>home::admin</storage_id>
        <storage>1</storage>
        <item_source>4</item_source>
        <file_source>4</file_source>
        <file_parent>2</file_parent>
        <file_target>/welcome.txt</file_target>
        <share_with>test1</share_with>
        <share_with_displayname>TEst1</share_with_displayname>
        <share_with_additional_info/>
        <mail_send>0</mail_send>
        <attributes/>
    </data>
</ocs>

2 Test uppercase GroupName will be not be converted to lowercase

curl -X POST \
  http://localhost/ocs/v1.php/apps/files_sharing/api/v1/shares \
  -H 'Authorization: Basic YWRtaW46YWRtaW4=' \
  -d '{
	"name" : "Test Link",
	"path" : "welcome.txt",
	"shareType" : "1",
	"permissions" : "15",
	"expireDate" : "",
	"shareWith" : "People"
}'

Response:

<?xml version="1.0"?>
<ocs>
    <meta>
        <status>ok</status>
        <statuscode>100</statuscode>
        <message/>
        <totalitems></totalitems>
        <itemsperpage></itemsperpage>
    </meta>
    <data>
        <id>6</id>
        <share_type>1</share_type>
        <uid_owner>admin</uid_owner>
        <displayname_owner>admin</displayname_owner>
        <permissions>3</permissions>
        <stime>1556782677</stime>
        <parent/>
        <expiration/>
        <token/>
        <uid_file_owner>admin</uid_file_owner>
        <displayname_file_owner>admin</displayname_file_owner>
        <path>/welcome.txt</path>
        <item_type>file</item_type>
        <mimetype>text/plain</mimetype>
        <storage_id>home::admin</storage_id>
        <storage>1</storage>
        <item_source>4</item_source>
        <file_source>4</file_source>
        <file_parent>2</file_parent>
        <file_target>/welcome.txt</file_target>
        <share_with>People</share_with>
        <share_with_displayname>People</share_with_displayname>
        <mail_send>0</mail_send>
        <attributes/>
    </data>
</ocs>

micbar

Manual Testing successful

sharidas · 2019-05-02T08:08:23Z

Backport PR to stable10 branch : #35138

sharidas added the 2 - Developing label Apr 23, 2019

sharidas added this to the development milestone Apr 23, 2019

sharidas self-assigned this Apr 23, 2019

sharidas force-pushed the convert-share-with-tolowercase-inshare-api branch from cc00e9f to e909d72 Compare April 24, 2019 05:45

sharidas force-pushed the convert-share-with-tolowercase-inshare-api branch from e909d72 to 32742b6 Compare April 25, 2019 06:43

Convert shareWith to lower case

64c5103

Convert shareWith to lower case when share is created. This would help to make it consistent across oC. Signed-off-by: Sujith H <sharidasan@owncloud.com>

sharidas force-pushed the convert-share-with-tolowercase-inshare-api branch from 32742b6 to 64c5103 Compare April 25, 2019 12:36

sharidas requested review from PVince81 and VicDeo and removed request for VicDeo April 25, 2019 13:26

sharidas added 3 - To Review and removed 2 - Developing labels Apr 25, 2019

VicDeo approved these changes Apr 25, 2019

View reviewed changes

PVince81 requested a review from micbar April 26, 2019 09:12

phil-davis added the backport-request label Apr 29, 2019

micbar approved these changes May 2, 2019

View reviewed changes

sharidas mentioned this pull request May 2, 2019

[stable10] Backport of Convert shareWith to lower case #35138

Merged

11 tasks

sharidas removed the backport-request label May 2, 2019

sharidas merged commit 45e8578 into master May 2, 2019

delete-merged-branch Bot deleted the convert-share-with-tolowercase-inshare-api branch May 2, 2019 11:33

micbar mentioned this pull request May 31, 2019

File is not shared with user whose username contains capital letter #35382

Closed

haribhandari07 mentioned this pull request Jun 4, 2019

Sharing using email with uppercase letters in TLD owncloud/guests#222

Closed

micbar mentioned this pull request Jun 6, 2019

Revert "Convert shareWith to lower case" #35448

Merged

individual-it mentioned this pull request Jun 11, 2019

sharing via API and changing the cases in the username does not work correctly #35484

Closed

lock Bot locked as resolved and limited conversation to collaborators May 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Convert shareWith to lower case#35084

Convert shareWith to lower case#35084
sharidas merged 1 commit into
masterfrom
convert-share-with-tolowercase-inshare-api

sharidas commented Apr 23, 2019

Uh oh!

codecov Bot commented Apr 24, 2019

Uh oh!

codecov Bot commented Apr 24, 2019 •

edited

Loading

Uh oh!

sharidas commented Apr 25, 2019

Uh oh!

VicDeo left a comment

Uh oh!

phil-davis commented Apr 25, 2019

Uh oh!

sharidas commented Apr 26, 2019

Uh oh!

PVince81 commented Apr 26, 2019

Uh oh!

micbar commented May 2, 2019

Uh oh!

micbar left a comment

Uh oh!

sharidas commented May 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

sharidas commented Apr 23, 2019

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Open tasks:

Uh oh!

codecov Bot commented Apr 24, 2019

Codecov Report

Uh oh!

codecov Bot commented Apr 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sharidas commented Apr 25, 2019

Uh oh!

VicDeo left a comment

Choose a reason for hiding this comment

Uh oh!

phil-davis commented Apr 25, 2019

Uh oh!

sharidas commented Apr 26, 2019

Uh oh!

PVince81 commented Apr 26, 2019

Uh oh!

micbar commented May 2, 2019

Manual Testing

1 Test uppercase Username will be converted to lowercase

2 Test uppercase GroupName will be not be converted to lowercase

Uh oh!

micbar left a comment

Choose a reason for hiding this comment

Uh oh!

sharidas commented May 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov Bot commented Apr 24, 2019 •

edited

Loading