feat(passport-exp): handle expiration edge cases [PXP-9235] #988

Avantol13 · 2021-11-29T22:11:36Z

PXP-9235

In order to better handle updating/expiration of authorization from Passports, this PR has number of pieces.

Adjusts overall architecture such that:

usersync no longer updates visas
Access Token Polling gets updated passport(s) for a user and updates authorization and visas. It also sets expiration for authorization.
A frequently-running cronjob handles the removal of stale, expired visas in the database

This PR also adds a variety of refactoring and updating around the above 3 to consolidate code, clarify logic, and fix unit tests.

Relies on uc-cdis/cloud-automation#1806

New Features

usersync no longer updates visas and Access Token Polling now both gets updated passport(s) for a user and updates authorization
A frequently-running cronjob handles the removal of expired visas

Breaking Changes

URL Signing when no_force_sign query param is provided: Previously Fence would make a decision based off if the data was public and no_force_sign provided. Fence will now NEVER sign if no_force_sign is provided (since the concept of "public" data has been abstracted out of Fence. Data access - public or not - is the sole responsibility of the policy engine). In other words, if no_force_sign is provided at the API level, Fence will respect that regardless of whether the resulting URL will actually work. The default Fence behavior should remain the same.

Bug Fixes

fix issue with anonymous Google signed URL generation (would fail with unauthenticated requests even if the data was supposed to be accessible)

Improvements

various refactoring around Passports

Dependency updates

Deployment changes

…ad' into feat/passport-exp

github-actions · 2021-11-29T22:12:17Z

The style in this PR agrees with black. ✔️

This formatting comment was generated automatically by a script in uc-cdis/wool.

coveralls · 2021-11-29T22:17:29Z

Pull Request Test Coverage Report for Build 12229

254 of 303 (83.83%) changed or added relevant lines in 11 files are covered.
51 unchanged lines in 8 files lost coverage.
Overall coverage decreased (-0.05%) to 72.493%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
fence/resources/openid/idp_oauth2.py	0	1	0.0%
fence/resources/openid/ras_oauth2.py	41	42	97.62%
fence/resources/ga4gh/passports.py	54	60	90.0%
fence/resources/google/utils.py	18	24	75.0%
fence/blueprints/data/indexd.py	62	69	89.86%
fence/blueprints/login/ras.py	7	14	50.0%
fence/scripting/fence_create.py	23	31	74.19%
fence/sync/sync_users.py	28	41	68.29%

Files with Coverage Reduction	New Missed Lines	%
fence/resources/openid/ras_oauth2.py	1	58.65%
fence/resources/admin/admin_projects.py	2	79.41%
fence/scripting/fence_create.py	2	57.58%
fence/sync/passport_sync/ras_sync.py	2	86.49%
fence/blueprints/login/ras.py	3	78.85%
fence/job/visa_update_cronjob.py	6	87.91%
fence/resources/userdatamodel/userdatamodel_project.py	9	59.26%
fence/sync/sync_users.py	26	73.54%

Totals
Change from base Build 12200:	-0.05%
Covered Lines:	6681
Relevant Lines:	9216

💛 - Coveralls

…to fix up unit tests

…ser.username, improve error handling

Avantol13 · 2021-12-06T20:56:35Z

fence/blueprints/data/indexd.py

@@ -46,10 +45,10 @@
    get_google_app_creds,
    give_service_account_billing_access_if_necessary,
 )
-from fence.resources.ga4gh.passports import get_gen3_users_from_ga4gh_passports
+from fence.resources.ga4gh.passports import sync_gen3_users_authz_from_ga4gh_passports


more descriptive to show that it's changing database and not just "get"-ing information

Avantol13 · 2021-12-06T20:56:57Z

fence/blueprints/data/indexd.py

@@ -62,15 +61,21 @@

 SUPPORTED_PROTOCOLS = ["s3", "http", "ftp", "https", "gs", "az"]
 SUPPORTED_ACTIONS = ["upload", "download"]
-ANONYMOUS_USER_ID = "anonymous"
+ANONYMOUS_USER_ID = "-1"


the fact that the user_id was not an integer was causing downstream validation problems since User.id is int in the database

Avantol13 · 2021-12-06T20:58:30Z

fence/blueprints/data/indexd.py

@@ -517,12 +526,28 @@ def set_acls(self):
        else:
            raise Unauthorized("This file is not accessible")

-    def check_authz(self, action, user_ids_from_passports=None):
+    def get_authorized_and_user_id(self, action, user_ids_from_passports=None):


more descriptive / consistent return for this functionality

Avantol13 · 2021-12-06T20:59:47Z

fence/blueprints/login/ras.py

@@ -37,76 +39,52 @@ def __init__(self):
        )

    def post_login(self, user=None, token_result=None, id_from_idp=None):
-        # TODO: I'm not convinced this code should be in post_login.


for this file, the goal was to reduce duplication and replace this additional custom logic with the already built sync_gen3_users_authz_from_ga4gh_passports functionality as much as possible

Avantol13 · 2021-12-06T21:02:15Z

fence/resources/openid/ras_oauth2.py

@@ -270,91 +285,31 @@ def map_iss_sub_pair_to_user(self, issuer, subject_id, username, email):
            return iss_sub_pair_to_user.user.username

    @backoff.on_exception(backoff.expo, Exception, **DEFAULT_BACKOFF_SETTINGS)
-    def update_user_visas(self, user, pkey_cache, db_session=current_session):
+    def update_user_authorization(self, user, pkey_cache, db_session=current_session):


for this file, the goal was to reduce duplication and replace this additional custom logic with the already built sync_gen3_users_authz_from_ga4gh_passports functionality as much as possible

Avantol13 · 2021-12-06T21:03:02Z

fence/scripting/fence_create.py

@@ -210,7 +239,6 @@ def init_syncer(
    sync_from_local_yaml_file=None,
    arborist=None,
    folder=None,
-    sync_from_visas=False,


usersync no longer provides the option to sync from visas. This is handled entirely by the access token polling job

Avantol13 · 2021-12-06T21:03:45Z

fence/sync/passport_sync/ras_sync.py

-                    info["tags"] = {"dbgap_role": permission.get("role", "")}
-        else:
-            # Remove visas if its invalid or expired
-            user.ga4gh_visas_v1 = []


don't mess with db here, handle upstream

Avantol13 · 2021-12-06T21:05:05Z

fence/sync/sync_users.py

@@ -1954,196 +1952,24 @@ def parse_user_visas(self, db_session):

        return (user_projects, user_info)

-    def _sync_visas(self, sess):


this is handled entirely by access token polling and sync_gen3_users_authz_from_ga4gh_passports now

Avantol13 · 2021-12-06T21:05:56Z

tests/dbgap_sync/conftest.py

@@ -221,8 +221,8 @@ def add_visa_manually(db_session, user, rsa_private_key, kid):
        "ga4gh_visa_v1": {
            "type": "https://ras.nih.gov/visas/v1",
            "asserted": int(time.time()),
-            "value": "https://nig/passport/dbgap",
-            "source": "https://ncbi/gap",
+            "value": "https://stsstg.nih.gov/passport/dbgap/v1.1",


A lot of tests had to change so that the fake visas were actually "valid", since sync_gen3_users_authz_from_ga4gh_passports validates all the Visas

…in usersync), ensure single visa sync is only updating storage backend and arborist

…mprove handling of IdPs throughout

Avantol13 · 2022-01-21T22:37:33Z

fence/blueprints/data/indexd.py

-            return self.public_authz
-        else:
-            return self.public_acl
-
    @cached_property
    def public_acl(self):


need this for some backwards compatibility

Avantol13 · 2022-01-21T22:38:34Z

tests/data/test_azure_blob_storage_indexed_file_location.py

-        ("upload", 5, None, False, "fake conn str", "some user", False, False),
-        ("upload", 5, True, None, "fake conn str", ANONYMOUS_USER_ID, True, False),
-        ("upload", 5, True, None, "fake conn str", ANONYMOUS_USER_ID, False, False),
+        ("download", 5, None, "fake conn str", "some user", False, False),


remove public_data as an option, remove duplicate tests, and adjust expected behavior to meet new expectations. See "Breaking Changes" in the PR description

Maybe the swagger doc could be updated too?

johnfrancismccann · 2022-01-25T15:24:30Z

fence/blueprints/login/ras.py

+            users_from_passports = fence.resources.ga4gh.passports.sync_gen3_users_authz_from_ga4gh_passports(
+                [passport],
+                authz_policy_prefix="POST.LOGIN",
+                pkey_cache=pkey_cache,


I tried logging in and am getting: NameError: name 'pkey_cache' is not defined for this line.

Avantol13-machine-user added 5 commits November 15, 2021 09:35

feat(passport): refactoring and removal of usersync passport sync

1364aca

Merge branch 'feat/ras-milestone-3.2-add-visa-parsing-for-data-downlo…

e10a8bb

…ad' into feat/passport-exp

Merge branch 'feat/passport' into feat/passport-exp

1df84f7

Merge branch 'feat/passport' into feat/passport-exp

c40e1c8

Merge branch 'feat/passport' into feat/passport-exp

3bd9267

Avantol13 marked this pull request as draft November 29, 2021 22:11

Avantol13-machine-user added 6 commits December 1, 2021 14:15

Merge branch 'feat/passport' into feat/passport-exp

8eadb2a

fix(passports): refactoring/cleanup, improve session handling, start …

e1fb642

…to fix up unit tests

fix(google): allow looking up proxy group information by user.id or u…

1134c01

…ser.username, improve error handling

fix(google): allow looking up proxy group information by user.id or u…

8814c78

…ser.username, improve error handling

fix(indexd): pass correct param

b85dc76

fix(tests): clear out user-related tables between tests

4940ff8

Avantol13 commented Dec 6, 2021

View reviewed changes

Avantol13-machine-user added 7 commits December 6, 2021 16:29

feat(ga4gh): add fence-create job for cleaning up expired visas

e89221b

feat(fence-create): add command to run cleanup for ga4gh info

363ba3e

chore(sync): remove unused features (fallback to dbgap and visa sync …

011721c

…in usersync), ensure single visa sync is only updating storage backend and arborist

Merge branch 'feat/passport' into feat/passport-exp

ff3960e

Merge branch 'feat/passport' into feat/passport-exp

de3dc28

chore(deps): update dependencies

8989bad

feat(visa_sync): choose client based on identity_provider for user, i…

f1f70f9

…mprove handling of IdPs throughout

Avantol13 commented Jan 21, 2022

View reviewed changes

Avantol13 added the test-apis-dbgapTest label Jan 24, 2022

Avantol13 changed the title ~~feat(passport-exp): handle expiration edge cases~~ feat(passport-exp): handle expiration edge cases [PXP-9235] Jan 24, 2022

johnfrancismccann reviewed Jan 25, 2022

View reviewed changes

Avantol13 removed the test-apis-dbgapTest label Jan 25, 2022

fix(login): initialize in-memory public key cache for RAS login

7d4edaa

Avantol13 added test-apis-dbgapTest test-google-googleServiceAccountKeyTest test-google-googleServiceAccountTest test-google-googleServiceAccountRemovalTest labels Jan 25, 2022

fix(passport): don't close session as it needs to be used downstream

7ad2256

Avantol13 removed test-google-googleServiceAccountTest test-apis-dbgapTest test-google-googleServiceAccountKeyTest test-google-googleServiceAccountRemovalTest labels Jan 25, 2022

johnfrancismccann approved these changes Jan 25, 2022

View reviewed changes

Avantol13 merged commit 5d214c7 into feat/passport Jan 26, 2022

Avantol13 deleted the feat/passport-exp branch January 26, 2022 16:44

Avantol13 mentioned this pull request Jan 26, 2022

Feat/passport #964

Merged

15 tasks

This was referenced Jun 23, 2022

Gen3 Monthly Release 2022.06 tbi.datacommons.io 1655955061 uc-cdis/cdis-manifest#4792

Closed

Gen3 Monthly Release 2022.06 gen3-neuro.datacommons.io 1655958659 uc-cdis/cdis-manifest#4793

Closed

PlanXCyborg mentioned this pull request Jul 21, 2022

Update qa-anvil to 2022.07 qa-anvil.planx-pla.net - 2022.07 1658416156 uc-cdis/gitops-qa#2107

Open

PlanXCyborg mentioned this pull request Sep 14, 2022

chore/UpdateBloodpac: update servaces to match production versions uc-cdis/gitops-qa#2187

Merged

PlanXCyborg mentioned this pull request Oct 21, 2022

Gen3 Monthly Release 2022.10 dcf-interop.kidsfirstdrc.org 1666320713 uc-cdis/cdis-manifest#5281

Closed

github-actions bot mentioned this pull request Aug 15, 2024

Updated fence to expose metrics uc-cdis/cdis-manifest#7742

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(passport-exp): handle expiration edge cases [PXP-9235] #988

feat(passport-exp): handle expiration edge cases [PXP-9235] #988

Avantol13 commented Nov 29, 2021 •

edited

Loading

github-actions bot commented Nov 29, 2021

coveralls commented Nov 29, 2021 •

edited

Loading

Avantol13 Dec 6, 2021

Avantol13 Dec 6, 2021 •

edited

Loading

Avantol13 Dec 6, 2021

Avantol13 Dec 6, 2021

Avantol13 Dec 6, 2021

Avantol13 Dec 6, 2021

Avantol13 Dec 6, 2021

Avantol13 Dec 6, 2021

Avantol13 Dec 6, 2021

Avantol13 Jan 21, 2022

Avantol13 Jan 21, 2022

johnfrancismccann Jan 25, 2022

johnfrancismccann Jan 25, 2022

		@@ -1954,196 +1952,24 @@ def parse_user_visas(self, db_session):

		return (user_projects, user_info)

		def _sync_visas(self, sess):

feat(passport-exp): handle expiration edge cases [PXP-9235] #988

feat(passport-exp): handle expiration edge cases [PXP-9235] #988

Conversation

Avantol13 commented Nov 29, 2021 • edited Loading

New Features

Breaking Changes

Bug Fixes

Improvements

Dependency updates

Deployment changes

github-actions bot commented Nov 29, 2021

coveralls commented Nov 29, 2021 • edited Loading

Pull Request Test Coverage Report for Build 12229

💛 - Coveralls

Choose a reason for hiding this comment

Avantol13 Dec 6, 2021 • 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

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

Avantol13 commented Nov 29, 2021 •

edited

Loading

coveralls commented Nov 29, 2021 •

edited

Loading

Avantol13 Dec 6, 2021 •

edited

Loading