Page MenuHomePhabricator

Use Phobject class constants for Passphrase credential types
Needs RevisionPublic

Authored by joshuaspence on Nov 5 2015, 10:13 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Sep 5, 5:14 AM
Unknown Object (File)
Wed, Aug 28, 8:28 PM
Unknown Object (File)
Sat, Aug 24, 10:46 PM
Unknown Object (File)
Sun, Aug 18, 5:15 AM
Unknown Object (File)
Fri, Aug 9, 5:01 AM
Unknown Object (File)
Aug 5 2024, 5:16 AM
Unknown Object (File)
Aug 1 2024, 6:24 AM
Unknown Object (File)
Jul 28 2024, 6:08 AM
Subscribers

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Summary

Currently the PassphraseCredentialType class has abstract getCredentialType and getProvidesType methods and all implementations do something like this:

final class PassphraseSomethingCredentialType extends PassphraseCredentialType {

  const CREDENTIAL_TYPE = 'something';
  const PROVIDES_TYPE = 'provides/something';

  public function getCredentialType() {
    return self::CREDENTIAL_TYPE;
  }

  public function getProvidesType() {
    return self::PROVIDES_TYPE;
  }

  ...
  
}

Instead, provide a concrete implementation of these methods which reads the class constant values with the getPhobjectClassConstant method.

Test Plan

Loaded passphrase.

Diff Detail

Repository
rP Phabricator
Branch
master
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 8850
Build 10340: Run Core Tests
Build 10339: arc lint + arc unit

Event Timeline

joshuaspence retitled this revision from to Minor improves for Passphrase credential types.
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
This revision now requires changes to proceed.Nov 8 2015, 2:24 PM
joshuaspence retitled this revision from Minor improves for Passphrase credential types to Use Phobject class constants for Passphrase credential types.Nov 8 2015, 8:15 PM
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence edited edge metadata.
joshuaspence edited edge metadata.
joshuaspence removed a subscriber: Korvin.

Admittedly, I don't actually understand what the "provides type" is used for...

epriestley edited edge metadata.

"Provides" is used because things that need an SSH key can accept either a PassphraseSSHPrivateKeyFileCredentialType (path to a key on disk) or a PassphraseSSHPrivateKeyTextCredentialType (actual key text): both "provide" an SSH key. Specifically, both can build a PassphraseSSHKey object.

The former is a legacy credential type which you can no longer generate from the UI, but which we can't cleanly/easily migrate away from (bin/storage upgrade may not have access to read the key, and the key may not even be present on the host where bin/storage upgrade runs) and which isn't clearly or unambiguously undesirable, although I do think we'd generally be better off without it.

The way this works is generally a little funky / nonstandard.

PassphraseCredential also currently stores a providesType in the database, but this change does not include a migration. My expectation is that it would break every existing SSH key without this migration.

That column probably should not exist, and the Query should implement searches for a particular providesType by enumerating all credential types which provide that type of credential, then doing a WHERE credentialType IN (...) instead. That is, the credentialType always strictly determines the providesType, so the column which stores providesType separately is needless and undesirable.

I think a reasonable pathway forward might look like this:

  1. Do this change, but don't change the PROVIDES_TYPE constants, since that will break withProvidesTypes(...) calls on the Query if you don't migrate.
  2. Change the Query to resolve withProvidesTypes(...) like this instead:
    • Get all credential types.
    • Find the ones that provide the right PROVIDES_TYPE.
    • Query as credentialType IN (%LS) for those types.
  3. Remove the providesType column from the database, as it should no longer have use sites.
  4. Maybe change PROVIDES_TYPE to something like withImplements(array $interfaces), then define PassphraseCredentialSSHKeyProviderInterface, etc. Do the lookup in the same way (find matching credential types, query by credential type). This would give us a more standard-looking class tree which was a bit more flexible, I think?
This revision now requires changes to proceed.Nov 15 2015, 3:54 PM