[#832] fix the issue when private key was never used #833
Conversation
| $config = $this->stepdef->get('config'); | ||
|
|
||
| if (empty($config)) { | ||
| return $password; | ||
| } |
There was a problem hiding this comment.
Is config itself ever empty for this step? I think it's not possible given it's validation.
There was a problem hiding this comment.
well, it's a trait so we can't be 100% sure how it could be used... for me it makes sense to add this check.
| if (!empty($config->password)) { | ||
| // Because variables use redacted values by default, we evaluate the secret explicitly. | ||
| $password = $this->get_variables()->evaluate($config->password); | ||
| } |
There was a problem hiding this comment.
I believe this would miss cases where the password is an empty string or zero(es). It might be too edgy of a case to care though, and it's probably something that shouldn't be set so I'm okay with this.
There was a problem hiding this comment.
good point. Changing to isset
|
|
||
| // Use key authorisation if privkeyfile is set. | ||
| if (!empty($config->privkeyfile)) { | ||
| $privkeycontents = file_get_contents($this->resolve_path($config->privkeyfile)); | ||
| $privkeypassphrase = $config->password ?: false; | ||
| $privkeypassphrase = $password ?: false; |
There was a problem hiding this comment.
By this stage, password would have already been set. It's safe to use the password variable as-is and you do not have to assign it to a new variable (or default it back to false)
There was a problem hiding this comment.
This is annoying, but it's not very clear what will be returned by $this->get_variables()->evaluate($config->password); Ideally we should add more validation here to make sure it's a string.
| @@ -293,13 +293,22 @@ public function execute($input = null) { | |||
| * @return AsymmetricKey|string | |||
| */ | |||
| private function load_key() { | |||
| // Because variables use redacted values by default, we evaluate the secret explicitly. | |||
| $password = $this->get_variables()->evaluate($this->stepdef->config->password); | |||
| $password = false; | |||
There was a problem hiding this comment.
I'm also concerned whether or not the current user:pass and user:nopassword will work, if false is returned. If it's OK, that's great. We should update the phpdoc to support AsymmetricKey|string|false
|
All those good points that need to be sorted. For now we re moving on with minimum changes #834 |
No description provided.