Add SecretsRevealCommand

[danielburger1337]

Q	A
Branch?	7.1
Bug fix?	no
New feature?	yes
Deprecations?	no
Issues	n/a
License	MIT

Add a new command to reveal a specific secrets value. The output is able to be piped into other commands.

Docs PR will be submitted if this is a desired feature.

[Nyholm]

This is a great PR. Thank you.

[stof]

should the command fail when passing a name and there is no secret with that name ?

[danielburger1337]

should the command fail when passing a name and there is no secret with that name ?

Thats one way to do it. I just reused the current behavior when the secret store is empty.

If we go your way, we should refactor a bit so that the comments about how to reference/reveal a secret are not displayed when an invalid name is passed.

    protected function execute(InputInterface $input, OutputInterface $output): int
    {
        $io = new SymfonyStyle($input, $output instanceof ConsoleOutputInterface ? $output->getErrorOutput() : $output);

        $reveal = $input->getOption('reveal');

        $secrets = $this->vault->list($reveal);

        if (null !== $name = $input->getArgument('name')) {
            if (!\array_key_exists($name, $secrets)) {
                $io->error(\sprintf('The secret "%s" does not exist.', $name));

                return self::INVALID;
            }

            $secrets = [$name => $secrets[$name]];
        }

        $localSecrets = $this->localVault?->list($reveal);

        $io->comment('Use <info>"%env(<name>)%"</info> to reference a secret in a config file.');

        if (!$reveal) {
            $io->comment(\sprintf('To reveal the secrets run <info>php %s %s --reveal</info>', $_SERVER['PHP_SELF'], $this->getName()));
        }

[chalasr]

If we go your way, we should refactor a bit so that the comments about how to reference/reveal a secret are not displayed when an invalid name is passed.

Sounds good to me

[fabpot]

Would allowing more than one name to get a subset of the list make sense? It would be more consistent with the command name.

[danielburger1337]

Would allowing more than one name to get a subset of the list make sense? It would be more consistent with the command name.

I agree that this would fit better with the command name, but how would you handle passing a valid AND an invalid secret name? Only return an error? Return the valid secret and an error message? Would this result in exit code 0, 1 or 2?

[chalasr]

It would be more consistent with the command name.

Good point. Given the number of questions that supporting multiple names raise, what about adding a secrets:show command instead?
Otherwise, I would just make the command fail entirely when an invalid name is passed.

[fabpot]

It would be more consistent with the command name.

Good point. Given the number of questions that supporting multiple names raise, what about adding a secrets:show command instead? Otherwise, I would just make the command fail entirely when an invalid name is passed.

That's maybe a better option indeed.

[nicolas-grekas]

secrets:show

Or secrets:reveal to use the existing wording?
but I agree with the proposal. That could be even more useful since this wouldn't require decorating the output with fancy layout. Instead, the result could be directly piped to another command when needed.

[chalasr]

👍 for reveal

[fabpot]

@danielburger1337 Are you still interested in moving this PR forward?

[danielburger1337]

@danielburger1337 Are you still interested in moving this PR forward?

Yes I am. Just to make sure, you guys want to add the new SecretsRevealCommand. This would entail removing the added "name" argument from the SecretsListCommand again and then the "-r" option will be deprecated?

[nicolas-grekas]

I wouldn't deprecated -r, but yes for the rest

[danielburger1337]

Updated title and description to match new proposed idea.

[94noni]

nice one, small comments regarding desc

[fabpot]

Can you rebase to get rid of the merge commit?

[fabpot]

Thank you @danielburger1337.

[danielburger1337]

@fabpot All done. Sorry for the extra work on your part.

Is there any particular reason why you guys don't add the "void" return type to test cases?

[OskarStark]

Is this needed when the class is final?

[danielburger1337]

I have no idea. I just copied it from SecretsListCommand (as this command is based on it).

I don't think it makes much sense on either.

[chalasr]

@OskarStark That gives us more flexibility 👍 as there is no need to use that class from userland, only the command itself (via CLI) matters

[OskarStark]

Sorry I don't get what you mean.

I can do absolutely the same without the internal tag, the class is final and I can only execute the command in userland

[xabbuh]

I guess what @chalasr means is that we could also rename this class or move it to another namespace without having to worry about consumers of it. Not sure if we really need that flexibility though tbh.

[OskarStark]

Ah ok got it

[chalasr]

Not sure if we really need that flexibility though tbh.

👍 Thinking about it, I agree we should not fall in being too much defensive in general.