New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we not need this validation with the new changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These validations happen during the construction of ServiceAccountCredential
object. So we can assume, they are all present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! LGTM!
src/auth/credential.ts
Outdated
} | ||
|
||
constructor(json: object) { | ||
if (typeof json !== 'object' || json === null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use the validators utils here and below for type validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering why the type
property is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Refactoring and simplifying the existing credential implementation for better design and async project ID discovery.
CertCredential
class was renamed toServiceAccountCredential
.Certificate
type is renamed toServiceAccount
, and is no longer exported. Instead theprojectId
,clientEmail
andprivateKey
properties are directly exposed from theServiceAccountCredential
class.MetadataServiceCredential
class was renamed toComputeEngineCredential
. This class now also hasgetProjectId(): Promise<string>
method to discover the project ID from the Compute Engine environment.ApplicationDefaultCredential
class has been removed. Instead now there's agetApplicationDefault()
function that returns aCredential
implementation suitable for the environment.