(PXP:9493):(feat)Non-dbgap whitelist in usersync #1020

BinamB · 2022-04-14T18:40:57Z

New Features

Add non-dbgap whitelist support in usersync.

…ustom-data-access

github-actions · 2022-04-14T18:45:56Z

The style in this PR agrees with black. ✔️

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

coveralls · 2022-04-14T18:51:10Z

Pull Request Test Coverage Report for Build 12645

27 of 30 (90.0%) changed or added relevant lines in 1 file are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+0.05%) to 73.764%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
fence/sync/sync_users.py	27	30	90.0%

Files with Coverage Reduction	New Missed Lines	%
fence/sync/sync_users.py	1	74.66%

Totals
Change from base Build 12642:	0.05%
Covered Lines:	6818
Relevant Lines:	9243

💛 - Coveralls

Avantol13 · 2022-04-14T18:54:22Z

fence/config-default.yaml

@@ -904,6 +904,8 @@ SERVICE_ACCOUNT_LIMIT: 6
 GLOBAL_PARSE_VISAS_ON_LOGIN:
 # Settings for usersync with visas
 USERSYNC:
+  # allowed list of custom project ids that can be processed by usersync
+  allowed_custom_project_ids: ["CUSTOM-*"]


is this necessary? I don't see where this is used

Avantol13 · 2022-04-14T18:56:34Z

fence/sync/sync_users.py

@@ -509,7 +523,7 @@ def _parse_csv(self, file_dict, sess, dbgap_config={}, encrypted=True):
                        continue

                    phsid_privileges = {}
-                    phsid = row.get("phsid", "").split(".")
+                    phsid = row.get("phsid", row.get("project_id", "")).split(".")


why the change here? does that row exist in dbgap files? If so this is potentially a change in behavior

oh I see the new file might have a different row name. This should be configuration

Avantol13 · 2022-04-14T18:59:43Z

tests/test-fence-config.yaml

@@ -341,6 +341,8 @@ dbGaP:
      proxy_user: ''
    protocol: 'sftp'
    decrypt_key: ''
+    allow_non_dbGaP_whitelist: false
+    allowed_id_patterns: ['PROJECT-12345']


this is somewhat confusingly named, since this is for file matching. I think this should be a full regex to match the filename and default it to ['authentication_file_phs(\d{6})', 'authentication_file_PROJECT-*']' or something. There's too much custom logic in the code for someone to know by looking at the configuration how to set this up or what it means. Probably deserves a comment about it as well

Avantol13 · 2022-04-14T19:00:00Z

tests/test-fence-config.yaml

@@ -368,6 +370,8 @@ dbGaP:
      'phs000178': ['/orgA/', '/orgB/', '/']
      # study when parsing consent codes
      'phs000178.c2': ['/orgA/', '/orgB/', '/']
+      # non dbgap study:
+      'PROJECT-12345': ['']


I assume this is optional?

…ustom-data-access

Avantol13 · 2022-06-27T20:01:42Z

fence/sync/sync_users.py

+                pattern += ".enc"
+            pattern += "$"
+            if re.match(pattern, os.path.basename(filepath)):
+                return True


is this intended to return early on the first matched pattern? Or should this only return True if ALL patterns match that are passed in?

Avantol13 · 2022-06-27T20:03:05Z

fence/sync/sync_users.py

@@ -527,7 +543,11 @@ def _parse_csv(self, file_dict, sess, dbgap_config={}, encrypted=True):
                        continue

                    phsid_privileges = {}
-                    phsid = row.get("phsid", "").split(".")
+                    if self.non_dbGaP_whitelist:


should you check allow_non_dbgap_whitelist?

Avantol13 · 2022-06-27T20:04:30Z

tests/dbgap_sync/test_user_sync.py

+    )
+    monkeypatch.setattr(syncer, "non_dbGaP_whitelist", allow_non_dbgap_whitelist)
+    # merge sftp server containing both dbgap and non-dbgap whitelists
+    monkeypatch.setattr(syncer, "is_sync_from_dbgap_server", allow_non_dbgap_whitelist)


is this used?

Avantol13 · 2022-06-27T20:05:14Z

tests/dbgap_sync/test_user_sync.py

@@ -633,7 +711,7 @@ def mock_merge(dbgap_servers, sess):

    # this function will be called once for each sftp server
    # the test config file has 2 dbgap sftp servers


Suggested change

# the test config file has 2 dbgap sftp servers

# the test config file has 3 dbgap sftp servers

Avantol13 · 2022-06-27T20:05:22Z

tests/test-fence-config.yaml

@@ -662,6 +676,10 @@ EXPIRED_AUTHZ_REMOVAL_JOB_FREQ_IN_SECONDS: 1
 GLOBAL_PARSE_VISAS_ON_LOGIN:
 # Settings for usersync with visas
 USERSYNC:
+  non_dbgap_whitelist_enabled: false


why have both this and the per-sftp config allow_non_dbGaP_whitelist?

Avantol13 · 2022-07-05T21:31:47Z

tests/test-fence-config.yaml

@@ -662,6 +676,9 @@ EXPIRED_AUTHZ_REMOVAL_JOB_FREQ_IN_SECONDS: 1
 GLOBAL_PARSE_VISAS_ON_LOGIN:
 # Settings for usersync with visas
 USERSYNC:
+  sync_from_visas: false


I think these were removed since our merge of feat/passport, can you double check we actually need this and the one below?

Avantol13 · 2022-07-05T21:33:07Z

tests/dbgap_sync/test_user_sync.py

 @pytest.mark.parametrize("syncer", ["google", "cleversafe"], indirect=True)
-@pytest.mark.parametrize("parse_consent_code_config", [False, True])
+@pytest.mark.parametrize("parse_consent_code_config", [True, False])


Is this just to be consistently passing true then false or was there some real issue when it was ordered the other way?

oh woops. I was messing around with it and reversed the order. I'll change it back.

…fence into feat/custom-data-access

Avantol13 · 2022-07-12T16:45:42Z

fence/sync/sync_users.py

+            pattern += "$"
+            pattern = pattern.encode().decode(
+                "unicode_escape"
+            )  # when converting from yaml, python reads it as Python string literal. So "\" turns into "\\" which messes with the regex match


can you move this above the line, I generally try to avoid inline comments unless absolutely necessary

also, why is there YAML at all now?

Oh should I keep the comment and instead of "yaml", call it fence-config or should I just completely get rid of the comment?

I think the comment is helpful, let's clarify that it's YAML from the fence-config

Avantol13 · 2022-07-12T16:48:55Z

fence/sync/sync_users.py

@@ -527,9 +542,13 @@ def _parse_csv(self, file_dict, sess, dbgap_config={}, encrypted=True):
                        continue

                    phsid_privileges = {}
-                    phsid = row.get("phsid", "").split(".")
+                    if dbgap_config.get("allow_non_dbGaP_whitelist", False):
+                        phsid = row.get("phsid", row.get("project_id", "")).split(".")


should this be configurable? Or I guess we can just require that people either use phsid or project_id, but this feels like something that could be a cfg

the way usersync is written right now makes ait a little difficult for us to have a per sftp server for picking phsid and project_id. We could include it in the next design for usersync that i want to do for fix-it-Friday though?

yeah that's fine

Avantol13 · 2022-07-12T16:49:41Z

fence/sync/sync_users.py

+    def _merge_multiple_local_csv_files(
+        self, dbgap_file_list, encrypted, dbgap_configs, session
+    ):
+


maybe a quick docstring here?

…ustom-data-access

BinamB added 11 commits February 18, 2022 10:43

allow downloading custom phsids

485cc34

install vim on docker build

3cfae50

fix docker

bce50df

black

d681451

fix vim install

0fa758f

encrypted

371ace1

fix typo

c81e2a9

fix logs

0cdf78c

add test

0437d33

change phsid to project_id

9c90043

Merge branch 'master' of https://github.com/uc-cdis/fence into feat/c…

492a969

…ustom-data-access

BinamB changed the title ~~Feat/custom data access~~ (PXP:9493):(feat)Non-dbgap whitelist in usersync Apr 14, 2022

Avantol13 requested changes Apr 14, 2022

View reviewed changes

BinamB and others added 3 commits May 9, 2022 11:33

Merge branch 'master' into feat/custom-data-access

16bbe64

Resolve Reivew

eb16450

Merge branch 'master' of https://github.com/uc-cdis/fence into feat/c…

5f7d11a

…ustom-data-access

Avantol13 reviewed Jun 27, 2022

View reviewed changes

BinamB and others added 3 commits June 29, 2022 13:26

Merge branch 'master' into feat/custom-data-access

04e6063

Fix all the things

85bb785

Merge branch 'master' into feat/custom-data-access

15d43d7

Avantol13 requested changes Jul 5, 2022

View reviewed changes

Avantol13 reviewed Jul 5, 2022

View reviewed changes

BinamB added 2 commits July 5, 2022 16:48

separate basename

1afd7c2

Merge branch 'feat/custom-data-access' of https://github.com/uc-cdis/…

ba1c456

…fence into feat/custom-data-access

BinamB added 2 commits July 6, 2022 15:04

small changes

0aa3d45

add unicode convert

c480e0c

Avantol13 reviewed Jul 12, 2022

View reviewed changes

doc string + comment fix

d99ceca

Avantol13 previously approved these changes Jul 12, 2022

View reviewed changes

PlanXCyborg added test-apis-etlTest test-google-googleServiceAccountRemovalTest and removed test-google-googleServiceAccountRemovalTest labels Jul 13, 2022

Merge branch 'master' of https://github.com/uc-cdis/fence into feat/c…

2add969

…ustom-data-access

BinamB dismissed Avantol13’s stale review via 2add969 July 14, 2022 16:45

Avantol13 approved these changes Jul 14, 2022

View reviewed changes

BinamB merged commit c720f21 into master Jul 14, 2022

BinamB deleted the feat/custom-data-access branch July 14, 2022 18:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(PXP:9493):(feat)Non-dbgap whitelist in usersync #1020

(PXP:9493):(feat)Non-dbgap whitelist in usersync #1020

BinamB commented Apr 14, 2022

github-actions bot commented Apr 14, 2022

coveralls commented Apr 14, 2022 •

edited

Avantol13 Apr 14, 2022

Avantol13 Apr 14, 2022

Avantol13 Apr 14, 2022

Avantol13 Apr 14, 2022

Avantol13 Apr 14, 2022

Avantol13 Jun 27, 2022

Avantol13 Jun 27, 2022

Avantol13 Jun 27, 2022

Avantol13 Jun 27, 2022

Avantol13 Jun 27, 2022

Avantol13 Jul 5, 2022

Avantol13 Jul 5, 2022

BinamB Jul 5, 2022

Avantol13 Jul 12, 2022

Avantol13 Jul 12, 2022

BinamB Jul 12, 2022

Avantol13 Jul 12, 2022

Avantol13 Jul 12, 2022

BinamB Jul 12, 2022

Avantol13 Jul 12, 2022

Avantol13 Jul 12, 2022

		@@ -633,7 +711,7 @@ def mock_merge(dbgap_servers, sess):

		# this function will be called once for each sftp server
		# the test config file has 2 dbgap sftp servers

	# the test config file has 2 dbgap sftp servers
	# the test config file has 3 dbgap sftp servers

(PXP:9493):(feat)Non-dbgap whitelist in usersync #1020

(PXP:9493):(feat)Non-dbgap whitelist in usersync #1020

Conversation

BinamB commented Apr 14, 2022

New Features

github-actions bot commented Apr 14, 2022

coveralls commented Apr 14, 2022 • edited

Pull Request Test Coverage Report for Build 12645

💛 - Coveralls

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

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

coveralls commented Apr 14, 2022 •

edited