Repair job to fix permissions for avatars#24898
Merged
Conversation
|
By analyzing the blame information on this pull request, we identified @DeepDiver1975, @PVince81 and @nickvergessen to be potential reviewers |
d6da6a7 to
dbdaf56
Compare
Contributor
|
Great fix. Makes sense to backport 👍 |
Contributor
There was a problem hiding this comment.
Column not found: 1054 Unknown column 'home::%' in 'where clause'
Needs to be a parameter in qb2
Contributor
There was a problem hiding this comment.
Or see
core/lib/private/SystemTag/SystemTagManager.php
Lines 143 to 146 in 8fbb63d
Contributor
Author
|
All fixed. Review time! |
Contributor
|
Looks good, but lets wait for tests |
Contributor
|
Guess which one failed ? |
Contributor
Author
|
of course... man... oracle... will take care... |
Contributor
Author
|
Ah right the platform stuff... yeah lets fix that indeed... |
Contributor
Author
|
Ok now using platform... |
Contributor
|
looks like pgsql had a hiccup 😦 |
Contributor
Author
|
Rebased... |
Contributor
Author
|
Postgres is still having trouble... but is unrelated to this PR from my POV... final review time |
| ->where($path) | ||
| ->andWhere($qb2->expr()->in('storage', $qb2->createFunction($qb->getSQL()))) | ||
| ->andWhere($qb2->expr()->neq('permissions', $qb2->createNamedParameter(23))) | ||
| ->setParameter('like', 'home::%'); |
Contributor
There was a problem hiding this comment.
ew, this will not work with LDAP and long storage IDs 😢
Contributor
There was a problem hiding this comment.
you will need to use the Storage/Cache API to find the matching storage and adjust permissions
Contributor
Author
There was a problem hiding this comment.
why? ldap also has home::.... right?
Contributor
There was a problem hiding this comment.
there is some twisted logic in the storage id code: if the actual storage id string is bigger than the column width (64?) then it will convert it to the md5 of the storage id instead. With LDAP user ids it is more likely to happen.
See https://github.com/owncloud/core/wiki/Storage-IDs#random-numberstring
It has and is still a nightmare...
Contributor
There was a problem hiding this comment.
hmpf, should have the same as with share:: that is keeping the prefix outside of the md5.
Contributor
Author
There was a problem hiding this comment.
O that is nasty... I still propose to merge this and have a separate job that does the query magic...
Contributor
There was a problem hiding this comment.
second job could search for "not starting with home::, local:: and shared::" and then only analyze those, but I agree, lets get this in and continue in a second PR
Contributor
There was a problem hiding this comment.
A second job to fill in the gaps ? Ideally there should be only one job that does the work to its full extent. Because else later if a dev fixes a bug in one, they might forget the second one...
The sad part is that it requires iterating over users https://github.com/owncloud/core/blob/v9.0.2/lib/private/repair/repairlegacystorages.php#L214
Contributor
There was a problem hiding this comment.
Okay, the piece that iterates over user should be added in this same job, not in a separate one.
In this case this PR is acceptable as a start.
Contributor
|
@rullzer how to test ? I guess manually set the permissions wrong in the database ? |
Contributor
Author
|
@PVince81 yes... the only way indeed... look at the test so see. |
Fixes #22978 On some older installations the permissions for the userRoot and the avatars are not correct. This breaks since we now use the Node API in the avatar code. This repair job makes sure that the permissions are set correctly. * Unit tests added
Contributor
Author
|
Rebased so solve conflict |
Contributor
|
Tested, works 👍 I set the permissions to 0 on the user's storage root and also on the avatar files. The UI would not show them any more. Then I ran the upgrade (had to increase version.php) with this PR and the avatars were back. |
Contributor
|
@rullzer please prepare the backport PR for 9.0 |
rullzer
added a commit
that referenced
this pull request
Jun 10, 2016
Contributor
Author
|
Stable9 in #25068 |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #22978
On some older installations the permissions for the userRoot and the
avatars are not correct. This breaks since we now use the Node API in
the avatar code.
This repair job makes sure that the permissions are set correctly.
CC: @PVince81 @nickvergessen @icewind1991 @blizzz
@karlitschek should be backported to 9.0 since that is where the issue originally happend.