Skip to content

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

Merged
merged 7 commits into from
Sep 27, 2019

Conversation

rsgowman
Copy link
Member

@rsgowman rsgowman commented Sep 25, 2019

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.

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
Copy link
Contributor

@bojeil-google bojeil-google left a 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);
Copy link
Contributor

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Contributor

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?

Copy link
Member Author

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:
Copy link
Contributor

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."

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@hiranya911 hiranya911 removed their assignment Sep 27, 2019
@rsgowman rsgowman assigned bojeil-google and unassigned rsgowman Sep 27, 2019
@@ -653,6 +653,15 @@ describe('UserRecord', () => {
});
});

describe('clears REDACTED passwordHash', () => {
Copy link
Contributor

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', () => {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@rsgowman rsgowman merged commit 47c5056 into master Sep 27, 2019
@rsgowman rsgowman deleted the rsgowman/fix_redacted_passwords branch September 27, 2019 20:57
@hiranya911 hiranya911 changed the title Remove (base64) 'REDACTED' passwords from user records. fix(auth): Remove (base64) 'REDACTED' passwords from user records. Oct 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants