[PXP-3384] fix(patch-sa): add all granting_project_ids (#670)

* fix(patch-sa): add all granting_project_ids

* fix(patch-sa): continue to use set diff in to_add but for db only

* fix(patch-sa): update tests to reflect new patch logic

* fix(patch-sa): rm deprecated check, edit docstring

fence/resources/google/access_utils.py

@@ -498,11 +498,20 @@ def patch_user_service_account(
     _revoke_user_service_account_from_google(
         session, to_delete, google_project_id, service_account
     )
+
+    # Use granting_project_ids here, not to_add, bc the google-delete-expired-service-account
+    # job doesn't clean out the entries in the ServiceAccountAccessPrivilege table.
+    # So the set diff (=to_add) won't include the proj if the SA was previously registered for that proj,
+    # even if the SA later expired and was removed from the relevant GBAG.
     add_user_service_account_to_google(
-        session, to_add, google_project_id, service_account
+        session, granting_project_ids, google_project_id, service_account
     )
+
     _revoke_user_service_account_from_db(session, to_delete, service_account)
 
+    # On the other hand, use to_add here and not granting_project_ids,
+    # otherwise this will add duplicates to ServiceAccountAccessPrivilege.
+    # Because at time of writing, aforementioned tbl has no compound unique constraint.
     add_user_service_account_to_db(session, to_add, service_account)
 
 

tests/google/test_access_utils.py

@@ -298,7 +298,7 @@ def test_update_user_service_account_success(cloud_manager, db_session, setup_da
     """
     (
         cloud_manager.return_value.__enter__.return_value.add_member_to_group.return_value
-    ) = {}
+    ) = {"email": "test@gmail.com"}
     (
         cloud_manager.return_value.__enter__.return_value.remove_member_from_group.return_value
     ) = {}
@@ -405,18 +405,17 @@ def test_update_user_service_account_success2(cloud_manager, db_session, setup_d
 def test_update_user_service_account_success3(cloud_manager, db_session, setup_data):
     """
     test@gmail.com has access to test_auth_1 and test_auth_2 already
-    Test that there is no add and delete operations when client try to update
+    Test that there are no delete operations when client try to update
     with projects already granted access
+    (This test used to also check that no Google-side add operations occurred
+    given the same situation, but this is no longer expected as we are now
+    adding SA to every project/GBAG every time--see #670)
     """
     service_account = (
         db_session.query(UserServiceAccount).filter_by(email="test@gmail.com").first()
     )
     patch_user_service_account("test", "test@gmail.com", ["test_auth_1", "test_auth_2"])
 
-    assert not (
-        cloud_manager.return_value.__enter__.return_value.add_member_to_group.called
-    )
-
     assert not (
         cloud_manager.return_value.__enter__.return_value.remove_member_from_group.called
     )