Allow organization admins to view pending and rejected invitations #5243
Conversation
…ing reject invites - Includes migrations from the 'accepted' column to the 'status' column
Codecov ReportBase: 73.23% // Head: 73.27% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #5243 +/- ##
=============================================
+ Coverage 73.23% 73.27% +0.04%
- Complexity 4375 4394 +19
=============================================
Files 292 292
Lines 16701 16717 +16
Branches 1834 1839 +5
=============================================
+ Hits 12231 12250 +19
- Misses 3596 3599 +3
+ Partials 874 868 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| UPDATE organization_user SET status='ACCEPTED' where accepted=true; | ||
| UPDATE organization_user SET status='PENDING' where accepted=false; | ||
| </sql> | ||
| <dropColumn columnName="accepted" tableName="organization_user"/> |
There was a problem hiding this comment.
Once the migration is done, this will break old web service code that is still running. We often do have downtime with major releases, but we could try as an experiment to not have the downtime. So either
- Can you please note this in the 1.14 deploy notes -- we always hypothesize if the web service will be broken by a release, we should start tracking it when we know it will.
- Optionally, you could do something with a trigger, that keeps the new column in sync with the old column. Then in 1.15 we drop this column. Probably overkill, but OTOH if we want to eventually get to fully automated deploys, we may need to start doing this.
There was a problem hiding this comment.
I'm ok with either but second that this should either be clearly noted in the deploy notes or deprecated in the second case.
There was a problem hiding this comment.
Ended up going with number 2, maybe overkill, but it will be interesting to experiment with this
dockstore-integration-testing/src/test/java/io/dockstore/common/CommonTestUtilities.java
Show resolved
Hide resolved
| .collect(Collectors.toSet()); | ||
|
|
||
| return acceptedUsers; | ||
| return getOrganizationByIdOptionalAuth(user, id).getUsers(); |
There was a problem hiding this comment.
I didn't try running this, so I might be wrong, but won't this allow all Dockstore users to view all invited/rejected users of an org?
If so, I don't think that should be allowed; only org admins should have access to invites and rejections.
| UPDATE organization_user SET status='ACCEPTED' where accepted=true; | ||
| UPDATE organization_user SET status='PENDING' where accepted=false; | ||
| </sql> | ||
| <dropColumn columnName="accepted" tableName="organization_user"/> |
There was a problem hiding this comment.
I'm ok with either but second that this should either be clearly noted in the deploy notes or deprecated in the second case.
dockstore-integration-testing/src/test/java/io/dockstore/common/CommonTestUtilities.java
Outdated
Show resolved
Hide resolved
dockstore-webservice/src/main/java/io/dockstore/webservice/resources/OrganizationResource.java
Show resolved
Hide resolved
- Bring back the accepted column and deprecate it - Create a DB trigger that syncs the accepted and status columns - Fix permissions so only accepted admins/maintainers can perform actions - Only return all organization members if the user is an org admin
|
Re-requesting reviews for these new changes:
|
| * Test that invited users who have not accepted their invitations are powerless. Tests all roles: Member, Maintainer, Admin | ||
| */ | ||
| @Test | ||
| public void testPendingAndRejectedUsersArePowerless() { |
There was a problem hiding this comment.
very nice for catching this and locking it down with a test
There was a problem hiding this comment.
This is great! Thanks for figuring out this path. We can use it as a template for future migrations.
One question, after the migration has been run, and you bring up a new instance of the old web service, will that work? I know existing old running code will work, but would the startup schema check pass or fail?
If it fails, I don't know that there's a lot we can do about it. Your code solves the problem of a no-downtime update, but the next thing to potentially plan for is a rollback (something is so wrong with the new version, that we need to temporarily deploy the old version). We've never needed to do this, and hopefully never will, so this is just a question, no need to change anything, and we already have this issue -- just trying to think ahead a little.
Also, if you agree, could you create a followup ticket with a 1.15 milestone to delete the accepted column/field? I suppose we could live with it forever and it would work, but might as well clean it up.
| // and if the user is neither an admin nor curator, then throw an error | ||
| if ((orgUser == null || orgUser.getRole() != OrganizationUser.Role.ADMIN) && (!user.isCurator() && !user.getIsAdmin())) { | ||
| // If the user is not an admin or maintainer of the organization and if the user is neither an admin nor curator, then throw an error | ||
| if (!isUserAdminOrMaintainer && (!user.isCurator() && !user.getIsAdmin())) { |
There was a problem hiding this comment.
Should maintainers be able to delete their own org? I would argue that they shouldn't, if they're not allowed to update memberships.
This is pretty edge-casey, as it's unlikely that a deleted or rejected org will have any maintainers to begin with.
There was a problem hiding this comment.
Will change it bac 6DAF k so only the organization admin can delete the org
dockstore-webservice/src/main/java/io/dockstore/webservice/resources/OrganizationResource.java
Show resolved
Hide resolved
| <addNotNullConstraint tableName="organization_user" columnName="status"/> | ||
| <sql dbms="postgresql"> | ||
| <comment>Create a trigger that syncs the old accepted column with the new status column</comment> | ||
| CREATE OR REPLACE FUNCTION insert_organization_user_sync_status_func() |
There was a problem hiding this comment.
Yowsa, that was more complex than I thought it would be. I see you have a two-way sync as well. Nice!
FYI, had this same thought separately in our meeting. As long as we have https://github.com/dockstore/compose_setup/blob/develop/templates/web.yml.template#L115 existing webservices will continue running if currently running, but will kill themselves on start-up |
| @ApiModelProperty(value = "Has the user accepted their membership.", required = true) | ||
| private boolean accepted = false; | ||
| @Schema(description = "Has the user accepted their membership.", required = true, deprecated = true) | ||
| private Boolean accepted; |
There was a problem hiding this comment.
Belated thought after this came up in a different meeting with @denis-yuen -- do we need this at all?
I guess it depends on how the Hibernate schema validator works. Does it allow extra columns that aren't mapped? According to this unverified StackOverflow question it does. The user is complaining that it does, but that would work to our advantage.
If that's true, then I think you can just remove this from the Java code entirely. The 1.14 code will access and only know about the new column, the 1.13 code will access and only know about the old column, and the DB keeps them in sync behind the scenes. Untested theory.
There was a problem hiding this comment.
Interesting. The thought above about turning off the validator is if it didn't allow unmapped columns.
There was a problem hiding this comment.
I tested this by removing the accepted field and the webservice starts with no errors, but I believe this will fail the CircleCI build here because the migrations are not consistent with the JPA class
| */ | ||
| public static List<String> listMigrations(String... additionals) { | ||
| if (additionals.length > 0) { | ||
| return Stream.concat(COMMON_MIGRATIONS.stream(), Stream.of(additionals)).collect(Collectors.toList()); |
There was a problem hiding this comment.
Entirely out of curiosity, and no need to change this, but could this line do all the work by itself, no if necessary?
There was a problem hiding this comment.
Yeah you're right, the if isn't necessary
I tried testing this locally by running the 1.14 migrations then starting the develop webservice (aka an old webservice without 1.14 migrations) and the webservice started with no schema errors. Keep in mind that if we actually do rollback in AWS, the 1.13 migrations will run before the old webservice container starts since every task has its own migrations container.
Created #5256 |
|
Kudos, SonarCloud Quality Gate passed! |
|
My newest change removes the Pros:
Table summarizing the state of
|
* java 17 and compile issue with trs api * quoting issue CircleCI-Public/gcp-cli-orb#22 (comment) * cli version of dockstore/dockstore#4792 and dockstore/dockstore#5279 * Need cli equivalent of dockstore/dockstore#5243 * syntax from https://stackoverflow.com/questions/23190107/cannot-use-jacoco-jvm-args-and-surefire-jvm-args-together-in-maven
Description
The
OrganizationUserclass currently has theacceptedboolean, which tracks if the invite is accepted or pending, but it doesn't track if the invite was rejected. This PR adds thestatusfield toOrganizationUserso that we can track the status of an organization invite and removes theacceptedfield.There is a UI PR dockstore/dockstore-ui2#1648 that relies on this PR. There is also a compose_setup PR that adds 1.14.0 migrations dockstore/compose_setup#242.
Review Instructions
Follow the review instructions in the UI PR dockstore/dockstore-ui2#1648
Issue
#4226
DOCK-1781
Please make sure that you've checked the following before submitting your pull request. Thanks!
mvn clean install@RolesAllowedannotation