Paths

Table of Contentst

Passphrase v0
ClosedPublic
Actions

Authored by epriestley on Nov 19 2013, 9:42 PM.

Details

Reviewers

btrahan
chad

Maniphest Tasks

T4122: Implement "Passphrase", a credential management application

Commits

Restricted Diffusion Commit
rP91d084624b9c: Passphrase v0

Summary

Ref T4122. Implements a credential management application for the uses described in T4122.

@chad, this needs an icon, HA HA HAHA HA BWW HA HA HA

bwahaha

Test Plan

See screenshots.

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

Credential List:

{F83321}

New Credential:

{F83323}

Edit Credential Info:

{F83326}

Credential Detail:

{F83330}

Destroy:

{F83332}

Destroyed:

{F83334}

Reveal:

{F83336}

Revealed:

{F83338}

willdo

epriestley added inline comments.Nov 19 2013, 11:38 PM

src/applications/policy/storage/PhabricatorPolicy.php
146	@chad I made more work for you here too (policy icon for "one specific user").

My assumption was 'noone' was for items visible to only me. As in, who can see this item (besides myself), 'noone'. It does feel more specific to list a user.

Overall, I just want consistency. We can use a 'single person' icon and have it say 'Only Me (chad)' in place of the 'noone' icon. Would that work everywhere?

As of this diff, there are two separate classes of policy:

"No One" means "No one, except users who have automatic capabilities". This often is not the same as "Only Me". For example, if a task's "Can Edit" policy is set to "No One", that means "No one, except the task owner". That might be you, but also might be someone else. In the case of Credentials here, there are no policy exceptions so "No One" really means "no one at all" (the UI either already prevents you from actually doing this now or should at some point, since the object will become completely inaccessible).
"User: X" means "Just that user (and users who have automatic capabilities)". Again, this is often not the same as "Only Me". For example, if a task's "Can Edit" policy is set to "epriestley", that means "epriestley and the task owner". Unless you're epriestley, it shouldn't render "Only Me".

We could render the policy "User: X" as "Only Me (X)" instead of "X" if the viewer is the same as X, but I think that would reduce consistency (although it might increase clarity).

Makes sense. Thanks. Surprised I didn't include an only me icon anyways.

Nice little Tuesday you're having here. :D

One big picture thing I can't 100% grok from this diff is how you anticipate applications integrating with these credentials. I guess by having some sort of credential chooser in a given application, and then the application should store the credential id / phid or what have you? ...and then this chooser would have to handle the case if there are no credentials or no credentials that are applicable for the integration? Plus the corner case of if the credential has been destroyed? (God bless the credential chooser.)

src/applications/passphrase/controller/PassphraseCredentialEditController.php
63	if would be cool if the bullet string was as long as the actual secret
104–106	PhutilOpaqueEnvelope to the rescue?
108	so this means you can't have a secret that is bullets? I don't even know if you can type in a bullet but hey.
144–145	i thought the editor would take care of this?

Yeah, the next diff will introduce some kind of credential chooser like:

SSH Key: [This part is a dropdown V] (Create New)

...or something. The control will filter credentials so you can only select the right kind of credential (e.g., only SSH keys or only usernames/passwords or whatever).

The callsites which use the credentials will have to do the unpacking/verification work, but I think that can look roughly like "get the credential with <phid>, of type <type>, and throw an exception if any of that isn't like it's supposed to be" so probably this code will really only get written once. I don't think it'll be too much of a mess.

(I might do edges too, so you can tell what's still using the credentials.)

src/applications/passphrase/controller/PassphraseCredentialEditController.php
63	ah, but that would leak information about the secret
104–106	We can't use OpaqueEnvelope when writing to the database itself, and I don't want any of the data to end up in the transaction. We could play some games here with the Editor (passing the OpaqueEnvelope as part of the transaction, doing the save inside the editor, then rewriting the transaction), but this seemed cleaner overall. If we end up with another workflow that has to do this I'd probably push this into the editor, but adding credentials via API doesn't seem like it has any use cases offhand.
108	That's correct. This seemed like a reasonable price to pay. I think Option+8 types bullets, but I'm not sure if those are the same as unicode bullets. ••••••••
144–145	These cover the case where the transactions failed to apply. The issue here is just that `AphrontFormPolicyControl` reads its values directly out of the object, so if we don't write them here we lose the values the user selected. A maybe-cleaner approach would be to have a `setValue($v_view_policy)` on the control explicitly.

btrahan added inline comments.Nov 20 2013, 1:37 AM

src/applications/passphrase/controller/PassphraseCredentialEditController.php
63	true and its thus better as you have it. i initially thought folks might feel confused when the secret is really long though

btrahan added inline comments.Nov 20 2013, 1:41 AM

src/applications/passphrase/controller/PassphraseCredentialEditController.php
144–145	I thought parent::applyInternalEffects got called early enough to handle this (setViewPolicy and setEditPolicy do occur there). maybe it can be in the catch then? obviously NBD, geeking out on the Editor codepath here.