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

Add repair step to fix file share permissions #26562

Merged
merged 1 commit into from
Nov 14, 2016
Merged

Conversation

PVince81
Copy link
Contributor

@PVince81 PVince81 commented Nov 7, 2016

Description

Related Issue

Fixes for #26541.

Motivation and Context

See issue

How Has This Been Tested?

Unit test.

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:

  • stable9.1
  • stable9

@jvillafanez

@mention-bot
Copy link

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

$builder = $this->connection->getQueryBuilder();
$builder
->update('share')
->set('permissions', $builder->createFunction('permissions & ' . $mask))
Copy link
Member

Choose a reason for hiding this comment

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

❗️
According to http://www.orafaq.com/wiki/Bit , Oracle doesn't support that operator. I'm not sure if this is somehow patched in the DB layer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, when writing this I wasn't sure but wanted to let it run on CI... CI will confirm.
If we don't have bitmask operations then it will be much more painful to implement.
I'm not happy to have to first gather all shares and then iterate over each and do the combination on PHP level.

Copy link
Contributor

@nickvergessen nickvergessen Nov 7, 2016

Choose a reason for hiding this comment

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

To fix postgres $builder->createFunction('(' . $builder->getColumnName('permissions') . ' & ' . $mask . ')')) should be enough.

But I guess to correctly fix this, you should add a new method to IExpressionBuilder. Then Oracle can haz it's own fucked up implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the hints. Doing this inline for now, let's see if the tests pass...

@PVince81
Copy link
Contributor Author

PVince81 commented Nov 7, 2016

Hah, look... PostgreSQL failed first:

1) Test\Repair\RepairInvalidSharesTest::testRemoveExpirationDateForNonLinkShares
12:57:19 Doctrine\DBAL\Exception\DriverException: An exception occurred while executing 'UPDATE "oc_share" SET "permissions" = permissions & 19 WHERE ("item_type" = 'file') AND ("permissions" <> permissions & 19)':
12:57:19 
12:57:19 SQLSTATE[42883]: Undefined function: 7 ERROR:  operator does not exist: boolean & integer
12:57:19 LINE 1: ...item_type" = 'file') AND ("permissions" <> permissions & 19)
12:57:19                                                                   ^
12:57:19 HINT:  No operator matches the given name and argument type(s). You might need to add explicit type casts.
12:57:19 
12:57:19 /var/lib/jenkins/workspace/owncloud-core_core_PR-26562-37KEVOZ7VANINVKDU34LIJR7BDYJA5AB65ORYZHXL7OTWVZVE2DQ/lib/composer/doctrine/dbal/lib/Doctrine/DBAL/Driver/AbstractPostgreSQLDriver.php:91
12:57:19 /var/lib/jenkins/workspace/owncloud-core_core_PR-26562-37KEVOZ7VANINVKDU34LIJR7BDYJA5AB65ORYZHXL7OTWVZVE2DQ/lib/composer/doctrine/dbal/lib/Doctrine/DBAL/DBALException.php:128
12:57:19 /var/lib/jenkins/workspace/owncloud-core_core_PR-26562-37KEVOZ7VANINVKDU34LIJR7BDYJA5AB65ORYZHXL7OTWVZVE2DQ/lib/composer/doctrine/dbal/lib/Doctrine/DBAL/Connection.php:996
12:57:19 /var/lib/jenkins/workspace/owncloud-core_core_PR-26562-37KEVOZ7VANINVKDU34LIJR7BDYJA5AB65ORYZHXL7OTWVZVE2DQ/lib/private/DB/Connection.php:209
12:57:19 /var/lib/jenkins/workspace/owncloud-core_core_PR-26562-37KEVOZ7VANINVKDU34LIJR7BDYJA5AB65ORYZHXL7OTWVZVE2DQ/lib/composer/doctrine/dbal/lib/Doctrine/DBAL/Query/QueryBuilder.php:208
12:57:19 /var/lib/jenkins/workspace/owncloud-core_core_PR-26562-37KEVOZ7VANINVKDU34LIJR7BDYJA5AB65ORYZHXL7OTWVZVE2DQ/lib/private/DB/QueryBuilder/QueryBuilder.php:141
12:57:19 /var/lib/jenkins/workspace/owncloud-core_core_PR-26562-37KEVOZ7VANINVKDU34LIJR7BDYJA5AB65ORYZHXL7OTWVZVE2DQ/lib/private/Repair/RepairInvalidShares.php:105
12:57:19 /var/lib/jenkins/workspace/owncloud-core_core_PR-26562-37KEVOZ7VANINVKDU34LIJR7BDYJA5AB65ORYZHXL7OTWVZVE2DQ/lib/private/Repair/RepairInvalidShares.php:158
12:57:19 /var/lib/jenkins/workspace/owncloud-core_core_PR-26562-37KEVOZ7VANINVKDU34LIJR7BDYJA5AB65ORYZHXL7OTWVZVE2DQ/tests/lib/Repair/RepairInvalidSharesTest.php:108

@PVince81
Copy link
Contributor Author

PVince81 commented Nov 7, 2016

Looks like PostgreSQL will needs its own exception too: 😱

17:12:45 SQLSTATE[42883]: Undefined function: 7 ERROR:  operator does not exist: boolean & integer
17:12:45 LINE 1: ...em_type" = 'file') AND ("permissions" <> "permissions" & 19)

@nickvergessen
Copy link
Contributor

(Need (more (brackets)))

@PVince81
Copy link
Contributor Author

PVince81 commented Nov 7, 2016

Added cast(XYZ as int) as per https://www.postgresql.org/docs/9.4/static/functions-bitstring.html.
Seems to work for sqlite and mysql as well.

Let's see...

);
} else {
$permsFunc = $builder->createFunction(
'cast(' . $builder->getColumnName('permissions') . ' as int) & ' . $mask
Copy link
Contributor

Choose a reason for hiding this comment

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

As commented before '(' . $builder->getColumnName('permissions') . ' & ' . $mask . ')' is what you need...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to wait for another test run. Oh well, it failed again.
Will try with extra safety brackets.

@PVince81 PVince81 force-pushed the fix-invalid-share-perms branch from 3a5e859 to b99f808 Compare November 10, 2016 15:37
@PVince81
Copy link
Contributor Author

The magic has worked !

Now rebased+squashed.

@PVince81
Copy link
Contributor Author

@jvillafanez review ?

@jvillafanez
Copy link
Member

👍

@PVince81
Copy link
Contributor Author

stable9.1: #26617
stable9: #26618

@lock
Copy link

lock bot commented Aug 4, 2019

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.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants