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

Check if TTY is invalid in encryption:encrypt-all and encryption:decrypt-all #10439

Conversation

eugulixes
Copy link
Contributor

Inspired by #9894.
The commands occ encryption:decrypt-all and occ encryption:encrypt-all transform into interactive processes since they both use ConfirmationQuestion. It's not possible to execute the commands in a container via docker exec without the -i and -t options. However, it becomes not obvious from a user point of view why since the result of both commands in this case is aborted. This PR tries to solve the problem.
Fixes #9894.

Signed-off-by: Evgeny Golyshev eugulixes@gmail.com

@@ -123,6 +123,16 @@ protected function configure() {
}

protected function execute(InputInterface $input, OutputInterface $output) {
if (strtoupper(substr(PHP_OS, 0, 3)) !== 'WIN') {
Copy link
Member

Choose a reason for hiding this comment

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

windows is not supported anyway?

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, I suggest we will do nothing if we deal with Windows.

Copy link
Member

Choose a reason for hiding this comment

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

I would just remove this - that it breaks on windows is not a surprise, all of Nc breaks... Or should we keep it in @nickvergessen ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The official documentation doesn't suggest Windows as an option, so it would be fair enough to remove the Windows-related checks.

Copy link
Member

Choose a reason for hiding this comment

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

@eugulixes go ahead, I suppose, if this was the only issue @nickvergessen had then it would suffice to get this ready to go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@eugulixes eugulixes force-pushed the improve-encrypt-all-and-decrypt-all-commands branch from 818379e to 5cd4029 Compare August 11, 2018 08:50
@MorrisJobke MorrisJobke added this to the Nextcloud 14 milestone Aug 21, 2018
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Code looks good 👍 Tested and works

@rullzer
Copy link
Member

rullzer commented Aug 22, 2018

Failing unit tests

@ChristophWurst
Copy link
Member

Failing unit tests

Yeah, these built-in functions don't react well to phpunit (on CI?). I'd suggest to create a simple wrapper class for this that internally calls the php functions. This way we can mock them in unit tests. @eugulixes do you know how that works? I see this is your first contribution, so please let me know if you need assistance ✌️

That class would look something like

namespace OCP\Console;

class Environment {

    public function isInteractive(): bool {
        return posix_isatty(STDOUT);
    }
}

Inspiration was taken from https://github.com/sebastianbergmann/environment/blob/32c5cba90f7db47b1c10a777b36eccfd44ef8bd7/src/Console.php#L78, so that might also be an option to integrate. I've found traces of that class in my local dev setup.

@kesselb
Copy link
Contributor

kesselb commented Sep 5, 2018

https://github.com/nextcloud/3rdparty/blob/461663cd1143c593fc71ee976afeb0f4e628b16b/symfony/console/Input/InputInterface.php#L153

You could use $input->isInteractive() like @ChristophWurst suggested

@ChristophWurst
Copy link
Member

You could use $input->isInteractive() like @ChristophWurst suggested

Ha! Didn't know it was that simple! Thank you 😄

@ChristophWurst
Copy link
Member

You could use $input->isInteractive() like @ChristophWurst suggested

@eugulixes ping :)

@eugulixes
Copy link
Contributor Author

eugulixes commented Oct 2, 2018

@ChristophWurst, i'm planning to do it this weekend.

@ChristophWurst
Copy link
Member

Awesome, thanks!

@ChristophWurst ChristophWurst added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Oct 2, 2018
@eugulixes eugulixes force-pushed the improve-encrypt-all-and-decrypt-all-commands branch 2 times, most recently from 35dccff to 2bfccdf Compare October 7, 2018 15:36
@eugulixes
Copy link
Contributor Author

@ChristophWurst, I used $input->isInteractive() instead of pure posix_isatty(STDOUT) and tested. It works fine, however the tests fail. What should I do in this case?

@kesselb
Copy link
Contributor

kesselb commented Oct 7, 2018

$input->isInteractive() is false on ci.

$this->consoleInput = $this->getMockBuilder(InputInterface::class)->getMock();

You need to mock isInteractive. Take the file above and look for the line and insert the following below:

$this->consoleInput->expects($this->any())
    ->method('isInteractive')
    ->willReturn(true);

I guess the same applies to

$this->consoleInput = $this->getMockBuilder(InputInterface::class)->getMock();

@eugulixes eugulixes force-pushed the improve-encrypt-all-and-decrypt-all-commands branch from 2bfccdf to 4fcfe9a Compare October 14, 2018 12:00
…ypt-all

Signed-off-by: Evgeny Golyshev <eugulixes@gmail.com>
@eugulixes eugulixes force-pushed the improve-encrypt-all-and-decrypt-all-commands branch from 4fcfe9a to ec2f02f Compare October 14, 2018 12:06
@eugulixes
Copy link
Contributor Author

@ChristophWurst, @danielkesselberg, all tests passed. Thank you for your help.

@kesselb kesselb added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 14, 2018
@MorrisJobke MorrisJobke merged commit e36d4a9 into nextcloud:master Oct 15, 2018
@eugulixes eugulixes deleted the improve-encrypt-all-and-decrypt-all-commands branch October 15, 2018 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants