From 83033f179e96678a67c2dee1ac1c7e244dcd9ebd Mon Sep 17 00:00:00 2001 From: Pauline Ribeyre <4224001+paulineribeyre@users.noreply.github.com> Date: Tue, 19 Apr 2022 09:01:16 -0500 Subject: [PATCH 1/5] Fix generic OIDC: return the correct user_id field (#1021) --- fence/resources/openid/idp_oauth2.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/fence/resources/openid/idp_oauth2.py b/fence/resources/openid/idp_oauth2.py index 511afe4ae..219b47bc9 100644 --- a/fence/resources/openid/idp_oauth2.py +++ b/fence/resources/openid/idp_oauth2.py @@ -168,8 +168,11 @@ def get_user_id(self, code): if claims.get(user_id_field): if user_id_field == "email" and not claims.get("email_verified"): return {"error": "Email is not verified"} - return {"sub": claims[user_id_field]} + return {user_id_field: claims[user_id_field]} else: + self.logger.exception( + f"Can't get {user_id_field} from claims: {claims}" + ) return {"error": f"Can't get {user_id_field} from claims"} except Exception as e: From ec2c72db9d5a0e144e4b9d0eee27894569e80ba0 Mon Sep 17 00:00:00 2001 From: BinamB Date: Wed, 7 Sep 2022 10:48:16 -0500 Subject: [PATCH 2/5] remove redundant assert --- tests/data/test_indexed_file.py | 99 +++++++++++++++++++++++++++++++++ 1 file changed, 99 insertions(+) diff --git a/tests/data/test_indexed_file.py b/tests/data/test_indexed_file.py index d280a3b6e..6b690d8a6 100755 --- a/tests/data/test_indexed_file.py +++ b/tests/data/test_indexed_file.py @@ -461,6 +461,105 @@ def test_internal_get_gs_signed_url_cache_new_key_if_old_key_expired( assert before_cache != after_cache +@mock.patch.object(utils, "_get_proxy_group_id", return_value=None) +@mock.patch.object(indexd, "get_or_create_proxy_group_id", return_value="1") +def test_internal_get_gs_signed_url_clear_cache_and_parse_json( + mock_get_or_create_proxy_group_id, + mock_get_proxy_group_id, + app, + indexd_client_accepting_record, + db_session, +): + """ + Test fence.blueprints.data.indexd.GoogleStorageIndexedFileLocation._generate_google_storage_signed_url + Scenario: - Create presigned url, cache in-mem and in db + - Roll pods, which removes in-mem cache but keeps db entry + - Make sure in-mem is populated correctly when creating presigned url again + + create presigned url + set cache in db + clear cache + create presigned url again + make sure cache is set correctly + """ + + indexd_record_with_non_public_authz_and_public_acl_populated = { + "urls": [f"gs://some/location"], + "authz": ["/programs/DEV/projects/test"], + "acl": ["*"], + } + indexd_client_accepting_record( + indexd_record_with_non_public_authz_and_public_acl_populated + ) + + mock_google_service_account_key = GoogleServiceAccountKey() + mock_google_service_account_key.expires = 10 + mock_google_service_account_key.private_key = "key" + sa_private_key = { + "type": "service_account", + "project_id": "project_id", + "private_key": "pdashoidhaspidhaspidhiash", # pragma: allowlist secret + } + + with mock.patch( + "fence.blueprints.data.indexd.get_or_create_primary_service_account_key", + return_value=(sa_private_key, mock_google_service_account_key), + ): + with mock.patch( + "fence.blueprints.data.indexd.create_primary_service_account_key", + return_value=(sa_private_key), + ): + with mock.patch.object( + cirrus.google_cloud.utils, + "get_signed_url", + return_value="https://cloud.google.com/compute/url", + ): + indexed_file = IndexedFile(file_id="some id") + google_object = GoogleStorageIndexedFileLocation("gs://some/location") + google_object._assume_role_cache_gs = {"1": ("key", 10)} + + before_cache = db_session.query(AssumeRoleCacheGCP).first() + + google_object._generate_google_storage_signed_url( + http_verb="GET", + resource_path="gs://some/location", + expires_in=0, + user_id=1, + username="some user", + r_pays_project=None, + ) + + assert google_object._assume_role_cache_gs["1"][0] == sa_private_key + + after_cache = db_session.query(AssumeRoleCacheGCP).first() + + assert after_cache + # check if json loads can properly parse json string stored in cache + assert "1" in google_object._assume_role_cache_gs + assert len(google_object._assume_role_cache_gs["1"]) > 1 + assert google_object._assume_role_cache_gs["1"][0] == sa_private_key + + # make sure cache is added back in the proper format after clearing + google_object._assume_role_cache_gs = {} + + google_object._generate_google_storage_signed_url( + http_verb="GET", + resource_path="gs://some/location", + expires_in=0, + user_id=1, + username="some user", + r_pays_project=None, + ) + + redo_cache = db_session.query(AssumeRoleCacheGCP).first() + + assert redo_cache + # check if json loads can properly parse json string stored in cache + assert "1" in google_object._assume_role_cache_gs + assert len(google_object._assume_role_cache_gs["1"]) > 1 + assert google_object._assume_role_cache_gs["1"][0] == sa_private_key + + def test_set_acl_missing_unauthorized( app, supported_protocol, indexd_client_accepting_record ): From 859566b196016a557b7afec60f03a14284f8ff35 Mon Sep 17 00:00:00 2001 From: Yogesh Kalbhore <7050109+tyro-ops@users.noreply.github.com> Date: Tue, 25 Oct 2022 13:52:11 -0500 Subject: [PATCH 3/5] Update config-default.yaml --- fence/config-default.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fence/config-default.yaml b/fence/config-default.yaml index 6776d4455..a37a7ee87 100755 --- a/fence/config-default.yaml +++ b/fence/config-default.yaml @@ -52,7 +52,7 @@ MOCK_AUTH: false # NOTE: this will also modify the behavior of /link/google endpoints # WARNING: DO NOT ENABLE IN PRODUCTION (for testing purposes only) # will login as the username set in cookie DEV_LOGIN_COOKIE_NAME -MOCK_GOOGLE_AUTH: false +MOCK_GOOGLE_AUTH: true DEV_LOGIN_COOKIE_NAME: "dev_login" # if true, will ignore anything configured in STORAGE_CREDENTIALS MOCK_STORAGE: true From 50b9c9db19106e7d3968e0edd4b5676a199c4e31 Mon Sep 17 00:00:00 2001 From: Yogesh Kalbhore <7050109+tyro-ops@users.noreply.github.com> Date: Tue, 25 Oct 2022 14:15:27 -0500 Subject: [PATCH 4/5] Update config-default.yaml revert change --- fence/config-default.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fence/config-default.yaml b/fence/config-default.yaml index a37a7ee87..6776d4455 100755 --- a/fence/config-default.yaml +++ b/fence/config-default.yaml @@ -52,7 +52,7 @@ MOCK_AUTH: false # NOTE: this will also modify the behavior of /link/google endpoints # WARNING: DO NOT ENABLE IN PRODUCTION (for testing purposes only) # will login as the username set in cookie DEV_LOGIN_COOKIE_NAME -MOCK_GOOGLE_AUTH: true +MOCK_GOOGLE_AUTH: false DEV_LOGIN_COOKIE_NAME: "dev_login" # if true, will ignore anything configured in STORAGE_CREDENTIALS MOCK_STORAGE: true From e36a17eb3a517f31430b114fe9f28222e1628131 Mon Sep 17 00:00:00 2001 From: Pauline Ribeyre <4224001+paulineribeyre@users.noreply.github.com> Date: Wed, 1 Mar 2023 16:37:27 -0600 Subject: [PATCH 5/5] cherry-pick 9c8b70506f59e8c9a7c33d7002aba9d1fc3c7a9c --- .secrets.baseline | 4 ++-- .../a04a70296688_non_unique_client_name.py | 21 +++++++++++-------- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/.secrets.baseline b/.secrets.baseline index 642b6e493..fa00b0286 100644 --- a/.secrets.baseline +++ b/.secrets.baseline @@ -207,7 +207,7 @@ "filename": "migrations/versions/a04a70296688_non_unique_client_name.py", "hashed_secret": "bb2372fb50034d559d2920e7229fb5879cf1be72", "is_verified": false, - "line_number": 13 + "line_number": 14 } ], "migrations/versions/e4c7b0ab68d3_create_tables.py": [ @@ -368,5 +368,5 @@ } ] }, - "generated_at": "2023-01-26T20:09:03Z" + "generated_at": "2023-02-22T17:22:09Z" } diff --git a/migrations/versions/a04a70296688_non_unique_client_name.py b/migrations/versions/a04a70296688_non_unique_client_name.py index 83b74fae1..f9ddd2783 100644 --- a/migrations/versions/a04a70296688_non_unique_client_name.py +++ b/migrations/versions/a04a70296688_non_unique_client_name.py @@ -6,6 +6,7 @@ """ from alembic import op +import logging # revision identifiers, used by Alembic. @@ -14,6 +15,8 @@ branch_labels = None depends_on = None +logger = logging.getLogger("fence.alembic") + def upgrade(): # get all "unique" constraints on "client" table @@ -22,16 +25,16 @@ def upgrade(): "SELECT conname FROM pg_constraint WHERE conrelid = 'client'::regclass and contype = 'u'" ) - # filter out the constraints that are not for the "name" column; only 1 constraint should be left + # filter out the constraints that are not for the "name" column name_constraints = [e[0] for e in results if "name" in e[0]] - if len(name_constraints) != 1: - raise Exception( - f"Found multiple 'unique client name' constraints: {name_constraints}" - ) - - # the `name` does not have to be unique anymore: remove the constraint - print(f"Droppping 'unique client name' constraint: '{name_constraints[0]}'") - op.drop_constraint(name_constraints[0], "client") + logger.info( + f"Found {len(name_constraints)} 'unique client name' constraints; deleting all of them: {name_constraints}" + ) + + # the `name` does not have to be unique anymore: remove the constraints + for name in name_constraints: + logger.info(f"Droppping 'unique client name' constraint: '{name}'") + op.drop_constraint(name, "client") def downgrade():