From ff14bf5bfb9207529ff0a416a041bcc69ae2814f Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Wed, 25 Sep 2019 16:51:54 -0400 Subject: [PATCH 1/2] Remove (base64) 'REDACTED' passwords from user records. 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 --- CONTRIBUTING.md | 14 ++++++++++++-- firebase_admin/_user_mgt.py | 18 ++++++++++++++---- integration/test_auth.py | 18 ++++++++++++++---- tests/test_user_mgt.py | 8 ++++++++ 4 files changed, 48 insertions(+), 10 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 8c58e63e..7b4a0ea8 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -115,7 +115,7 @@ pylint firebase_admin However, it is recommended that you use the [`lint.sh`](lint.sh) bash script to invoke pylint. This script will run the linter on both `firebase_admin` and the corresponding -`tests` module. It suprresses some of the noisy warnings that get generated +`tests` module. It suppresses some of the noisy warnings that get generated when running pylint on test code. Note that by default `lint.sh` will only validate the locally modified source files. To validate all source files, pass `all` as an argument. @@ -181,13 +181,23 @@ Then set up your Firebase/GCP project as follows: to set up Firestore either in the locked mode or in the test mode. 2. Enable password auth: Select "Authentication" from the "Develop" menu in Firebase Console. Select the "Sign-in method" tab, and enable the - "Email/Password" sign-in method. + "Email/Password" sign-in method, including the Email link (passwordless + sign-in) option. + 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. This is + required to ensure that exported user records contain the password hashes of + the user accounts: + 1. Go to [Google Cloud Platform Console / IAM & admin](https://console.cloud.google.com/iam-admin). + 2. Find your service account in the list, and click the 'pencil' icon to edit it's permissions. + 3. Click 'ADD ANOTHER ROLE' and choose 'Firebase Authentication Admin'. + 4. Click 'SAVE'. + Now you can invoke the integration test suite as follows: diff --git a/firebase_admin/_user_mgt.py b/firebase_admin/_user_mgt.py index 867b6dd8..bb911ef4 100644 --- a/firebase_admin/_user_mgt.py +++ b/firebase_admin/_user_mgt.py @@ -14,8 +14,8 @@ """Firebase user management sub module.""" +import base64 import json - import requests import six from six.moves import urllib @@ -26,6 +26,7 @@ MAX_LIST_USERS_RESULTS = 1000 MAX_IMPORT_USERS_SIZE = 1000 +B64_REDACTED = base64.b64encode("REDACTED") class Sentinel(object): @@ -257,9 +258,17 @@ def password_hash(self): If the Firebase Auth hashing algorithm (SCRYPT) was used to create the user account, this is the base64-encoded password hash of the user. If a different hashing algorithm was used to create this user, as is typical when migrating from another Auth system, this - is an empty string. If no password is set, this is ``None``. + is an empty string. If no password is set, or if the service account doesn't have permission + to read the password, then this is ``None``. """ - return self._data.get('passwordHash') + password_hash = self._data.get('passwordHash') + + # If the password hash is redacted (probably due to missing permissions) 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 password_hash == B64_REDACTED: + return None + return password_hash @property def password_salt(self): @@ -268,7 +277,8 @@ def password_salt(self): If the Firebase Auth hashing algorithm (SCRYPT) was used to create the user account, this is the base64-encoded password salt of the user. If a different hashing algorithm was used to create this user, as is typical when migrating from another Auth system, this is - an empty string. If no password is set, this is ``None``. + an empty string. If no password is set, or if the service account doesn't have permission to + read the password, then this is ``None``. """ return self._data.get('salt') diff --git a/integration/test_auth.py b/integration/test_auth.py index 1a4bacce..f2e8eabe 100644 --- a/integration/test_auth.py +++ b/integration/test_auth.py @@ -228,8 +228,14 @@ def test_list_users(new_user_list): assert isinstance(user, auth.ExportedUserRecord) if user.uid in new_user_list: fetched.append(user.uid) - assert user.password_hash is not None - assert user.password_salt is not None + assert user.password_hash is not None, ( + "Missing password_hash field. A common cause would be forgetting to add the " + + "'Firebase Authentication Admin' permission. See instructions in " + + "CONTRIBUTING.md") + assert user.password_salt is not None, ( + "Missing password_salt field. A common cause would be forgetting to add the " + + "'Firebase Authentication Admin' permission. See instructions in " + + "CONTRIBUTING.md") page = page.get_next_page() assert len(fetched) == len(new_user_list) @@ -239,8 +245,12 @@ def test_list_users(new_user_list): assert isinstance(user, auth.ExportedUserRecord) if user.uid in new_user_list: fetched.append(user.uid) - assert user.password_hash is not None - assert user.password_salt is not None + assert user.password_hash is not None, ( + "Missing password_hash field. A common cause would be forgetting to add the " + + "'Firebase Authentication Admin' permission. See instructions in CONTRIBUTING.md") + assert user.password_salt is not None, ( + "Missing password_salt field. A common cause would be forgetting to add the " + + "'Firebase Authentication Admin' permission. See instructions in CONTRIBUTING.md") assert len(fetched) == len(new_user_list) def test_create_user(new_user): diff --git a/tests/test_user_mgt.py b/tests/test_user_mgt.py index 3c19a98d..c7bffd51 100644 --- a/tests/test_user_mgt.py +++ b/tests/test_user_mgt.py @@ -14,6 +14,7 @@ """Test cases for the firebase_admin._user_mgt module.""" +import base64 import json import time @@ -152,6 +153,13 @@ def test_exported_record_empty_password(self): assert user.password_hash == '' assert user.password_salt == '' + def test_redacted_passwords_cleared(self): + user = auth.ExportedUserRecord({ + 'localId': 'user', + 'passwordHash': base64.b64encode('REDACTED'), + }) + assert user.password_hash is None + def test_custom_claims(self): user = auth.UserRecord({ 'localId' : 'user', From 22ad6770b191d7f60a724ae00db36dcaacda73fc Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Mon, 30 Sep 2019 09:41:39 -0400 Subject: [PATCH 2/2] review feedback --- firebase_admin/_user_mgt.py | 2 +- integration/test_auth.py | 18 ++++++++---------- tests/test_user_mgt.py | 2 +- 3 files changed, 10 insertions(+), 12 deletions(-) diff --git a/firebase_admin/_user_mgt.py b/firebase_admin/_user_mgt.py index bb911ef4..2e10fac1 100644 --- a/firebase_admin/_user_mgt.py +++ b/firebase_admin/_user_mgt.py @@ -26,7 +26,7 @@ MAX_LIST_USERS_RESULTS = 1000 MAX_IMPORT_USERS_SIZE = 1000 -B64_REDACTED = base64.b64encode("REDACTED") +B64_REDACTED = base64.b64encode(b'REDACTED') class Sentinel(object): diff --git a/integration/test_auth.py b/integration/test_auth.py index f2e8eabe..9d5d0dfe 100644 --- a/integration/test_auth.py +++ b/integration/test_auth.py @@ -220,6 +220,10 @@ def test_get_user(new_user_with_params): assert provider_ids == ['password', 'phone'] def test_list_users(new_user_list): + err_msg_template = ( + 'Missing {field} field. A common cause would be forgetting to add the "Firebase ' + + 'Authentication Admin" permission. See instructions in CONTRIBUTING.md') + fetched = [] # Test exporting all user accounts. page = auth.list_users() @@ -229,13 +233,9 @@ def test_list_users(new_user_list): if user.uid in new_user_list: fetched.append(user.uid) assert user.password_hash is not None, ( - "Missing password_hash field. A common cause would be forgetting to add the " + - "'Firebase Authentication Admin' permission. See instructions in " + - "CONTRIBUTING.md") + err_msg_template.format(field='password_hash')) assert user.password_salt is not None, ( - "Missing password_salt field. A common cause would be forgetting to add the " + - "'Firebase Authentication Admin' permission. See instructions in " + - "CONTRIBUTING.md") + err_msg_template.format(field='password_salt')) page = page.get_next_page() assert len(fetched) == len(new_user_list) @@ -246,11 +246,9 @@ def test_list_users(new_user_list): if user.uid in new_user_list: fetched.append(user.uid) assert user.password_hash is not None, ( - "Missing password_hash field. A common cause would be forgetting to add the " + - "'Firebase Authentication Admin' permission. See instructions in CONTRIBUTING.md") + err_msg_template.format(field='password_hash')) assert user.password_salt is not None, ( - "Missing password_salt field. A common cause would be forgetting to add the " + - "'Firebase Authentication Admin' permission. See instructions in CONTRIBUTING.md") + err_msg_template.format(field='password_salt')) assert len(fetched) == len(new_user_list) def test_create_user(new_user): diff --git a/tests/test_user_mgt.py b/tests/test_user_mgt.py index c7bffd51..8b1bab13 100644 --- a/tests/test_user_mgt.py +++ b/tests/test_user_mgt.py @@ -156,7 +156,7 @@ def test_exported_record_empty_password(self): def test_redacted_passwords_cleared(self): user = auth.ExportedUserRecord({ 'localId': 'user', - 'passwordHash': base64.b64encode('REDACTED'), + 'passwordHash': base64.b64encode(b'REDACTED'), }) assert user.password_hash is None