Refactored the credentials implementation #730
Conversation
… hkj-cred-refactor
| ); | ||
| } | ||
| if (!validator.isNonEmptyString(certificate.clientEmail) || !validator.isNonEmptyString(certificate.privateKey)) { |
There was a problem hiding this comment.
Do we not need this validation with the new changes?
There was a problem hiding this comment.
These validations happen during the construction of ServiceAccountCredential object. So we can assume, they are all present.
src/auth/credential.ts
Outdated
| } | ||
|
|
||
| constructor(json: object) { | ||
| if (typeof json !== 'object' || json === null) { |
There was a problem hiding this comment.
You can use the validators utils here and below for type validation.
| copyAttr(this, json, 'clientId', 'client_id'); | ||
| copyAttr(this, json, 'clientSecret', 'client_secret'); | ||
| copyAttr(this, json, 'refreshToken', 'refresh_token'); | ||
| copyAttr(this, json, 'type', 'type'); |
There was a problem hiding this comment.
I am wondering why the type property is needed.
There was a problem hiding this comment.
Yes, it's kind of redundant. It's always going to have the constant value "authorized_user" in this case. I think this type is meant to represent the refresh token JSON file, which usually contains a "type" field. I'd keep it as is, since removal of the property is not really related to this refactor. We can remove it in a future PR if needed.
| return new ComputeEngineCredential(httpAgent); | ||
| } | ||
|
|
||
| function copyAttr(to: {[key: string]: any}, from: {[key: string]: any}, key: string, alt: string) { |
There was a problem hiding this comment.
This seems like it should be a utility function. Also add some javadoc for some of these function that may not be trivial to figure out. For example, it was not clear to me what alt stands for.
There was a problem hiding this comment.
Added some jsdoc comments. This was also an pre-existing function.
test/unit/auth/credential.spec.ts
Outdated
| expect(() => getApplicationDefault()).to.throw(Error); | ||
| }); | ||
|
|
||
| it('should throw if a the credentials file content is not an object', () => { |
Refactoring and simplifying the existing credential implementation for better design and async project ID discovery.
CertCredentialclass was renamed toServiceAccountCredential.Certificatetype is renamed toServiceAccount, and is no longer exported. Instead theprojectId,clientEmailandprivateKeyproperties are directly exposed from theServiceAccountCredentialclass.MetadataServiceCredentialclass was renamed toComputeEngineCredential. This class now also hasgetProjectId(): Promise<string>method to discover the project ID from the Compute Engine environment.ApplicationDefaultCredentialclass has been removed. Instead now there's agetApplicationDefault()function that returns aCredentialimplementation suitable for the environment.