SEAB-6831: Fix aborted DOI alias creation during bot push #6057
Conversation
|
We could also cherrypick this to develop and deploy to qa, so that we can user test bot pushes and results there, before releasing/deploying to staging. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## hotfix/1.16.4 #6057 +/- ##
================================================
Coverage 74.52% 74.52%
Complexity 5506 5506
================================================
Files 381 381
Lines 19800 19801 +1
Branches 2044 2044
================================================
+ Hits 14756 14757 +1
Misses 4065 4065
Partials 979 979
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| * @return the Workflow Version | ||
| */ | ||
| public static WorkflowVersion addWorkflowVersionAliasesAndCheck(AuthenticatedResourceInterface authenticatedResourceInterface, WorkflowDAO workflowDAO, | ||
| public static WorkflowVersion addWorkflowVersionAliases(AuthenticatedResourceInterface authenticatedResourceInterface, WorkflowDAO workflowDAO, |
There was a problem hiding this comment.
authenticatedResourceInterface and workflowDAO parameters are now unused, you can remove them.
Can't comment directly there in the PR, but JavaDoc could be updated -- the overview (no longer only works for the owner of the entry), the user description, and remove the unused params if you remove them from code.
There was a problem hiding this comment.
authenticatedResourceInterfaceandworkflowDAOparameters are now unused, you can remove them.Can't comment directly there in the PR, but JavaDoc could be updated -- the overview (no longer only works for the owner of the entry), the user description, and remove the unused params if you remove them from code.
Sure, I updated the javadoc comment.
I didn't remove the unused params on purpose, because the changes ripple a ways up through the call stack, and I didn't want the diffs to cloud this PR, given that it's a hotfix. I will write a ticket to clean up, which should probably happen on develop after the hotfix branch merges, could actually be best done as a slightly-bigger refactor, some of the params could change type...
|
Description
This PR removes the auth check from
AliasHelper.addWorkflowVersionAliasesAndCheckthat was causing the problem described in https://ucsc-cgl.atlassian.net/browse/SEAB-6831Basically, the auto DOI generation code was calling
addWorkflowVersionAliasesAndCheck, which was throwing when a bot pushed, because no Dockstore user was present, so the "can the user write to the workflow" check failed. This exception was caught, meaning that the DOI was created, but since the alias generation code failed, no alias was saved to the db. So, the Dockstore alias link in the DOI didn't work when clicked upon.When removing an auth check, the big danger is that you accidentally open a security hole. So, I tried to analyze the execution paths, and, on the "manual DOI generation" path, I've added a necessary equivalent check, further "upstream" (earlier). Everywhere else, I think, either the user has already been confirmed to be able to write to the workflow, or the webservice is initiating an automatic generation. Please do your own analysis to make sure.
Given the hotfix target and the sensitivity of the code, this is a minimal bug fix, and as such, I didn't clean up now-unused parameters, try to restructure for clarity, or anything like that.
Given our current testing infra, user testing might be best, ala:
Review Instructions
See steps above.
Issue
https://ucsc-cgl.atlassian.net/browse/SEAB-6831
Security and Privacy
We are mucking with auth checks, please scrutinize to make sure we're still checking everywhere we should.
e.g. Does this change...
Please make sure that you've checked the following before submitting your pull request. Thanks!
mvn clean install@RolesAllowedannotation