Resolves T5868. This implements passphrase.query and a mechanism for allowing Conduit access to credentials.
Details
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T5868: Provide a Conduit call for retrieving Passphrase credentials
- Commits
- Restricted Diffusion Commit
rP26f283fe21a5: Implement `passphrase.query` for querying credentials
Tested locally.
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Comment Actions
This looks generally good. Mostly-nitpicky inlines, high-level stuff is:
- One actual important behavioral issue: enabling API access MUST require MFA if MFA is enabled on the account. See inline for more discussion.
- There's some more modern stuff available for doing cursors/paging now, see DiffusionQueryCommitsConduitAPIMethod for an example.
- I think the flag should only prevent secret access, not hide the entire credential.
| src/applications/passphrase/conduit/PassphraseQueryConduitAPIMethod.php | ||
|---|---|---|
| 12 | pht() | |
| 20–21 | There's a parent method to add these, isn't there? getPagingParamers() or something? | |
| 36 | I don't think "Allow API" needs to completely prevent the credential from being retrieved over the API, just the secret. As written, this behavior seems a bit confusing -- you'll query for stuff and not get it back. | |
| 50–57 | I think there are some CursorPager things in the parent? | |
| 65–67 | Just omit from result list, I think? This seems like a bad behavior for an API method. | |
| 69–74 | These are currently really expensive to pull -- we have to extract them from the private keys. Let's add a needPublicKeys option for now to enable pulling them? Alternatively, we could add a cache, or remove this feature for now if you don't need it. I don't want to do 100 synchronous openssl calls to respond to this query, though. | |
| 83–84 | Let's add monogram, name and uri? | |
| 86 | This is forward-looking, but maybe we should call this secrets, and return a map. Particularly, T5553 contemplates storing SSL certificates in Passphrase, which seems reasonable. However, SSL certificates have multiple secrets (key, cert, possibly chain although that's not really secret). Or maybe we could call it material and return a map like: "material": {
"publicKey": ...
"privateKey": ...
}Or: "material": {
"sslCertificate": ...,
"sslKey": ...,
"sslChain": ...,
}And potentially (although I'm not sure this is a great idea or relevant at all): "material": {
"publicKey": ...,
"noAPIAccess": "This credential's private material is not accessible via API calls."
}...which might make the API flag less confusing. | |
| 91 | As a modern method, this should return data in data and provide pager results. Look at DiffusionQueryCommitsConduitAPIMethod for an example of the most modern stuff. It simplifies some of the cursor/paging stuff. | |
| src/applications/passphrase/controller/PassphraseCredentialConduitController.php | ||
| 37 | We must require MFA (if enabled) to access this endpoint. Otherwise an attacker can access a credential, enable API access for it, and then retrieve the credential over the API without surviving a MFA check. (You can just add the blahblah->requireMFA() thing present in the secret controller to implement this.) | |
| 58 | it's -> its | |
| 72 | Let's call this "Allow API Access" (vs "Allow Conduit Access") in the UI? I think that's probably clearer. | |
| 76–89 | Remove unreachable copypasta. | |
| src/applications/passphrase/storage/PassphraseCredentialTransaction.php | ||
| 96–104 | In this case where we have more space, maybe "Conduit API access". | |
| src/applications/passphrase/controller/PassphraseCredentialConduitController.php | ||
|---|---|---|
| 76–89 | mmm pasta | |
Comment Actions
Couple of straightforward inlines.
| src/applications/passphrase/conduit/PassphraseQueryConduitAPIMethod.php | ||
|---|---|---|
| 100–101 | pht() this value | |
| 117 | This should be something like: $result = array( 'data' => $results, ); return $this->addPagerResults($result, $pager); | |
| src/applications/passphrase/controller/PassphraseCredentialConduitController.php | ||
| 43 | Remove; same as line 29. | |
| src/applications/passphrase/query/PassphraseCredentialQuery.php | ||
| 11 | We could remove this stuff since we don't need it anymore, although maybe it's interesting to add the ability to search for these credentials in the UI ("What stuff are we exposing?"). I think it's fine to keep unless that use case sounds silly to you. | |