Fix: Final URL to managed access manifests is broken (#3922, #3328) #3931

nadove-ucsc · 2022-03-08T23:04:00Z

Connected issue: #3922

Checklist

Author

PR title references the connected issue
PR title matches¹ title of connected issue _{or comment in PR explains why they're different}
Title of at least one commit references connected issue
PR is connected to issue via Zenhub
PR description links to connected issue

¹ when the issue title describes a problem, the PR title is Fix: followed by the issue title

Author (reindex)

Added r tag to commit title _{or this PR does not require reindexing}
Added reindex label to PR _{or this PR does not require reindexing}

Author (chains)

This PR is blocked by previous PR in the chain _{or this PR is not chained to another PR}
Added chain label to the blocking PR _{or this PR is not chained to another PR}

Author (upgrading)

Documented upgrading of deployments in UPGRADING.rst _{or this PR does not require upgrading}
Added u tag to commit title _{or this PR does not require upgrading}
Added upgrade label to PR _{or this PR does not require upgrading}
Added announcement to PR description _{or this PR does not require announcement}
Added checklist items for additional operator tasks _{or this PR does not require additional tasks}

Author (hotfixes)

Added h tag to commit title and PR targets prod _{or this PR does not include a temporary hotfix}
Added H tag to commit title _{or this PR does not include a permanent hotfix}
Added hotfix label to PR _{or this PR does not include a hotfix}
Reverted the temporary hotfix connected to the issue _{or there is no temporary hotfix for the connected issue on the prod branch}

Author (requirements, before every review)

Ran make requirements_update _{or this PR does not touch requirements*.txt, common.mk, Makefile and Dockerfile}
Added R tag to commit title _{or this PR does not touch requirements*.txt}
Added reqs label to PR _{or this PR does not touch requirements*.txt}

Author (before every review)

make integration_test passes in personal deployment _{or this PR does not touch functionality that could break the IT}
Rebased branch on develop, squashed old fixups

Primary reviewer (after approval)

Labeled connected issue as demo or no demo
Commented on connected issue about demo expectations _{or labelled connected issue as no demo}
Decided if PR can be labeled no sandbox
PR title is appropriate as title of merge commit
Moved ticket to Approved column
Assigned PR to an operator

Operator (before pushing merge the commit)

Operator (after pushing the merge commit)

Made announcement requested by author _{or PR description does not contain an announcement}
Shortened the PR chain _{or this PR is not the base of another PR}
Verified that N reviews labelling is accurate _{or this PR is authored by lead}
Pushed merge commit to Gitlab _{or merge commit can be pushed later, with another PR}
Deleted PR branch from Github and Gitlab
Build passes on Gitlab
Moved connected issue to prod or Merged prod _{or this PR does not represent a promotion}

Operator (reindex)

Started reindex in target deployment _{or this PR does not require reindexing}
Checked for and triaged indexing failures _{or this PR does not require reindexing}
Emptied fail queues in target deployment _{or this PR does not require reindexing}
Filed backport PR _{or this PR does not represent a hotfix}

Operator

Unassigned PR

Shorthand for review comments

L line is too long
W line wrapping is wrong
Q bad quotes
F other formatting problem

codecov · 2022-03-08T23:24:01Z

Codecov Report

Merging #3931 (93762d2) into develop (d56e6ed) will increase coverage by 0.01%.
The diff coverage is 75.00%.

@@             Coverage Diff             @@
##           develop    #3931      +/-   ##
===========================================
+ Coverage    81.97%   81.99%   +0.01%     
===========================================
  Files          124      124              
  Lines        15150    15149       -1     
===========================================
+ Hits         12419    12421       +2     
+ Misses        2731     2728       -3

Impacted Files	Coverage Δ
src/azul/service/manifest_service.py	`94.64% <ø> (+0.54%)`	⬆️
src/azul/service/source_controller.py	`90.90% <ø> (ø)`
src/azul/service/manifest_controller.py	`96.47% <75.00%> (-1.10%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d56e6ed...93762d2. Read the comment docs.

src/azul/service/manifest_service.py

achave11-ucsc

Another reason for adding a second commit is that the present commit doesn't resolve (#3328) AFAICT, so I am unsure it is accurate to reference it in this form in the commit or the PR tittle.

achave11-ucsc

LGTM!

hannes-ucsc

I blocked the connected ticket on #2956 for which there is another PR in the works (#3925). Please hold this warm until all blockers are in dev.

coveralls · 2022-03-17T19:57:17Z

Coverage increased (+0.02%) to 82.2% when pulling 93762d2 on issues/noah-aviel-dove/3922-manifest-final-link-broken into d56e6ed on develop.

hannes-ucsc

I think this leaves the source code in a bit of a confusing state. With this surgical approach, the service still goes through all the motions of actually listing the accessible sources in the controller and populating the filter object with the result even though the wrong credentials are used to request the listing.

Maybe this makes it more clear

Index: src/azul/service/source_controller.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/azul/service/source_controller.py b/src/azul/service/source_controller.py
--- a/src/azul/service/source_controller.py	(revision 7e6b6143b13fc42cd9ed60dd76d70fd52ad68950)
+++ b/src/azul/service/source_controller.py	(date 1648084946401)
@@ -54,7 +54,7 @@
 
     def get_filters(self,
                     catalog: CatalogName,
-                    authentication: Authentication,
+                    authentication: Optional[Authentication],
                     filters: Optional[str] = None
                     ) -> MutableFilters:
         return MutableFilters(explicit=self._parse_filters(filters),
Index: src/azul/service/manifest_controller.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/azul/service/manifest_controller.py b/src/azul/service/manifest_controller.py
--- a/src/azul/service/manifest_controller.py	(revision 7e6b6143b13fc42cd9ed60dd76d70fd52ad68950)
+++ b/src/azul/service/manifest_controller.py	(date 1648084888573)
@@ -107,10 +107,10 @@
         token = query_params.get('token')
         if token is None:
             format_ = ManifestFormat(query_params['format'])
-            filters = self.get_filters(catalog, authentication, query_params.get('filters'))
             try:
                 object_key = query_params['objectKey']
             except KeyError:
+                filters = self.get_filters(catalog, authentication, query_params.get('filters'))
                 object_key, manifest = self.service.get_cached_manifest(format_=format_,
                                                                         catalog=catalog,
                                                                         filters=filters,
@@ -128,6 +128,10 @@
                     }
                     token = self.async_service.start_generation(state)
             else:
+                # FIXME: Add support for long-lived API tokens
+                #        https://github.com/DataBiosphere/azul/issues/3328
+                assert authentication is None, authentication
+                filters = self.get_filters(catalog, authentication, query_params.get('filters'))
                 try:
                     manifest = self.service.get_cached_manifest_with_object_key(
                         format_=format_,

But check the Optional[Authentication] change and look for more places where that is wrong.

Squash the FIXME commit and the main commit. The Optional[Authentication] changes should be a separate commit.

hannes-ucsc · 2022-04-04T21:49:30Z

I'm assuming the intention is for me to be assigned.

hannes-ucsc

Index: src/azul/service/manifest_service.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/azul/service/manifest_service.py b/src/azul/service/manifest_service.py
--- a/src/azul/service/manifest_service.py	(revision 4885cf2f20167ff2039f547f8b6a9221861331fe)
+++ b/src/azul/service/manifest_service.py	(date 1649120207370)
@@ -451,12 +451,15 @@
                                                  catalog,
                                                  filters,
                                                  authentication)
-        current_source_key = generator.compute_source_key()
-        manifest_key, source_key, extension = object_key.rsplit('/', 1)[-1].split('.')
-        # FIXME: Add support for long-lived API tokens
-        #        https://github.com/DataBiosphere/azul/issues/3328
-        if False and source_key != current_source_key:
-            raise CachedManifestSourcesChanged
+        # FIXME: Add support for long-lived API tokens
+        #        https://github.com/DataBiosphere/azul/issues/3328
+        if False:
+            current_source_key = generator.compute_source_key()
+            # FIXME: Consolidate parsing of manifest object key
+            #        REVIEW: insert link to ticket
+            manifest_key, source_key, extension = object_key.rsplit('/', 1)[-1].split('.')
+            if source_key != current_source_key:
+                raise CachedManifestSourcesChanged
         file_name = self._get_cached_manifest_file_name(generator, object_key)
         if file_name is None:
             raise CachedManifestNotFound
@@ -918,6 +921,8 @@
             timestamp = datetime.now().strftime("%Y-%m-%d %H.%M")
             file_name = f'{file_name_prefix} {timestamp}.{self.file_name_extension}'
         else:
+            # FIXME: Consolidate parsing of manifest object key
+            #        REVIEW: insert link to ticket
             file_name = 'hca-manifest-' + object_key.rsplit('/', )[-1]
         return file_name
 
Index: src/azul/service/manifest_controller.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/azul/service/manifest_controller.py b/src/azul/service/manifest_controller.py
--- a/src/azul/service/manifest_controller.py	(revision 4885cf2f20167ff2039f547f8b6a9221861331fe)
+++ b/src/azul/service/manifest_controller.py	(date 1649120524595)
@@ -133,8 +133,7 @@
                 if authentication is None:
                     filters = self.get_filters(catalog, authentication, query_params.get('filters'))
                 else:
-                    raise BadRequestError(f'Authentication is not accepted when using the `objectKey` parameter. '
-                                          f'Recieved authentication {authentication.as_http_header()}')
+                    raise BadRequestError("Must omit authentication when passing 'objectKey'")
                 try:
                     manifest = self.service.get_cached_manifest_with_object_key(
                         format_=format_,

hannes-ucsc · 2022-04-05T00:51:53Z

src/azul/service/manifest_controller.py

+                    filters = self.get_filters(catalog, authentication, query_params.get('filters'))
+                else:
+                    raise BadRequestError(f'Authentication is not accepted when using the `objectKey` parameter. '
+                                          f'Recieved authentication {authentication.as_http_header()}')


Probably shouldn't include the token in the response body.

hannes-ucsc

~~I think the two commits~~

github-actions bot added the orange [process] Done by the Azul team label Mar 8, 2022

nadove-ucsc requested a review from achave11-ucsc March 8, 2022 23:39

nadove-ucsc assigned achave11-ucsc Mar 8, 2022

achave11-ucsc reviewed Mar 9, 2022

View reviewed changes

src/azul/service/manifest_service.py Show resolved Hide resolved

achave11-ucsc reviewed Mar 9, 2022

View reviewed changes

achave11-ucsc removed their assignment Mar 9, 2022

nadove-ucsc force-pushed the issues/noah-aviel-dove/3921-manifest-cmd-lacks-fail branch from 3f1d66d to 479785a Compare March 10, 2022 19:13

nadove-ucsc force-pushed the issues/noah-aviel-dove/3922-manifest-final-link-broken branch from 54142bb to e0a29cf Compare March 11, 2022 01:29

nadove-ucsc requested a review from achave11-ucsc March 11, 2022 02:09

nadove-ucsc assigned achave11-ucsc Mar 11, 2022

achave11-ucsc approved these changes Mar 11, 2022

View reviewed changes

achave11-ucsc removed their assignment Mar 11, 2022

nadove-ucsc requested a review from hannes-ucsc March 11, 2022 20:28

nadove-ucsc assigned hannes-ucsc Mar 11, 2022

hannes-ucsc reviewed Mar 15, 2022

View reviewed changes

hannes-ucsc added the hold warm [process] PR can't land just yet but needs to be rebased daily label Mar 15, 2022

hannes-ucsc removed their assignment Mar 15, 2022

achave11-ucsc force-pushed the issues/noah-aviel-dove/3921-manifest-cmd-lacks-fail branch from 479785a to 5444a57 Compare March 16, 2022 22:10

achave11-ucsc changed the base branch from issues/noah-aviel-dove/3921-manifest-cmd-lacks-fail to develop March 17, 2022 00:15

nadove-ucsc force-pushed the issues/noah-aviel-dove/3922-manifest-final-link-broken branch from e0a29cf to 6521f09 Compare March 17, 2022 19:32

nadove-ucsc force-pushed the issues/noah-aviel-dove/3922-manifest-final-link-broken branch from 6521f09 to 7e6b614 Compare March 22, 2022 00:18

nadove-ucsc requested a review from hannes-ucsc March 22, 2022 21:46

nadove-ucsc assigned hannes-ucsc Mar 22, 2022

hannes-ucsc requested changes Mar 24, 2022

View reviewed changes

hannes-ucsc added the 1 review [process] Lead requested changes once label Mar 24, 2022

hannes-ucsc removed their assignment Mar 24, 2022

hannes-ucsc removed the hold warm [process] PR can't land just yet but needs to be rebased daily label Mar 24, 2022

nadove-ucsc force-pushed the issues/noah-aviel-dove/3922-manifest-final-link-broken branch from 7e6b614 to 2ad65f9 Compare March 28, 2022 22:09

hannes-ucsc force-pushed the develop branch 2 times, most recently from 01fb85d to 4aacfd5 Compare March 29, 2022 01:27

nadove-ucsc force-pushed the issues/noah-aviel-dove/3922-manifest-final-link-broken branch 2 times, most recently from 976e358 to 0ec35e3 Compare April 4, 2022 19:25

nadove-ucsc requested a review from hannes-ucsc April 4, 2022 20:06

nadove-ucsc assigned nadove-ucsc and unassigned nadove-ucsc Apr 4, 2022

hannes-ucsc self-assigned this Apr 4, 2022

hannes-ucsc requested changes Apr 5, 2022

View reviewed changes

hannes-ucsc removed their assignment Apr 5, 2022

hannes-ucsc added 1 review [process] Lead requested changes once and removed 1 review [process] Lead requested changes once labels Apr 5, 2022

nadove-ucsc force-pushed the issues/noah-aviel-dove/3922-manifest-final-link-broken branch from 4885cf2 to 7705de0 Compare April 5, 2022 06:08

nadove-ucsc requested a review from hannes-ucsc April 5, 2022 06:40

nadove-ucsc assigned hannes-ucsc Apr 5, 2022

hannes-ucsc approved these changes Apr 6, 2022

View reviewed changes

hannes-ucsc assigned amarjandu and unassigned hannes-ucsc Apr 6, 2022

nadove-ucsc added 3 commits April 6, 2022 11:17

Disallow authentication on manifest request with object key (#3328)

871cb2d

Disable cached manifest access check (#3922, #3328)

5651035

Add FIXME (#4050)

93762d2

amarjandu force-pushed the issues/noah-aviel-dove/3922-manifest-final-link-broken branch from 9ef5a72 to 93762d2 Compare April 6, 2022 18:17

amarjandu added the sandbox [process] Resolution is being verified in sandbox deployment label Apr 6, 2022

amarjandu merged commit 900aa09 into develop Apr 6, 2022

amarjandu deleted the issues/noah-aviel-dove/3922-manifest-final-link-broken branch April 6, 2022 19:28

amarjandu removed their assignment Apr 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Final URL to managed access manifests is broken (#3922, #3328) #3931

Fix: Final URL to managed access manifests is broken (#3922, #3328) #3931

nadove-ucsc commented Mar 8, 2022 •

edited by amarjandu

Loading

codecov bot commented Mar 8, 2022 •

edited

Loading

achave11-ucsc left a comment

achave11-ucsc left a comment

hannes-ucsc left a comment

coveralls commented Mar 17, 2022 •

edited

Loading

hannes-ucsc left a comment

hannes-ucsc commented Apr 4, 2022

hannes-ucsc left a comment

hannes-ucsc Apr 5, 2022

hannes-ucsc left a comment •

edited

Loading

Fix: Final URL to managed access manifests is broken (#3922, #3328) #3931

Fix: Final URL to managed access manifests is broken (#3922, #3328) #3931

Conversation

nadove-ucsc commented Mar 8, 2022 • edited by amarjandu Loading

Checklist

Author

Author (reindex)

Author (chains)

Author (upgrading)

Author (hotfixes)

Author (requirements, before every review)

Author (before every review)

Primary reviewer (after approval)

Operator (before pushing merge the commit)

Operator (after pushing the merge commit)

Operator (reindex)

Operator

Shorthand for review comments

codecov bot commented Mar 8, 2022 • edited Loading

Codecov Report

achave11-ucsc left a comment

Choose a reason for hiding this comment

achave11-ucsc left a comment

Choose a reason for hiding this comment

hannes-ucsc left a comment

Choose a reason for hiding this comment

coveralls commented Mar 17, 2022 • edited Loading

hannes-ucsc left a comment

Choose a reason for hiding this comment

hannes-ucsc commented Apr 4, 2022

hannes-ucsc left a comment

Choose a reason for hiding this comment

hannes-ucsc Apr 5, 2022

Choose a reason for hiding this comment

hannes-ucsc left a comment • edited Loading

Choose a reason for hiding this comment

nadove-ucsc commented Mar 8, 2022 •

edited by amarjandu

Loading

codecov bot commented Mar 8, 2022 •

edited

Loading

coveralls commented Mar 17, 2022 •

edited

Loading

hannes-ucsc left a comment •

edited

Loading