fix(add-bearer-token-header) add authorization header for Bearer token #88

plooploops · 2021-04-24T05:59:51Z

Add retry logic for refreshing tokens for Gen3Auth.

New Features

Breaking Changes

Bug Fixes

Include retry logic for caching refreshed access tokens.

Improvements

Added notes for Python set up and manual testing in devTest.md
Additional unit tests for Gen3Auth / Gen3Submission

Dependency updates

Include using backoff lib

Deployment changes

Avantol13

Hello! Thanks for the contribution, excited to work with you on this.

There should have been a pull request template to fill out, do you know if that showed up when you initially created the PR? Either way, can you change your description and follow this template:

<!--
Make sure to follow the DEV guidelines (https://gen3.org/resources/developer/dev-introduction/) before asking for review.

- Describe what this pull request does.
- Add short descriptive bullet points for each section if relevant. Keep in mind that they will be parsed automatically to generate official release notes.
- Test manually.
- Maintain or increase the test coverage (if relevant).
- Update the documentation, or justify if not needed.

-->

### New Features


### Breaking Changes


### Bug Fixes


### Improvements


### Dependency updates


### Deployment changes
<!-- This section should only contain important things devops should know when updating service versions. -->

Avantol13 · 2021-04-26T19:48:59Z

README.md

@@ -34,3 +34,59 @@ To get the latest released version of the SDK:
 This SDK exposes a Command Line Interface (CLI). You can now import functions from `gen3` into your own Python scripts or you can use the command line interface:

 `gen3 --help`
+
+
+## Local Development


There's a separate development page that covers most of this. https://github.com/uc-cdis/gen3sdk-python/blob/master/docs/howto/devTest.md if there are pieces missing, feel free to modify that page instead

Sounds good, moved these over.

Avantol13 · 2021-04-26T19:56:26Z

gen3/submission.py

@@ -198,8 +199,22 @@ def submit_record(self, program, project, json):

        """
        api_url = "{}/api/v0/submission/{}/{}".format(self._endpoint, program, project)
-        output = requests.put(api_url, auth=self._auth_provider, json=json)


the auth provider should already handle the population of the authorization header behind the scenes, are you sure this wasn't working before you added this code? https://github.com/uc-cdis/gen3sdk-python/blob/feat/merge-refactor/gen3/auth.py#L214-L227 If there's some bug with this logic, we should find a way to correct the Auth class, not handle the headers here.

In using the existing version, it looks like we get the following from using a submit_record call:

File "/sheepdog/sheepdog/blueprint/routes/views/__init__.py", line 59, in get_programs auth.validate_request(aud={"openid"}, purpose=None) File "/usr/local/lib/python3.6/site-packages/authutils/token/validate.py", line 236, in validate_request raise JWTError("no authorization header provided") authutils.errors.JWTError: [401] - Authentication Error: no authorization header provided

However, in trying to replicate the 401's, the error doesn't occur with Sheepdog every time.

In looking into the Auth class, it looks like there's a couple of spots that may be interesting:

refreshing a token

getting an access token

Made an update to include retry logic around writing to files for refresh_token.

Looks like the calls should include the header; this could also be the case of using an older version of the lib.

Avantol13 · 2021-04-26T19:59:56Z

gen3/tools/indexing/index_manifest.py

@@ -514,11 +517,12 @@ def index_object_manifest(
    commons_url = commons_url.strip("/")
    # if running locally, indexd is deployed by itself without a location relative
    # to the commons
-    if "http://localhost" in commons_url:
-        service_location = ""
+    if "http://localhost" in commons_url and include_service_location:


can you describe the behavior you're expecting here and/or why the change (was this not working with a local instance)? There's similar logic elsewhere that also might be useful reference: https://github.com/uc-cdis/gen3sdk-python/blob/feat/merge-refactor/gen3/metadata.py#L55-L62

This was breaking in the unit tests under test_manifests.py

For example, if include_service_location is set to True, the unit test would include the following extra /index in the test:

Detail 404 Client Error: no record found for url: http://localhost:8001/index/index/

hmm, so I'd rather keep this similar to the link I supplied where the variable is not a boolean, but the actual string path of the service relative to the domain. These won't change often, but it's probably better to leave that option open instead of hard-coding /index without a way to change it

Updated logic.

tests/test_submission.py

Avantol13 · 2021-04-29T17:11:37Z

docs/howto/devTest.md

+You can set up a Python development environment with a virtual environment:
+
+```bash
+python3 -m venv py38


while everything should work for python ^3.6, I'd prefer to generalize specific versions to just 3 where possible. So python3 -m venv py3 instead of 38

Updated based on comments.

Avantol13 · 2021-04-29T18:27:10Z

gen3/tools/indexing/index_manifest.py

@@ -514,11 +517,12 @@ def index_object_manifest(
    commons_url = commons_url.strip("/")
    # if running locally, indexd is deployed by itself without a location relative
    # to the commons
-    if "http://localhost" in commons_url:
-        service_location = ""
+    if "http://localhost" in commons_url and include_service_location:


hmm, so I'd rather keep this similar to the link I supplied where the variable is not a boolean, but the actual string path of the service relative to the domain. These won't change often, but it's probably better to leave that option open instead of hard-coding /index without a way to change it

Avantol13 · 2021-04-29T18:29:15Z

tests/test_auth.py

@@ -38,6 +38,85 @@ def test_token_cache():
    assert cache_file == expected


+def test_refresh_access_token(mock_gen3_auth):
+    with patch("gen3.auth.get_access_token_with_key") as mock_access_token:


We're not super consistent about this, but I'd prefer that each test have a docstring with a quick sentence about what it's testing. """Make sure that access token ends up in header when refresh is called""" or something like that. same for other tests

Included in the added tests.

Avantol13 · 2021-04-29T18:39:00Z

gen3/auth.py

@@ -267,18 +267,37 @@ def refresh_access_token(self):
        cache_file = token_cache_file(
            self._refresh_token and self._refresh_token["api_key"] or self._wts_idp
        )
+
+        sleep_amount_seconds = 1
+        retry_total = 5


we actually already have a backoff library with configurable settings I'd rather reuse than implement custom retry logic (example: https://github.com/uc-cdis/gen3sdk-python/blob/feat/merge-refactor/gen3/jobs.py#L110) using https://github.com/litl/backoff. You'll have to change the logic a bit because it retries on an Exception, so instead of _write_to_file being a boolean you could just raise exceptions and the backoff library/cfg will retry the whole function if you decorate it

Included using the backoff library.

Avantol13 · 2021-05-03T19:46:00Z

gen3/tools/indexing/index_manifest.py

@@ -520,6 +521,9 @@ def index_object_manifest(
    if not commons_url.endswith(service_location):
        commons_url += "/" + service_location

+    print(commons_url)


Suggested change

print(commons_url)

Avantol13 · 2021-05-04T15:24:24Z

docs/howto/devTest.md

 ```

 There are various ways to select a subset of python unit-tests - see: https://stackoverflow.com/questions/36456920/is-there-a-way-to-specify-which-pytest-tests-to-run-from-a-file

+### Manual Testing
+
+You can also set up credentials to submit metadata to your data commons.  This assumes that you can get API access by downloading your [credentials.json](https://gen3.org/resources/user/using-api/#credentials-to-send-api-requests).


metadata is sometimes a loaded term, I would prefer to explicitly say something like "submit data to the graph in your data commons".

Sounds good, updated!

xritter1 · 2021-05-05T15:05:53Z

Hello @plooploops, could you take a look at the output of the Wool test before we can merge this PR? Thanks!

`Black output:
--- gen3/auth.py
+++ blackened
@@ -255,11 +255,11 @@
_response.request = newreq

     return _response

 def refresh_access_token(self):

```
   """ Get a new access token """
```

   """Get a new access token"""
   if self._use_wts:
       self._access_token = get_access_token_from_wts(
           self._wts_namespace, self._wts_idp
       )
   else:

@@ -296,11 +296,11 @@
logging.warning("failed to write token cache file: " + cache_file)
logging.warning(str(e))
raise e

 def get_access_token(self):

   """ Get the access token - auto refresh if within 5 minutes of expiration """

   """Get the access token - auto refresh if within 5 minutes of expiration"""
   if not self._access_token:
       cache_file = token_cache_file(
           self._refresh_token and self._refresh_token["api_key"] or self._wts_idp
       )
       if os.path.isfile(cache_file):

{'message': 'Resource not accessible by integration', 'documentation_url': 'https://docs.github.com/rest/reference/issues#create-an-issue-comment'}`

for submission

plooploops · 2021-05-06T16:35:07Z

Thanks @Avantol13 and @xritter1!

Avantol13 requested changes Apr 26, 2021

View reviewed changes

plooploops force-pushed the fix/add-bearer-token-header branch 6 times, most recently from 56360cd to 98080f3 Compare April 28, 2021 22:55

plooploops requested a review from Avantol13 April 28, 2021 23:58

Avantol13 requested changes Apr 29, 2021

View reviewed changes

plooploops force-pushed the fix/add-bearer-token-header branch 4 times, most recently from 099130d to 8834b97 Compare April 30, 2021 02:01

Avantol13 reviewed May 3, 2021

View reviewed changes

plooploops force-pushed the fix/add-bearer-token-header branch from 8834b97 to c5c4328 Compare May 4, 2021 00:17

Avantol13 reviewed May 4, 2021

View reviewed changes

plooploops force-pushed the fix/add-bearer-token-header branch from c5c4328 to dd55435 Compare May 4, 2021 20:54

Avantol13 approved these changes May 5, 2021

View reviewed changes

fix(add-bearer-token-header) add authorization header for Bearer token

375304e

for submission

plooploops force-pushed the fix/add-bearer-token-header branch from dd55435 to 375304e Compare May 5, 2021 21:44

xritter1 merged commit 9bbfa7b into uc-cdis:master May 6, 2021

paulineribeyre mentioned this pull request Sep 13, 2024

Print errors in submit_record #234

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(add-bearer-token-header) add authorization header for Bearer token #88

fix(add-bearer-token-header) add authorization header for Bearer token #88

plooploops commented Apr 24, 2021 •

edited

Loading

Avantol13 left a comment

Avantol13 Apr 26, 2021

plooploops Apr 27, 2021 •

edited

Loading

Avantol13 Apr 26, 2021

plooploops Apr 27, 2021

plooploops Apr 28, 2021

Avantol13 Apr 26, 2021

plooploops Apr 27, 2021 •

edited

Loading

Avantol13 Apr 29, 2021

plooploops Apr 30, 2021

Avantol13 Apr 29, 2021

plooploops Apr 30, 2021

Avantol13 Apr 29, 2021

Avantol13 Apr 29, 2021

plooploops Apr 30, 2021

Avantol13 Apr 29, 2021

plooploops Apr 30, 2021

Avantol13 May 3, 2021

plooploops May 4, 2021

Avantol13 May 4, 2021

plooploops May 4, 2021

xritter1 commented May 5, 2021

plooploops commented May 6, 2021

fix(add-bearer-token-header) add authorization header for Bearer token #88

fix(add-bearer-token-header) add authorization header for Bearer token #88

Conversation

plooploops commented Apr 24, 2021 • edited Loading

New Features

Breaking Changes

Bug Fixes

Improvements

Dependency updates

Deployment changes

Avantol13 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plooploops Apr 27, 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

plooploops Apr 27, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xritter1 commented May 5, 2021

plooploops commented May 6, 2021

plooploops commented Apr 24, 2021 •

edited

Loading

plooploops Apr 27, 2021 •

edited

Loading

plooploops Apr 27, 2021 •

edited

Loading