-
Notifications
You must be signed in to change notification settings - Fork 396
fix(auth): Remove (base64) 'REDACTED' passwords from user records. #660
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
Conversation
These values *look* like passwords hashes, but aren't, leading to potential confusion. Additionally, added docs to CONTRIBUTING.md detailing how to add the permission that causes password hashes to be properly returned as well as adjusting the test failure message should the developer not add that permission. b/141189502
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 for putting this together.
// then clear it out, similar to how the salt is returned. (Otherwise, it | ||
// *looks* like a b64-encoded hash is present, which is confusing.) | ||
if (response.passwordHash === B64_REDACTED) { | ||
utils.addReadonlyGetter(this, 'passwordHash', undefined); |
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.
Please add a unit test for this. When a user record is initialized with a server response containing a redacted password, confirm it is cleared.
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.
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 for adding to auth.spec.ts. Can you also add to user-record.spec.ts, the test associated with this class?
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. I've removed the test from auth.spec.ts, as I think with this change, it's become mostly redundant. Though we can add it back in if you'd prefer to keep it.
CONTRIBUTING.md
Outdated
3. Enable the IAM API: Go to the | ||
[Google Cloud Platform Console](https://console.cloud.google.com) and make | ||
sure your Firebase/GCP project is selected. Select "APIs & Services > | ||
Dashboard" from the main menu, and click the "ENABLE APIS AND SERVICES" | ||
button. Search for and enable the "Identity and Access Management (IAM) | ||
API". | ||
4. Grant your service account the 'Firebase Authentication Admin' role: |
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.
Can we also add a note here indicating why this permission is needed?
"This is required to ensure that exported user records contain the password hashes of the user accounts."
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.
test/unit/auth/user-record.spec.ts
Outdated
@@ -653,6 +653,15 @@ describe('UserRecord', () => { | |||
}); | |||
}); | |||
|
|||
describe('clears REDACTED passwordHash', () => { |
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 a test of passwordHash
getter.
Can you move it next to it('should return expected undefined passwordHash', () => {
You can define something like: it('should clear REDACTED passwordHash', () => {
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
These values look like passwords hashes, but aren't, leading to
potential confusion.
Additionally, added docs to CONTRIBUTING.md detailing how to add the
permission that causes password hashes to be properly returned as well
as adjusting the test failure message should the developer not add that
permission.
b/141189502
RELEASE NOTE:
ExportedUserRecord
no longer exposes password hashes that are redacted due to lack of permissions in the service account credentials.