Closes T8481.
Details
- Reviewers
epriestley eadler lpriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T8481: Storing secret file in paste
- Commits
- Restricted Diffusion Commit
rP9537f983f6c0: Added a Note Credential Type for Passphrase
Verify that in Passphrase > Create an option to create a Note credential exists and credentials of type Note are createable.
Diff Detail
- Repository
- rP Phabricator
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
src/applications/passphrase/credentialtype/PassphraseCredentialTypeNote.php | ||
---|---|---|
30 ↗ | (On Diff #32052) | I think this should be a AphrontFormTextAreaControl instead of a AphrontFormPasswordControl |
src/applications/passphrase/credentialtype/PassphraseCredentialTypeNote.php | ||
---|---|---|
31 ↗ | (On Diff #32059) | I don't think that new class has a setDisableAutocomplete method on it, and looking through it, I don't think there are any custom properties there that need to be set, so you're okay just removing setDisableAutocomplete. |
@joshuaspence I think that since the other files in the directory share this type of naming (PassphraseCredentialTypePassword.php and PassphraseCredentialTypeSSHPrivateKey.php) it is better to maintain that naming style for consistency purposes.
I agree that Login/Username does not make sense in this Passphrase type. Let's remove it.
Let's add a getter on parent class PassphraseCredentialType. Something like:
public function shouldRequireUsername() { return true; }
In your PassphraseCredentialTypeNote class that extends PassphraseCredentialType, add an override method that returns false.
First of all, we need to remove the input from the create and edit page. The create and edit pages share a controller, PassphraseCredentialEditController. The main method of that class, processRequest, handles both the rendering and saving of the edit page. If you notice, on lines 29 and 34, we save a variable $type which is the type object of the credential (for example, it could be an instance of PassphraseCredentialTypeNote). Now we can call $type->shouldRequireUsername() to know if we need to add an input for the username.
The if ($request->isFormPost()) block handles the validating and saving of the edit POST, but once the block ends, you'll see that there is a chain of blocks like $form->appendChild() where the actual edit form is built. Find the one where an input with label "Login/Username" is added. This should only be added if the note type requires a username field. Now you know how to do that. Put the $form->appendChild(<username field>) inside an if condition. Check that the create form in the UI no longer provides a username input on note type passphrases.
Now if you hit save, you will get an error that username is required. This is because the transactions that are being applied, still expect a username transaction. Let's go back to the if ($request->isFormPost()) block. That is where we collect the transactions to be applied. Specifically, this happens inside the if (!$errors) block. Find the block where transaction of type PassphraseCredentialTransaction::TYPE_USERNAME is added to the $xactions array. Change this so that it's only added if the shouldRequireUsername() flag is true.
If you try to save the new note credential now, you will still get an error. This is because in the PassphraseCredentialTransactionEditor class (where all the transactions are actually applied), there is a method validateTransaction that always checks that every POST has a valid username. In that switch statement, in the case, case PassphraseCredentialTransaction::TYPE_USERNAME: we should first check if the type of the credential is a note type. $object is the credential object, and the credential class, PassphraseCredential has a convenient helper method getCredentialTypeImplementation() that let's us get the type class of the credential. Save that type in the switch case, and if shouldRequireUsername() flag is false, break out of the switch. (We don't want to just return; because we still want to return all the other errors we have collected.) Now try saving.
You should get an error saying that the username database column can't be null. Go back to the PassphraseCredentialEditController class, and see around line 50, we attempt to set a default username on the credential. Casting that as a string will by default set that to '' instead of null. Like this:
$credential->setUsername((string)$request->getStr('username'));
Now try saving. You should have no errors, and you should arrive at the credential summary page. Notice that the left column of the summary, the properties column, has a label "Username" with no value. Because the credential should never have a username, we should clean that up. In PassphraseCredentialViewController, there is a method buildPropertyView where that properties column is built. At the top of that method, save the $type object of the credential similar to what we did in the transaction editor using the helper method on the credential class. Then, add an if() {} block to check if the $type requires a username, and only add the property to the column if the type requires a username.
Also, see D13266 to get the format that you should follow to rename the PassphraseCredentialTypeNote class. Let me know if I can clarify anything.
src/applications/passphrase/credentialtype/PassphraseCredentialTypeNote.php | ||
---|---|---|
3 ↗ | (On Diff #32061) | Since @joshuaspence renamed all others, see his diff, and rename them to match. |
Just some minor things to polish.
src/applications/passphrase/controller/PassphraseCredentialEditController.php | ||
---|---|---|
124–125 | Since this is simply saving a transaction type, there isn't any benefit to making this conditional. Plus, the code would be easier to read. | |
154–158 | This looks good, but there seems to be an issue with incorrect indents. | |
276 | Minor thing. The current standard syntax would prefer that the closing brace be on the same indent as the initial if (). | |
src/applications/passphrase/editor/PassphraseCredentialTransactionEditor.php | ||
178 | This looks misaligned by like a space or something. Also, for code clarity, it might be better to put the break on its own line. |
src/applications/passphrase/controller/PassphraseCredentialEditController.php | ||
---|---|---|
154–158 | Actually, I think this needs to be indented another layer, since it's inside that outer if. |