feat(auth): Add bulk get/delete methods #400

rsgowman · 2020-01-30T22:42:04Z

This PR allows callers to retrieve a list of users by unique identifier (uid, email, phone, federated provider uid) as well as to delete a list of users.

RELEASE NOTE: Added get_users() and delete_users() APIs for retrieving and deleting user accounts in bulk.

rsgowman · 2020-01-30T22:47:00Z

firebase_admin/_user_mgt.py

+        last_refresh_at_millis = None
+        last_refresh_at_iso8601 = self._data.get('lastRefreshAt', None)
+        if last_refresh_at_iso8601 is not None:
+            last_refresh_at_millis = iso8601.parse_date(last_refresh_at_iso8601).timestamp() * 1000


datetime.timestamp() is python 3.3. But I think that's ok now.

requirements.txt

integration/test_auth.py

hiranya911

Looks pretty good. I pointed out a few areas that can be improved.

firebase_admin/_identifier.py

firebase_admin/_user_mgt.py

integration/test_auth.py

tests/test_user_mgt.py

Replace with a custom (ugh) rfc3339 module.

hiranya911

Looks great. I think we just need to expose one of the new types in the public API.

hiranya911 · 2020-02-05T22:02:33Z

firebase_admin/_rfc3339.py

+    datestr_modified = re.sub(r'(\d\d):(\d\d)$', r'\1\2', datestr_modified)
+
+    try:
+        print("trying with micros")


Drop the print statements.

firebase_admin/_user_mgt.py

firebase_admin/auth.py

hiranya911 · 2020-02-05T22:17:48Z

Adding @egilmorez to review the documentation bits in auth.py

hiranya911

LGTM with one suggestion about the public API.

I'm also ok with updating the docstring of ErrorInfo to read some like "Error encountered while performing a batch operation such as importing users or deleting multiple user accounts" and then use it instead of the new BatchDeleteErrorInfo.

firebase_admin/auth.py

egilmorez · 2020-02-18T23:56:29Z

firebase_admin/_user_mgt.py

+
+    def __init__(self, errors=None):
+        """Constructs a BatchDeleteAccountsResponse instance, corresponseing to
+        the json representing the BatchDeleteAccountsResponse proto.


Suggest "JSON"

egilmorez · 2020-02-18T23:56:45Z

firebase_admin/_user_mgt.py

+    """Represents the results of a delete_users() call."""
+
+    def __init__(self, errors=None):
+        """Constructs a BatchDeleteAccountsResponse instance, corresponseing to


Suggest backticks for literal, here and just below.

Done (also fixed spelling typo)

egilmorez · 2020-02-18T23:57:20Z

firebase_admin/_user_mgt.py

+
+        Args:
+            errors: List of dictionaries, with each dictionary representing a
+                ErrorInfo instance as returned by the server. None implies an


Suggest backticks for literal, and "an ErrorInfo instance..."

egilmorez · 2020-02-18T23:57:41Z

firebase_admin/_user_mgt.py

+
+        Returns:
+            list[dict[string, string]]: List of dicts representing the json
+            UserInfo responses from the server.


Suggest backticks for literal.

Done. (Also json->JSON)

egilmorez · 2020-02-19T00:00:21Z

firebase_admin/auth.py

+    result list is not guaranteed to correspond to the nth entry in the input
+    parameters list.
+
+    Only a maximum of 100 identifiers may be supplied. If more than 100


Suggest "A maximum . . . "

egilmorez · 2020-02-19T00:00:52Z

firebase_admin/auth.py

+    parameters list.
+
+    Only a maximum of 100 identifiers may be supplied. If more than 100
+    identifiers are supplied, this method will immediately raise a ValueError.


Suggest "method raises a ValueError.

egilmorez · 2020-02-19T00:01:51Z

firebase_admin/auth.py

+    DeleteUserResult.success_count value.
+
+    Only a maximum of 1000 identifiers may be supplied. If more than 1000
+    identifiers are supplied, this method will immediately raise a ValueError.


Suggest "method raises a ValueError.

egilmorez · 2020-02-19T00:02:10Z

firebase_admin/auth.py

+    """Deletes the users specified by the given identifiers.
+
+    Deleting a non-existing user won't generate an error. (i.e. this method is
+    idempotent.) Non-existing users will be considered to be successfully


Suggest "are considered"

egilmorez

Some minor style suggestions for parsed comments. Thanks!

…lete

cbonnie

Left small comment, otherwise LGTM. (Reviewing on behalf of egilmore@)

cbonnie · 2020-03-09T15:06:09Z

firebase_admin/_user_mgt.py

+
+    @property
+    def errors(self):
+        """A list of ``auth.ErrorInfo`` instances describing the errors that


Just checking, is this supposed to be in double back ticks?

No idea! It seems both double and single backticks are used throughout. Single backticks are more common though, so I switched to that.

cbonnie · 2020-03-09T15:32:29Z

firebase_admin/auth.py

+    Deleting a non-existing user won't generate an error. (i.e. this method is
+    idempotent.) Non-existing users are considered to be successfully
+    deleted, and will therefore be counted in the
+    `DeleteUserResult.success_count` value.


Suggest:

"Deleting a non-existing user does not generate an error (the method is idempotent). Non-existing users are considered to be successfully deleted and are therefore included in the DeleteUserResult.success_count value."

…lete

hiranya911

Looks quite good. I just made a few suggestions. See if those make sense for this PR.

firebase_admin/_user_identifier.py

hiranya911 · 2020-05-08T22:17:50Z

firebase_admin/_user_identifier.py

+        Args:
+            uid: A user ID string.
+        """
+        self.uid = uid


Do we need to validate these arguments? (Not empty, not None, string type etc)

It can't hurt, though implies we'll be doing some of the validation twice. (I don't think that's a problem.) Done. (Also, no longer doing validation twice; see other comment).

Note that we don't validate arguments to (eg) auth.get_user_by_email(), instead, just deferring to the implementation function (user_mgt.get_user). This is a slightly different scenario, since for UserIdentifier, the user can create that ahead of time and possibly use it in some other context.

Another improvement (that I don't want to make in this PR) is to use type annotations. All of our supported python versions now support type annotations and they'd give you some of this for free (and at "compile" time too.)

firebase_admin/_auth_client.py

hiranya911 · 2020-05-08T22:29:51Z

firebase_admin/_user_mgt.py

+        payload = {}
+        for identifier in identifiers:
+            if isinstance(identifier, _user_identifier.UidIdentifier):
+                _auth_utils.validate_uid(identifier.uid, required=True)


What if we push this validation and the subsequent marshaling to the corresponding classes?

Each identifier class can expose something like:

def marshal(self, payload): payload['localId'] = payload.get('localId', []).append(self.uid)

I did this (just now) but didn't really like the results. It makes this function shorter, but it alters the UserIdentifier types such that they need to know how the json payload is constructed. (For instance, should it be ProviderIdentifiers job to know that provider_uid maps to rawId on the wire?) It also spreads building the payload across 2 files and 5 classes, instead of having it in just one spot.

One possible improvement we could make here is to eliminate the validation (or at least, refactor it somewhat). As per one of your other comments, I've already moved some validation to the ctors, but that won't stop users from changing the values after construction, but before calling get_users(), so we should keep the validation here. But if we altered (eg) UidIdentifier.uid to be UidIdentifier._uid and added a getter (essentially making these objects immutable) I think that could allow us to skip validation here entirely, which would allow this block to focus solely on creating the payload. I've done that. PTAL.

I like how validation turned out (and the immutability of the properties). I don't feel strongly about the marshalling part. When I first suggested it, I felt that having each class being in charge of marshalling was a good thing. If we add more implementations of the UserIdentifier interface in the future, that's when this will matter more. I'm ok with leaving this as is for now.

firebase_admin/_user_mgt.py

hiranya911 · 2020-05-11T19:26:12Z

firebase_admin/_user_identifier.py

@@ -30,7 +32,12 @@ def __init__(self, uid):
        Args:
            uid: A user ID string.
        """
-        self.uid = uid
+        _auth_utils.validate_uid(uid, required=True)
+        self._uid = uid


You can collapse these into a single line:

self._uid = _auth_utils.validate_uid(...)

Same below.

firebase_admin/_user_mgt.py

hiranya911

Thanks. LGTM with a suggestion.

Also fix multi-line Returns: doc statements. (See sphinx-contrib/napoleon#4)

rsgowman self-assigned this Jan 30, 2020

rsgowman added 4 commits January 30, 2020 18:37

Add get_users() bulk lookup method

aa8207d

Add delete_users() bulk deletion method

a83f66e

add last_refresh_timestamp

c5f57d0

Fix lint error

0e282f7

rsgowman force-pushed the rsgowman/bulk_getdelete branch from 6913eca to 0e282f7 Compare January 30, 2020 23:37

rsgowman added 2 commits January 30, 2020 18:41

fixup! Add delete_users() bulk deletion method

070c1a8

Remove force_delete flag

466890b

rsgowman commented Jan 30, 2020

View reviewed changes

rsgowman requested a review from hiranya911 January 30, 2020 23:44

rsgowman assigned hiranya911 and unassigned rsgowman Jan 30, 2020

hiranya911 suggested changes Jan 31, 2020

View reviewed changes

hiranya911 assigned rsgowman and unassigned hiranya911 Jan 31, 2020

rsgowman added 3 commits February 5, 2020 10:42

review feedback (except iso8601)

598ffc9

Add (missing) bulk lookup by provider

1993007

Remove iso8601 dependency.

0b18203

Replace with a custom (ugh) rfc3339 module.

rsgowman assigned hiranya911 and unassigned rsgowman Feb 5, 2020

hiranya911 reviewed Feb 5, 2020

View reviewed changes

hiranya911 assigned egilmorez and rsgowman and unassigned hiranya911 Feb 5, 2020

hiranya911 requested a review from egilmorez February 5, 2020 22:18

review feedback 2

65d7d63

rsgowman assigned hiranya911 and unassigned rsgowman Feb 5, 2020

hiranya911 approved these changes Feb 6, 2020

View reviewed changes

firebase_admin/auth.py Show resolved Hide resolved

egilmorez reviewed Feb 18, 2020

View reviewed changes

egilmorez reviewed Feb 19, 2020

View reviewed changes

rsgowman added 2 commits February 20, 2020 12:55

(mostly) doc review feedback

d4a8a3e

Merge remote-tracking branch 'origin/master' into rsgowman/bulk_getde…

3c6b776

…lete

cbonnie approved these changes Mar 9, 2020

View reviewed changes

cbonnie reviewed Mar 9, 2020

View reviewed changes

rsgowman and others added 5 commits May 7, 2020 14:55

Merge remote-tracking branch 'origin/master' into rsgowman/bulk_getde…

2167764

…lete

Fixes to address changes since last merge master

9ade7a8

Doc nits

278ee4a

lint

7087016

Merge remote-tracking branch 'origin/master' into rsgowman/bulk_getde…

5990d44

…lete

hiranya911 suggested changes May 8, 2020

View reviewed changes

hiranya911 assigned rsgowman May 8, 2020

more review feedback

ba60c65

hiranya911 reviewed May 11, 2020

View reviewed changes

combine validate/assign in ctor

e9dafc1

hiranya911 approved these changes May 11, 2020

View reviewed changes

egilmorez approved these changes May 12, 2020

View reviewed changes

rsgowman merged commit d1aae52 into master May 12, 2020

rsgowman deleted the rsgowman/bulk_getdelete branch May 12, 2020 20:57

hiranya911 added the release-note label May 12, 2020

rsgowman added a commit that referenced this pull request May 12, 2020

Followup to #400 to ensure all new types are exported

e38598c

rsgowman added a commit that referenced this pull request May 13, 2020

Followup to #400 to ensure all new types are exported (#461)

f8b1fff

Also fix multi-line Returns: doc statements. (See sphinx-contrib/napoleon#4)

feat(auth): Add bulk get/delete methods #400

feat(auth): Add bulk get/delete methods #400

Conversation

rsgowman commented Jan 30, 2020 • edited by hiranya911 Loading

Choose a reason for hiding this comment

hiranya911 left a comment

Choose a reason for hiding this comment

hiranya911 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiranya911 commented Feb 5, 2020

hiranya911 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egilmorez left a comment

Choose a reason for hiding this comment

cbonnie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbonnie Mar 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiranya911 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsgowman May 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiranya911 left a comment

Choose a reason for hiding this comment

rsgowman commented Jan 30, 2020 •

edited by hiranya911

Loading

cbonnie Mar 9, 2020 •

edited

Loading

rsgowman May 11, 2020 •

edited

Loading