BI-2255 - Experimental Collaborator Micronaut Changes by davedrp · Pull Request #388 · Breeding-Insight/bi-api

davedrp · 2024-08-13T15:50:46Z

Description

BI-2255 - Experimental Collaborator Micronaut Changes

Added EXPERIMENTAL_COLLABORATOR to the enum ProgramSecuredRole
Changed ProgramSecuredRoleGroup.ALL to ProgramSecuredRoleGroup.PROGRAM_SCOPED_ROLES.
Added ProgramSecuredRole.EXPERIMENTAL_COLLABORATOR to ProgramSecuredRoleGroup.PROGRAM_SCOPED_ROLES
Created the @ExperimentCollaboratorSecured annotation for endpoints with experimentId (or traitId)

Testing

see Acceptance Criteria in BI-2255

Checklist:

I have performed a self-review of my own code
I have tested my code and ensured it meets the acceptance criteria of the story
I have tested that my code works with both the brapi-java-server and BreedBase
I have create/modified unit tests to cover this change
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to documentation
I have run TAF: <please include a link to TAF run>

[BI-1692] - Change role names

BI-2257 - Experimental Collaborator DAO and Services for experimental_program_user_role

…aboratorSecured

mlm483

I haven't tested functionality yet, I wanted to get my change requests in ASAP.

See comments for changes, the most important of which is:

ProgramSecuredRoleGroup.PROGRAM_SCOPED_ROLES currently includes ProgramSecuredRole.EXPERIMENTAL_COLLABORATOR and it should not.

mlm483 · 2024-08-14T16:35:49Z

src/main/java/org/breedinginsight/api/auth/ExperimentCollaboratorSecuredRole.java

+public enum ExperimentCollaboratorSecuredRole {
+    EXPERIMENTAL_COLLABORATOR("Experimental Collaborator");


I'm concerned that defining EXPERIMENTAL_COLLABORATOR here as well as in the ProgramSecuredRole enum could lead to confusion.

After seeing how this is used, I recommend deleting this file and using ProgramSecuredRole.EXPERIMENTAL_COLLABORATOR in ExperimentCollaboratorSecuredAnnotationRule::isExperimentCoordinator instead.

mlm483 · 2024-08-14T16:42:58Z

src/main/java/org/breedinginsight/api/auth/ProgramSecuredRoleGroup.java

 public enum ProgramSecuredRoleGroup {
-    ALL_PROGRAM_ROLES(List.of(ProgramSecuredRole.READ_ONLY, ProgramSecuredRole.PROGRAM_ADMIN)),
-    ALL(ListUtils.union(ALL_PROGRAM_ROLES.getProgramRoles(), List.of(ProgramSecuredRole.SYSTEM_ADMIN)));
+    PROGRAM_SCOPED_ROLES(List.of(ProgramSecuredRole.SYSTEM_ADMIN, ProgramSecuredRole.READ_ONLY, ProgramSecuredRole.PROGRAM_ADMIN, ProgramSecuredRole.EXPERIMENTAL_COLLABORATOR));


Per the Jira story, PROGRAM_SCOPED_ROLES should NOT include ProgramSecuredRole.EXPERIMENTAL_COLLABORATOR, it should look like this:

PROGRAM_SCOPED_ROLES(List.of(ProgramSecuredRole.SYSTEM_ADMIN, ProgramSecuredRole.MEMBER, ProgramSecuredRole.BREEDER));

Technically, ProgramSecuredRole.EXPERIMENTAL_COLLABORATOR is also program scoped... if you think of a better name than PROGRAM_SCOPED_ROLES, I'd take it.

What I was imagining when I wrote the story was that everywhere that previously used the ALL group would use the PROGRAM_SCOPED_ROLES group, and then the handful of endpoints that the Experimental Collaborator needs access to would explicitly list all of the roles. Later on, we could refactor and add back ALL to this enum to simplify those endpoints that explicitly list all of the roles, but I wanted to make sure we get the authorization logic right first.

Thanks, I now understand the new role.
The recommended changes have been made.

mlm483 · 2024-08-14T16:45:51Z

src/main/java/org/breedinginsight/brapps/importer/controllers/ImportController.java

    @AddMetadata
-    @ProgramSecured(roles = {ProgramSecuredRole.PROGRAM_ADMIN, ProgramSecuredRole.SYSTEM_ADMIN})
+    @ProgramSecured(roles = {ProgramSecuredRole.PROGRAM_ADMIN, ProgramSecuredRole.SYSTEM_ADMIN, ProgramSecuredRole.EXPERIMENTAL_COLLABORATOR})
    public HttpResponse<Response<DataResponse<ImportMapping>>> getMappings(@PathVariable UUID programId,
                                                                           @QueryValue(defaultValue = "false") Boolean draft) {


Does the Experimental Collaborator role need access to this endpoint? I think this change should be reverted.

The code has been reverted.

mlm483 · 2024-08-14T16:49:43Z

src/main/java/org/breedinginsight/brapps/importer/controllers/ImportController.java

    @Secured(SecurityRule.IS_ANONYMOUS)
+    @ProgramSecured(roles = {ProgramSecuredRole.PROGRAM_ADMIN, ProgramSecuredRole.SYSTEM_ADMIN})
    public HttpResponse<Response<DataResponse<ImportMapping>>> getSystemMappings(@Nullable @QueryValue String importName) {


Does the addition of @ProgramSecured(...) have any effect here, given the @Secured(SecurityRule.IS_ANONYMOUS) annotation? I think this change should be reverted, or, if this endpoint actually needs to be secured (I don't think it does), the @Secured(SecurityRule.IS_ANONYMOUS) annotation should be removed.

It also looks like the ImportControllerIntegrationTest::getSystemMappings test is failing

The @ProgramSecured should not have been added. I have removed it.

@nickpalladino yes it was causing a failure

mlm483 · 2024-08-14T16:51:36Z

src/main/java/org/breedinginsight/brapi/v2/BrAPITrialsController.java

+    private boolean isExperimentCoordinator(ProgramUser programUser){
+        List<Role> roles = programUser.getRoles();
+        return (roles.size()==1 &&
+                ProgramSecuredRole.getEnum(roles.get(0).getDomain())==ProgramSecuredRole.EXPERIMENTAL_COLLABORATOR);
+
+    }


Remove if unused. (I think I wrote this method... it doesn't seem to be needed).

Looks like it was probably moved to ExperimentCollaboratorSecuredAnnotationRule and updated

mlm483 · 2024-08-14T16:52:35Z

src/main/java/org/breedinginsight/brapi/v2/BrAPITrialsController.java

 public class BrAPITrialsController {

    private final String referenceSource;
+    private final SecurityService securityService;
    private final BrAPITrialService experimentService;
    private final ExperimentQueryMapper experimentQueryMapper;

    @Inject
-    public BrAPITrialsController(BrAPITrialService experimentService, ExperimentQueryMapper experimentQueryMapper, @Property(name = "brapi.server.reference-source") String referenceSource) {
+    public BrAPITrialsController(SecurityService securityService, BrAPITrialService experimentService, ExperimentQueryMapper experimentQueryMapper, @Property(name = "brapi.server.reference-source") String referenceSource) {
+        this.securityService = securityService;
        this.experimentService = experimentService;
        this.experimentQueryMapper = experimentQueryMapper;
        this.referenceSource = referenceSource;
    }


Revert the changes that add SecurityService if it's not used.

SecurityService is not used. It has been removed.

mlm483 · 2024-08-14T17:10:22Z

...ain/java/org/breedinginsight/api/auth/rules/ExperimentCollaboratorSecuredAnnotationRule.java

+        return experimentId;
+    }
+
+    private SecurityRuleResult processExperiment(AuthenticatedUser authenticatedUser, String experimentId, String programId) {


Could you rename this method? We use the term "process" in a lot of our import code, and I think for that reason it is confusing to use it here. Something like checkAuthorization or getSecurityRuleResult would be more clear.

renamed checkAuthorization

src/main/java/org/breedinginsight/api/v1/controller/ProgramController.java

mlm483 · 2024-08-14T17:27:26Z

src/main/java/org/breedinginsight/brapi/v2/BrAPIStudiesController.java

    @Produces(MediaType.APPLICATION_JSON)
-    @ProgramSecured(roleGroups = {ProgramSecuredRoleGroup.ALL})
+    @ProgramSecured(roleGroups = {ProgramSecuredRoleGroup.PROGRAM_SCOPED_ROLES})
    public HttpResponse<Response<DataResponse<List<BrAPIStudy>>>> getStudies(


Ultimately this method needs to allow Experimental Collaborator role access. You can either do that in this PR or I will make the change in BI-2258 (where I have also implemented filtering in this method).

The roles now includes ProgramSecuredRole.EXPERIMENTAL_COLLABORATOR

mlm483 · 2024-08-14T17:28:40Z

src/main/java/org/breedinginsight/brapi/v2/BrAPITrialsController.java

    @Produces(MediaType.APPLICATION_JSON)
-    @ProgramSecured(roleGroups = {ProgramSecuredRoleGroup.ALL})
+    @ProgramSecured(roleGroups = {ProgramSecuredRoleGroup.PROGRAM_SCOPED_ROLES})
    public HttpResponse<Response<DataResponse<List<BrAPITrial>>>> getExperiments(


Needs to allow Experimental Collaborator role access. You could do that in the PR or else I'll do it in BI-2258 where I've implemented filtering in this method.

nickpalladino

I'm seeing some integration tests failing that will need to be resolved. Some of them might be fixed by some of the suggested changes in this PR.

src/main/java/org/breedinginsight/api/v1/controller/ProgramController.java

dmeidlin

When navigating to the system Programs section as a SysAdmin the back end returns a 500 response to GET calls to /v1/programs, resulting in no programs being listed.
ERROR o.b.a.v.c.InternalServerErrorHandler - Error Id: 91a2d7e4-f1a9-4527-906e-5caa9a9d0eb3
io.micronaut.http.server.exceptions.HttpServerException: Endpoint does not have program id to check roles against
at org.breedinginsight.api.auth.rules.ProgramSecuredAnnotationRule.check(ProgramSecuredAnnotationRule.java:66)
Same error also when loggin in as Program Admin and Read Only.

davedrp · 2024-08-19T16:09:21Z

When navigating to the system Programs section as a SysAdmin the back end returns a 500 response to GET calls to /v1/programs, resulting in no programs being listed. ERROR o.b.a.v.c.InternalServerErrorHandler - Error Id: 91a2d7e4-f1a9-4527-906e-5caa9a9d0eb3 io.micronaut.http.server.exceptions.HttpServerException: Endpoint does not have program id to check roles against at org.breedinginsight.api.auth.rules.ProgramSecuredAnnotationRule.check(ProgramSecuredAnnotationRule.java:66) Same error also when loggin in as Program Admin and Read Only.

Fixed

mlm483 and others added 17 commits July 26, 2024 11:42

[BI-2255] - refactored ProgramSecuredRoleGroup enum

f9ee83c

[BI-2255] - refactored ProgramSecuredRoleGroup enum

2c5dce8

[BI-2255] WIP

f1279b7

[BI-2255] WIP

e9351a9

Merge pull request #377 from Breeding-Insight/feature/BI-1692

e037a0c

[BI-1692] - Change role names

[autocommit] bumping build number

69e68fe

[BI-2255] WIP

6cabaed

Merge branch 'develop' into feature/BI-2257

de9cad3

Merge pull request #383 from Breeding-Insight/feature/BI-2257

16e3768

BI-2257 - Experimental Collaborator DAO and Services for experimental_program_user_role

[autocommit] bumping build number

1a2c6de

[BI-2255] WIP

1654b5e

Merge branch 'develop' into feature/BI-2255

972848c

[BI-2055] adjusted order of SecuredAnnotationRules

e65f791

[BI-2055] Removed parameters from ExperimentSecuredRule

b7bc15b

[BI-2055] renamed annotation from ExperimentSecured -> ExperimentColl…

ed72bc8

…aboratorSecured

[BI-2055] created the extractExperimentId to make more readable

bf6f0de

[BI-2255] cleaned up code

760f054

davedrp requested review from a team, dmeidlin and nickpalladino and removed request for a team August 13, 2024 15:50

[BI-2255] move initialization of variable to inside if-statement

45fa5d4

davedrp assigned dmeidlin Aug 13, 2024

davedrp marked this pull request as ready for review August 13, 2024 17:30

Merge branch 'develop' into feature/BI-2255

b82badd

mlm483 self-requested a review August 13, 2024 18:58

mlm483 requested changes Aug 14, 2024

View reviewed changes

nickpalladino requested changes Aug 14, 2024

View reviewed changes

[BI-2255]Addressed PR-comments from Matthew

51adb85

davedrp requested a review from mlm483 August 15, 2024 19:41

davedrp requested a review from nickpalladino August 15, 2024 19:41

nickpalladino requested changes Aug 16, 2024

View reviewed changes

src/main/java/org/breedinginsight/api/v1/controller/ProgramController.java Outdated Show resolved Hide resolved

dmeidlin requested changes Aug 16, 2024

View reviewed changes

davedrp added 2 commits August 19, 2024 11:37

Merge branch 'develop' into feature/BI-2255

a9d71ea

[BI-2255] fixed bug

321f8ad

davedrp requested review from dmeidlin and nickpalladino August 19, 2024 16:09

nickpalladino approved these changes Aug 19, 2024

View reviewed changes

mlm483 approved these changes Aug 19, 2024

View reviewed changes

dmeidlin approved these changes Aug 20, 2024

View reviewed changes

davedrp merged commit ff9abc7 into develop Aug 20, 2024

davedrp deleted the feature/BI-2255 branch August 20, 2024 20:25

		public enum ExperimentCollaboratorSecuredRole {
		EXPERIMENTAL_COLLABORATOR("Experimental Collaborator");

Conversation

davedrp commented Aug 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing

Checklist:

Uh oh!

mlm483 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickpalladino Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickpalladino left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dmeidlin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davedrp commented Aug 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

davedrp commented Aug 13, 2024 •

edited

Loading

nickpalladino Aug 14, 2024 •

edited

Loading

dmeidlin left a comment •

edited

Loading