[stable9.1] Fix user casing in initMountPoints #26271

Merged
merged 1 commit into from Oct 18, 2016

Projects

None yet

3 participants

@PVince81
Collaborator
PVince81 commented Oct 4, 2016 edited

Description

Some calls to initMountsPoints use the wrong user casing, mostly due to passing through a user id as given as input from a URL or so. In cases where shares exist it can cause duplicate mount points to exist and cause replication of "(2)" in received share names.

This PR intends to cover all known cases already. In the event that additional mismatch cases are detected, it will log a warning with the stack trace.

Related Issue

#25718

Motivation and Context

See above

How Has This Been Tested?

To test:

  • create user "User1" and "User2" with shares between them. Request avatar with curl using lower case name. See that "(2)" doesn't appear any more
  • retest transfer ownership using different casings
  • retest apps that user getUserFolder() with wrong casings
  • retest getUsersSharingFile => done by editing text file from multiple sharees with encryption enabled

TODO:

  • add unit test for getUserFolder() with different casings
  • add unit test for initMountPoints

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Backports

  • forward port to master / 9.2
@PVince81 PVince81 added this to the 9.1.2 milestone Oct 4, 2016
@mention-bot

@PVince81, thanks for your PR! By analyzing the history of the files in this pull request, we identified @DeepDiver1975, @icewind1991 and @schiessle to be potential reviewers.

@PVince81
Collaborator
PVince81 commented Oct 4, 2016

Okay, adjusted the tests and did some manual testing, looks fine.

Please review @jvillafanez @DeepDiver1975 @butonic @VicDeo

@PVince81
Collaborator
PVince81 commented Oct 4, 2016

Steps to reproduce the original issue:

  1. Create two users "User1" and "User2"
  2. Share a folder from User1 to User2 and another one from User2 to User1
  3. Set an avatar for both users
  4. Query the avatar for one of the users but use the wrong casing for the user name: curl -D - -u User1:test http://localhost/owncloud/index.php/avatar/user2/32 -o avatar.png.
  5. Refresh the file view in the web UI
  6. See that a "(2)" is appended at the end of the received folder name every time the avatar was queried
@jvillafanez
Contributor

The code seems to rely on the DB backend, which does some magic converting the uids to lowercase. Other backends (maybe LDAP) might not do this and just do the matching in a case-sensitive way. I'm not sure if this is taken into account.

lib/private/Files/Filesystem.php
+ if ($user !== $userObject->getUID()) {
+ $dummyException = new \Exception();
+ $stack = $dummyException->getTraceAsString();
+ \OCP\Util::writeLog('files', 'initMountPoints() called with wrong user casing. This could be a bug. Expected: "' . $userObject->getUID() . '" got "' . $user . '". Stack: ' . $stack, \OCP\Util::WARN);
@jvillafanez
jvillafanez Oct 4, 2016 Contributor

I don't know if this is the best way to handle this.

  • Is the stacktrace needed? Maybe something like debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS) is cleaner, although I'm not sure about the memory consumption. It's better if we can remove the stacktrace.
@PVince81
PVince81 Oct 4, 2016 Collaborator

I want the stack trace to be able to find out which pieces of code are still calling initMountPoints without using the correct user id. I expect that this will never happen, as `initMountPoints' is private. But who knows what other bundled apps might be using it.

@PVince81
PVince81 Oct 4, 2016 Collaborator

I'll have a try. If I set it to null afterwards it should free memory ?

@PVince81
Collaborator
PVince81 commented Oct 4, 2016

and just do the matching in a case-sensitive way. I'm not sure if this is taken into account.

Good point. I think it should work fine with this PR. The PR doesn't do any casing magic and simply always uses the user id from the object as returned by the user manager instead of the user id given to user manager get(). This way we're sure that the user id we use is the real one.

In the case where a case sensitive user manager is used, a lot of code would just throw a NoUserException when passing a user id with different casing because the user doesn't exist.

@PVince81
Collaborator
PVince81 commented Oct 4, 2016
@PVince81
Collaborator
PVince81 commented Oct 5, 2016

Fixed unit tests in a way that should make them run more reliably and not depend on the last dirty test's registered user backend...

@PVince81
Collaborator
PVince81 commented Oct 5, 2016

Argh, that user manager crap is likely having side effects on tests that assume we're using the default one:

Test\Files\Storage\HomeStorageQuotaTest::testHomeStorageWrapperWithQuota
Failed asserting that false is true.

/var/lib/jenkins/workspace/owncloud-core_core_PR-26271-7CRPTZSQ5YJGGDTXDH4SEHLDTGYR7R4TIPB2L3AEU5HMIL6NU5RQ/tests/lib/Files/Storage/HomeStorageQuotaTest.php:66
@PVince81
Collaborator
PVince81 commented Oct 5, 2016

This is becoming a nightmare. One does not simply write new tests that replace the default user manager without creating random side effects that depend on test order. Raised #26280.

@PVince81
Collaborator
PVince81 commented Oct 5, 2016

Tests finally pass!

@jvillafanez
Contributor

Except from my previous comment, code looks good (I haven't tested it)

@PVince81
Collaborator
PVince81 commented Oct 6, 2016

Except from my previous comment

I fixed it to use debug_stacktrace, is the change acceptable ?

@jvillafanez
Contributor

I fixed it to use debug_stacktrace, is the change acceptable ?

I'm not seeing the change ๐Ÿ˜•

@PVince81
Collaborator

uh oh, rebase monster ate it apparently ๐Ÿ˜ข

@PVince81
Collaborator

@jvillafanez and my local git had already GC'ed, so was not able to find the commit.

Redid it and tested, please check f50d5e1

lib/private/Files/Filesystem.php
@@ -405,6 +402,17 @@ public static function initMountPoints($user = '') {
throw new \OC\User\NoUserException('Backends provided no user object for ' . $user);
}
+ // workaround in case of different casings
+ if ($user !== $userObject->getUID()) {
+ $stack = json_encode(debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS));
@jvillafanez
jvillafanez Oct 11, 2016 Contributor

Maybe we should limit the number of frames we get here to prevent fireworks because we don't know how big the stack trace will be.

@PVince81
PVince81 Oct 11, 2016 Collaborator

hmm, you mean in case of potential infinite recursions ?
so truncate the logged stack ?

@PVince81
Collaborator

@jvillafanez added array_slice for the debug backtrace

@PVince81
Collaborator

@jvillafanez can we merge this ?

lib/private/Files/Filesystem.php
@@ -405,6 +402,17 @@ public static function initMountPoints($user = '') {
throw new \OC\User\NoUserException('Backends provided no user object for ' . $user);
}
+ // workaround in case of different casings
+ if ($user !== $userObject->getUID()) {
+ $stack = json_encode(array_slice(debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS), 0, 50));
@jvillafanez
jvillafanez Oct 18, 2016 Contributor

the debug_backtrace function takes another optional parameter to limit the number of frames we get, instead of getting all the frames and filter them after: http://php.net/manual/en/function.debug-backtrace.php

@PVince81 PVince81 Add using casing check/fix for initMountPoints
6fe9f1d
@PVince81
Collaborator

@jvillafanez adjusted and retested

@jvillafanez
Contributor

๐Ÿ‘

@PVince81 PVince81 merged commit 23744e7 into stable9.1 Oct 18, 2016

3 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@PVince81 PVince81 deleted the stable9.1-initmountpoints-userid-casing branch Oct 18, 2016
@PVince81
Collaborator

master: #26398

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment