Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#1762 - Check approvers properly against allowed roles #1779

Conversation

Projects
None yet
3 participants
@arvindsv
Copy link
Member

commented Jan 8, 2016

Related to #1762.

@arvindsv arvindsv added this to the Release 16.1 milestone Jan 8, 2016

jyotisingh added a commit that referenced this pull request Jan 8, 2016

Merge pull request #1779 from arvindsv/1762_fix_issue_with_operate_pe…
…rmissions_and_approvals

#1762 - Check approvers properly against allowed roles

@jyotisingh jyotisingh merged commit 4e9c713 into gocd:master Jan 8, 2016

@arvindsv

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2016

@jyotisingh: I'm worried that this breaks existing configs. Earlier a user who was not in the "operate" auth group at pipeline group level could be given approval permission for a stage in a pipeline in that group. Now, it is not allowed.

That is why this build is failing. It's an easy fix in the tests (patch below), but we need to consider the effect on existing users.

I'm considering whether to change that clause back to hasOperationPermissionDefined, to keep that old (wrong) behavior, till we figure out what to do with existing configs. My gut feel is that we'll need to bit that bullet sometime and make it happen, but the error message shown is not good enough. It says:

User \"operate\" who is not authorized to operate pipeline group can not be authorized to approve stage

It should, at least, say:

User \"operate\" who is not authorized to operate pipeline group "pipeline_group1" can not be authorized to approve stage in pipeline "pipeline2"

What do you think?

Patch for fixing functional tests, in case I'm not around and you decide that the test needs to be fixed:

diff --git a/src/test/java/config/stage-security-cruise-config.xml b/src/test/java/config/stage-security-cruise-config.xml
index 8380fbe..92d2e2e 100644
--- a/src/test/java/config/stage-security-cruise-config.xml
+++ b/src/test/java/config/stage-security-cruise-config.xml
@@ -100,6 +100,9 @@
                <user>view</user>
                <role>misc</role>
            </admins>
+           <operate>
+               <user>operate</user>
+           </operate>
        </authorization>
        <pipeline name="p3">
            <materials>

I believe that should work. I've been having some trouble getting twist tests working on my Mac. Firefox doesn't come up and the README we have on it, is not complete.

@rajiesh

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2016

@arvindsv the regression testcase failed after merging this fix. https://build.go.cd/go/tab/build/detail/regression-gauge/92/regression-linux/2/functional-tests-runInstance-4

The failure is due to the below section of the config file which is setup by this test,

<pipelines group='B'>
        <authorization>
            <admins>
                <user>view</user>
                <role>misc</role>
            </admins>
        </authorization>
        <pipeline name="p3">
            <materials>
                <hg url="$hgurl-basic-pipeline" dest="hg" autoUpdate="false" />
            </materials>
            <stage name="defaultStage">
                <approval type="manual">
                    <authorization>
                        <user>operate</user>
                    </authorization>
                </approval>
                <jobs>
                    <job name="defaultJob">
                        <tasks>
                            <exec command="ls" />
                        </tasks>
                    </job>
                </jobs>
            </stage>
        </pipeline>
    </pipelines>

The error in config validation is User "operate" who is not authorized to operate pipeline group can not be authorized to approve stage. The 'operate' user is not provided operate permission for the group but specified as authorizer of stage - is this not allowed as part of this fix?

@arvindsv

This comment has been minimized.

Copy link
Member Author

commented Jan 11, 2016

The 'operate' user is not provided operate permission for the group but specified as authorizer of stage - is this not allowed as part of this fix?

@rajiesh: As I mentioned in this comment, that's not allowed after this fix. It's the correct thing to do, but it makes the config invalid and will affect configs of existing users.

I'm thinking of getting the old behavior back, so that this check is done only if operate user is defined in the authorization section. Though that is technically not completely correct, it keeps part of the old behavior working and will keep old configs working till we figure out a way to notify users of this behavior change.

Thoughts? /cc @jyotisingh.

@arvindsv

This comment has been minimized.

Copy link
Member Author

commented Jan 11, 2016

I mean, I don't want to have to fix the test, because that implies that users will have to fix their config. I'd rather get part of the old behavior back and keep this test the same for now.

arvindsv added a commit to arvindsv/gocd that referenced this pull request Jan 11, 2016

gocd#1762, gocd#1779 - Bring back part of bug for backward compatibility
Earlier a user who was not in the 'operate' part of 'authorization' for a pipeline group was
allowed to be a stage approver. Changes made for gocd#1762 (in gocd#1779) removed that ability. But,
it is needed for backward compatibility for now. Otherwise, users' configs will be marked
invalid, without any change and without any notification to them. Will cause problems during
an upgrade.

arvindsv added a commit to arvindsv/gocd that referenced this pull request Jan 11, 2016

@jyotisingh

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2016

@arvindsv - Does it make sense to add a migration that would automatically provide operate permissions for the group to such a user? That should take care of the older/incorrect configs, and we leave the code as is. What do you say?

@arvindsv

This comment has been minimized.

Copy link
Member Author

commented Jan 11, 2016

I think doing it through the XSLT migration will take effort. We can do it in a different PR. For now, I feel this is an improvement over what we had (because it fixes one of two bugs).

jyotisingh added a commit that referenced this pull request Jan 11, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.