SEAB-6978: Correct "metrics robot" design #6104
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release/1.17.0 #6104 +/- ##
====================================================
- Coverage 74.23% 74.19% -0.05%
+ Complexity 5660 5659 -1
====================================================
Files 389 389
Lines 20324 20326 +2
Branches 2098 2099 +1
====================================================
- Hits 15087 15080 -7
- Misses 4236 4244 +8
- Partials 1001 1002 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -13,7 +13,7 @@ public class PrivilegeRequest { | |||
| private boolean admin; | |||
| private boolean curator; | |||
| private Partner platformPartner; | |||
There was a problem hiding this comment.
Haven't looked in detail at this object. Does it make sense to do validation or reject requests where the platformPartner doesn't match metricsRobotPartner?
Actually do we need both? Proceeding below.
There was a problem hiding this comment.
Link a follow-up ticket if you don't have an objection. Doesn't block, but feels a little weird that this can represent a request that doesn't make sense.
dockstore-webservice/src/main/java/io/dockstore/webservice/core/User.java
Show resolved
Hide resolved
...c/main/java/io/dockstore/webservice/resources/proposedGA4GH/ToolsApiExtendedServiceImpl.java
Show resolved
Hide resolved
|
FYI, this PR is to develop, should be to https://github.com/dockstore/dockstore/tree/release/1.17.0 |
…bot-design
| @@ -13,7 +13,7 @@ public class PrivilegeRequest { | |||
| private boolean admin; | |||
| private boolean curator; | |||
| private Partner platformPartner; | |||
There was a problem hiding this comment.
Link a follow-up ticket if you don't have an objection. Doesn't block, but feels a little weird that this can represent a request that doesn't make sense.
| </sql> | ||
| <sql dbms="postgresql"> | ||
| CREATE TRIGGER update_metrics_robot_to_non_metrics_robot BEFORE UPDATE ON enduser FOR EACH ROW WHEN (OLD.metricsrobotpartner IS NOT NULL AND NEW.metricsrobotpartner IS NULL) EXECUTE FUNCTION raise_metrics_robot_privileges_exception(); | ||
| ALTER TABLE enduser ADD CONSTRAINT metrics_robot_cannot_have_other_privileges CHECK (NOT (metricsrobotpartner IS NOT NULL AND (isadmin OR curator OR platformpartner IS NOT NULL))); |
There was a problem hiding this comment.
I'm a little confused. There's a build step called "check that JPA classes are consistent with migrations"
It checks that the JPA classes match the migrations because it would be weird if they didn't.
It seems to have run, but this change is only in migrations.
i.e. its a little weird this check doesn't need to be in jpa annotations OR import.sql
Please add a ticket to follow-up if the script is broken or something
There was a problem hiding this comment.
I took at look at both import.sql and our db/migrations, and it appears there's lots of check constraints, most of which aren't catalogued in import.sql. Indeed, this page suggests that vanilla liquibase doesn't generate diffs for check constraints (see the Liquibase Pro diff types): https://docs.liquibase.com/commands/inspection/diff.html
So, everything appears to be working as expected, given the above caveat.
|
Description
This PR adjusts the design of the "metrics robot" role. In the preceding PR #6089, we created a new metrics robots role. Alas, the new role was denoted by a boolean flag in
User, and was not tied to a particularPartner, thus allowing a metrics robot to submit metrics for any partner, which should not be permitted. Thanks to Kathy for finding the problem whilst emptying her review bucket!This PR prevents the problem by associating a metrics robot with a
Partner, to which it is solely authorized to submit metrics, in a manner similar to how the "platform partner" role works.Regarding compatibility with the existing naming conventions, should it be
metricsRobotPartner, ormetricsRobotPlatform? There's valid arguments for either, I think. If you don't like the former, let me know, and we can change to the latter.Sidebar:
Whenever I make a significant mistake whilst creating software, I try to go back and determine why it happened, so I can do better next time. In this case, the scenario matches a failure mode which we might call "full mid-stream redesign". Originally, I'd proposed a simple implementation that added a boolean
isRobottoUser, which would allow such a user to only access endpoints that were specifically annotated for the user's role. We decided to go with another design (the one wrapped up by this PR) instead.In such a situation, it can be useful to go completely back to square one to evaluate the new design and its implementation requirements, to avoid conflating with the assumptions/conclusions from the previous design. Also, if you're adapting the code that was implemented for the previous design, extra attention to correctness/consistency/completeness is merited. I shoulda done more of all of the above.
Review Instructions
Create a user on qa, make them a metrics robot via the setUserPrivileges endpoint, and confirm that you can use their token to submit metrics, but cannot access authorized non-metrics endpoints. Also, attempt to submit metrics for a partner that's not authorized, and confirm that you cannot.
Issue
https://ucsc-cgl.atlassian.net/browse/SEAB-6978
Security and Privacy
See above. Please scrutinize the mechanics of the new "metrics robot" role.
e.g. Does this change...
Please make sure that you've checked the following before submitting your pull request. Thanks!
mvn clean install@RolesAllowedannotation