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

Created a config.php option to disable local mounts for files_externa… #26887

Closed
wants to merge 3 commits into from
Closed

Conversation

grischdian
Copy link
Contributor

@grischdian grischdian commented Dec 29, 2016

…l. fixing #26653

Description

As in #26653 mentioned an option would be good to disable the 'local' option from files_external.
There is now a new Option for config.php called 'files_external_deny_local' which is set to false on default. Only if you set the option to true then the local Backend will not get registered.

Related Issue

#26653

Motivation and Context

If Sysadmin and oCadmin are not the same the Sysadmin may want to disable the local mount feature for security reasons.

How Has This Been Tested?

Compiled the actual master, installed php56 and apach24 on centos7 (Softwarecollection)
Tested without setting the option in config.php, tested with the option set to false and set to true.

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.
    Line maybe to long - but have no shorter idea
  • 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.

@CLAassistant
Copy link

CLAassistant commented Dec 29, 2016

CLA assistant check
All committers have signed the CLA.

@mention-bot
Copy link

@grischdian, thanks for your PR! By analyzing the history of the files in this pull request, we identified @carlaschroder, @butonic, @PVince81 and @Xenopathic to be potential reviewers.

@grischdian
Copy link
Contributor Author

grischdian commented Jan 7, 2017

@carlaschroder, @butonic, @PVince81 and @Xenopathic can someone explain why jenkins is failing? If I click on Details I got a "unable to connect" error

];
];

$this->denyLocalMounts = \OC::$server->getConfig()->getSystemValue('files_external_deny_local', 'false');
Copy link
Member

Choose a reason for hiding this comment

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

this will return the string 'false'. What you want is ..., false);

@butonic
Copy link
Member

butonic commented Jan 9, 2017

regarding jenkins:

sharing.Merging shares for recipient when shared from outside with group then user and recipient renames in between

Schlägt fehl seit 1 Build (Seit Fehlgeschlagen#1 )
Dauer: 8 Sekunden.
Stacktrace

Then as "user1" gets properties of folder "/merge-test-outside-groups-renamebeforesecondshare-renamed" with:

HTTP error: 404

@PVince81
Copy link
Contributor

PVince81 commented Jan 9, 2017

@butonic that issue should disappear after rebasing onto master

Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

Great addition, thanks.

Please fix the indent and this should be good to go.


if ($this->denyLocalMounts !== true) {
$backends[] = $container->query('OCA\Files_External\Lib\Backend\Local');
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix indentation, in ownCloud we use tabs, not spaces.

@@ -93,6 +92,11 @@ public function getBackends() {
$container->query('OCA\Files_External\Lib\Backend\SMB_OC'),
];

$this->denyLocalMounts = \OC::$server->getConfig()->getSystemValue('files_external_deny_local', false);
Copy link
Contributor

@phisch phisch Jan 10, 2017

Choose a reason for hiding this comment

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

It might be better to call it allowLocalMounts with a default of true to avoid multiple negations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@PVince81 @butonic what do you prefer? In my opinion both sounds good ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

@grischdian if you have time, please rename it to allowLocalMounts. I also prefer to avoid the double negative 😄

@PVince81
Copy link
Contributor

Setting back to "Developing" then

@grischdian
Copy link
Contributor Author

will take care today

@grischdian
Copy link
Contributor Author

I messed up my branch during changing from deny to allow - I close this PR - will open a new one. sorry

@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.

6 participants