feat(pipeline executions/orca) : Added ability to add roles to manual… #3983

sanopsmx · 2020-10-29T18:55:08Z

feat(pipeline executions/orca) : Added ability to add roles to manual judgment stage.

This is part of: spinnaker/spinnaker#4792.

Enhanced OperationsController.groovy to

Get the application roles, stage roles and the user roles.
Check each of the user roles whether they are contained in the stage and application roles.
Once the user role is contained in the manual judgment stage.
We check for whether the stage role has 'READ', 'WRITE', 'EXECUTE', 'CREATE' role in the application permissions,
if the role has application permission as 'READ', then the user will not be allowed to proceed further to subsequent(downstream) stages.
if the role has application permission as 'WRITE, EXECUTE,CREATE', then the user will be allowed to proceed further to subsequent stages.
If yes/no, set a isAuthorized flag to true/false in each of the stage. By default, all the stages except Manual Judgment are true.

Enhanced ManualJudgmentStage.groovy to

Check for the flag in ManualJudgmentStage.groovy whether to execute to the next stage or not.
If yes/no, continue with the next stages/continues running the same stage.

Enhanced ManualJudgmentStageSpec.groovy to

Modified the testcases as per the requirement.

Added ManualJudgmentAuthzGroupsUtil.groovy

Added a utlity method to check authorized groups of the manual judgment stage.

… judgment stage. This is part of: spinnaker/spinnaker#4792. Enhanced OperationsController.groovy to Get the application roles, stage roles and the user roles. Check each of the user roles whether they are contained in the stage and application roles. Once the user role is contained in the manual judgment stage. We check for whether the stage role has 'READ', 'WRITE', 'EXECUTE', 'CREATE' role in the application permissions, if the role has application permission as 'READ', then the user will not be allowed to proceed further to subsequent(downstream) stages. if the role has application permission as 'WRITE, EXECUTE,CREATE', then the user will be allowed to proceed further to subsequent stages. If yes/no, set a isAuthorized flag to true/false in each of the stage. By default, all the stages except Manual Judgment are true. Enhanced ManualJudgmentStage.groovy to Check for the flag in ManualJudgmentStage.groovy whether to execute to the next stage or not. If yes/no, continue with the next stages/continues running the same stage. Enhanced ManualJudgmentStageSpec.groovy to Modified the testcases as per the requirement. Added ManualJudgmentAuthzGroupsUtil.groovy Added a utlity method to check authorized groups of the manual judgment stage.

robzienert · 2020-10-29T19:41:39Z

orca-echo/src/main/groovy/com/netflix/spinnaker/orca/echo/pipeline/ManualJudgmentStage.groovy

+    WaitForManualJudgmentTask() {
+    }
+
+    WaitForManualJudgmentTask(Optional<FiatPermissionEvaluator> fpe) {
+      this.fiatPermissionEvaluator = fpe.orElse(null)
+    }


Remove both. Neither of these constructors are necessary and would cause issues when doing dependency injection because none of them are annotated with @Autowired.

robzienert · 2020-10-29T19:43:05Z

...ho/src/main/groovy/com/netflix/spinnaker/orca/echo/util/ManualJudgmentAuthzGroupsUtil.groovy

@@ -0,0 +1,50 @@
+/*


Convert to Java: We don't accept new files in src/main that is Groovy.

robzienert · 2020-10-29T19:43:32Z

...ho/src/main/groovy/com/netflix/spinnaker/orca/echo/util/ManualJudgmentAuthzGroupsUtil.groovy

@@ -0,0 +1,50 @@
+/*
+ * Copyright 2020 Netflix, Inc.


Should probably update this new file's copyright such that it reflects OpsMx's legal name instead of Netflix.

robzienert · 2020-10-29T19:46:08Z

orca-web/src/main/groovy/com/netflix/spinnaker/orca/controllers/OperationsController.groovy

+        if (application.getPermission().permissions && application.getPermission().permissions.permissions) {
+          def permissions = objectMapper.convertValue(application.getPermission().permissions.permissions,
+              new TypeReference<Map<String, Object>>() {})
+          UserPermission.View permission = fiatPermissionEvaluator.getPermission(username);


fiatPermissionEvaluator is nullable (based on @Autowired(required = false). If fiat is not enabled, this code block will cause an NPE.

robzienert · 2020-10-29T19:48:27Z

orca-web/src/main/groovy/com/netflix/spinnaker/orca/controllers/OperationsController.groovy

+      Application application = front50Service.get(applicationName)
+      if (application) {


This usage is still incorrect - front50Service will throw a RetrofitError if applicationName is not found, so the conditional if (application) is always going to evaluate to true. I'd recommend doing something like this instead:

https://github.com/spinnaker/orca/blob/master/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/pollers/EphemeralServerGroupsPoller.java#L274-L283

… judgment stage. This is part of: spinnaker/spinnaker#4792. Enhanced OperationsController.groovy to Get the application roles, stage roles and the user roles. Check each of the user roles whether they are contained in the stage and application roles. Once the user role is contained in the manual judgment stage. We check for whether the stage role has 'READ', 'WRITE', 'EXECUTE', 'CREATE' role in the application permissions, if the role has application permission as 'READ', then the user will not be allowed to proceed further to subsequent(downstream) stages. if the role has application permission as 'WRITE, EXECUTE,CREATE', then the user will be allowed to proceed further to subsequent stages. If yes/no, set a isAuthorized flag to true/false in each of the stage. By default, all the stages except Manual Judgment are true. Enhanced ManualJudgmentStage.groovy to Check for the flag in ManualJudgmentStage.groovy whether to execute to the next stage or not. If yes/no, continue with the next stages/continues running the same stage. Enhanced ManualJudgmentStageSpec.groovy to Modified the testcases as per the requirement. Added ManualJudgmentAuthzGroupsUtil.groovy Added a utlity method to check authorized groups of the manual judgment stage.

sanopsmx · 2020-10-31T18:51:12Z

Addressed all the feedback and review.

robzienert · 2020-11-02T17:26:26Z

orca-echo/src/main/groovy/com/netflix/spinnaker/orca/echo/pipeline/ManualJudgmentStage.groovy

+      if (!Strings.isNullOrEmpty(username)) {
+        UserPermission.View permission = fiatPermissionEvaluator.getPermission(username);
+        if (permission == null) { // Should never happen?
+          return false;


If this should never happen, I presume we want to know when it does happen? Perhaps would be useful to have a warn-level log about it?

log.warn("Attempted to get user permission for '$username' but none were found." or something.

robzienert

Would like to see a test for the utility class, as well as some coverage for the controller logic that has been added. After that, we're looking pretty good, thanks!

robzienert · 2020-11-02T17:27:29Z

orca-echo/src/main/java/com/netflix/spinnaker/orca/echo/util/ManualJudgmentAuthzGroupsUtil.java

+  public static boolean checkAuthorizedGroups(
+      List<String> userRoles, List<String> stageRoles, Map<String, Object> permissions) {


Expected to see a test for this class.

6 test cases are written/added in ManualJudgmentStageSpec.groovy only to test this logic. Please have a look at this test case.

@unroll
void "should return execution status based on authorizedGroups"() {

Seems weird to have a test case for a different class (ManualJudgmentStage) specifically testing a different class, doesn't it?

We are using the logic in the ManualJudgmentStage.groovy. So i have written 6 test cases to check the ManualJudgmentAuthzGroupsUtil.java logic in the ManualJudgmentStageSpec.groovy.

You're also using it in a controller, which means that the test cases don't belong where there are today.

I'd say the tests that are testing this code should live in ManualJudgmentAuthzGroupsUtilSpec, then test the implementation in ManualJudgmentStageSpec and OperationsControllerSpec, as you would with any other unit under test.

robzienert · 2020-11-02T17:29:17Z

orca-web/src/main/groovy/com/netflix/spinnaker/orca/controllers/OperationsController.groovy

+        if (optApplication.isPresent()) {
+          Application application = optApplication.get();


nit: Could be simplified to getApplication(applicationName).ifPresent(application -> { /* ... */ });

sanopsmx · 2020-11-03T11:33:36Z

Can you review the code

sanopsmx · 2020-11-03T11:41:20Z

Raised a new PR.

#3988

This was referenced Oct 29, 2020

Design document for adding roles to manual judgment stage. spinnaker/governance#80

Merged

feat(pipeline executions/orca) : Added ability to add roles to manual… #3947

Closed

robzienert requested changes Oct 30, 2020

View reviewed changes

sanopsmx added 3 commits October 31, 2020 22:43

sanopsmx requested a review from robzienert October 31, 2020 18:50

robzienert reviewed Nov 2, 2020

View reviewed changes

sanopsmx requested a review from robzienert November 3, 2020 11:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(pipeline executions/orca) : Added ability to add roles to manual… #3983

feat(pipeline executions/orca) : Added ability to add roles to manual… #3983

sanopsmx commented Oct 29, 2020

robzienert Oct 29, 2020

sanopsmx Oct 31, 2020

robzienert Oct 29, 2020

sanopsmx Oct 31, 2020

robzienert Oct 29, 2020

sanopsmx Oct 31, 2020

robzienert Oct 29, 2020

sanopsmx Oct 31, 2020

robzienert Oct 29, 2020

sanopsmx Oct 31, 2020

sanopsmx commented Oct 31, 2020

robzienert Nov 2, 2020

sanopsmx Nov 3, 2020

robzienert left a comment

robzienert Nov 2, 2020

sanopsmx Nov 2, 2020

robzienert Nov 2, 2020

sanopsmx Nov 2, 2020

robzienert Nov 2, 2020

sanopsmx Nov 3, 2020

robzienert Nov 2, 2020

sanopsmx Nov 3, 2020

sanopsmx commented Nov 3, 2020

sanopsmx commented Nov 3, 2020

		Application application = front50Service.get(applicationName)
		if (application) {

		public static boolean checkAuthorizedGroups(
		List<String> userRoles, List<String> stageRoles, Map<String, Object> permissions) {

		if (optApplication.isPresent()) {
		Application application = optApplication.get();

feat(pipeline executions/orca) : Added ability to add roles to manual… #3983

Are you sure you want to change the base?

feat(pipeline executions/orca) : Added ability to add roles to manual… #3983

Conversation

sanopsmx commented Oct 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanopsmx commented Oct 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robzienert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanopsmx commented Nov 3, 2020

sanopsmx commented Nov 3, 2020