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.

Sign up for GitHub

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

Jump to bottom

Fix args check for role and scc modify #6868

Merged
merged 1 commit into from Feb 5, 2016
Merged

Fix args check for role and scc modify #6868

merged 1 commit into from Feb 5, 2016

Conversation

nak3
Copy link
Contributor

@nak3 nak3 commented Jan 28, 2016

No description provided.

@nak3
Copy link
Contributor Author

nak3 commented Jan 28, 2016

This patch fixes following go panic.

$ oc policy add-role-to-user -z foo
panic: runtime error: index out of range

goroutine 1 [running]:
github.com/openshift/origin/pkg/cmd/admin/policy.(*RoleModificationOptions).CompleteUserWithSA(0xc20806fb00, 0xc20801cce0, 0xc208268700, 0x0, 0x2, 0xc208274790, 0x1, 0x1, 0x0, 0x0)
    /builddir/build/BUILD/atomic-openshift-git-0.b57e8bd/_build/src/github.com/openshift/origin/pkg/cmd/admin/policy/modify_roles.go:233 +0x81a
github.com/openshift/origin/pkg/cmd/admin/policy.func·002(0xc20827e200, 0xc208268700, 0x0, 0x2)
    /builddir/build/BUILD/atomic-openshift-git-0.b57e8bd/_build/src/github.com/openshift/origin/pkg/cmd/admin/policy/modify_roles.go:74 +0x7a
...
~~

liggitt
liggitt reviewed Jan 28, 2016
View reviewed changes
pkg/cmd/admin/policy/modify_roles.go
@@ -226,7 +226,9 @@ func NewCmdRemoveClusterRoleFromUser(name, fullName string, f *clientcmd.Factory
}

func (o *RoleModificationOptions) CompleteUserWithSA(f *clientcmd.Factory, args []string, saNames []string) error {
if (len(args) < 2) && (len(saNames) == 0) {
if (len(args) < 1) && (len(saNames) == 1) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather see:

if len(args) < 1 {
    return errors.New("you must specify a role")
}
if (len(args) < 2) && (len(saNames) == 0) {
    return errors.New("you must specify a user or a service account")
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and actually, put the if (len(args) < 2) && (len(saNames) == 0) check after we've appended args[1:] to o.Users and check if (len(o.Users) == 0) && (len(saNames) == 0) instead

@liggitt liggitt self-assigned this Jan 28, 2016
@liggitt
Copy link
Contributor

liggitt commented Jan 28, 2016

Add tests to test/cmd/policy.sh with the bad invocations that expect failure and text

@nak3
Copy link
Contributor Author

nak3 commented Jan 29, 2016

@liggitt Thank you. I updated.

liggitt
liggitt reviewed Jan 29, 2016
View reviewed changes
pkg/cmd/admin/policy/modify_roles.go
@@ -235,6 +235,10 @@ func (o *RoleModificationOptions) CompleteUserWithSA(f *clientcmd.Factory, args
o.Users = append(o.Users, args[1:]...)
}

if (len(o.Users) == 0) && (len(saNames) == 0) {
return errors.New("you must specify at least two arguments: <role> <user> [user]...")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...at least one user or service account

@nak3
Copy link
Contributor Author

nak3 commented Jan 30, 2016

Sorry, it was my mistake. I updated.

@nak3
Copy link
Contributor Author

nak3 commented Jan 30, 2016

Although it might be better to update the Usage: section of oc policy add-role-to-user -h, this PR provides the fix of the args checks. (@liggitt, but if you think it should be updated in this PR, I will do. )

@liggitt
Copy link
Contributor

liggitt commented Jan 30, 2016

sure, go ahead and update the example text while we're in here

@liggitt
Copy link
Contributor

liggitt commented Jan 30, 2016

(don't forget to regen the docs to pick up the new example)

@nak3
Copy link
Contributor Author

nak3 commented Jan 31, 2016

OK, I updated.

liggitt
liggitt reviewed Feb 1, 2016
View reviewed changes
pkg/cmd/admin/policy/modify_roles.go
Use: name + " ROLE (USER | -z SERVICEACCOUNT) [USER ...]",
Short: "Add users or serviceaccount to a role in the current project",
Long: `Add users or serviceaccount to a role in the current project`,
Example: fmt.Sprintf(addRoleToUserExample, fullName),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"service accounts"

@nak3
Copy link
Contributor Author

nak3 commented Feb 2, 2016

Thank you again. I updated.

The robot service account in top-secret project is my favorite in the official-doc. (I removed the example from this PR.)

liggitt
liggitt reviewed Feb 2, 2016
View reviewed changes
test/cmd/policy.sh
os::cmd::expect_failure_and_text 'oc policy add-role-to-user' 'you must specify a role'
os::cmd::expect_failure_and_text 'oc policy add-role-to-user -z NamespaceWithoutRole' 'you must specify a role'
os::cmd::expect_failure_and_text 'oc policy add-role-to-user view' 'you must specify at least two arguments'

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't the error message change here?

@nak3
Copy link
Contributor Author

nak3 commented Feb 2, 2016

Sorry! I updated.

@liggitt
Copy link
Contributor

liggitt commented Feb 2, 2016

[test]

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 51e0f8a

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/743/)

@liggitt
Copy link
Contributor

liggitt commented Feb 5, 2016

LGTM, [merge]

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/4857/) (Image: devenv-rhel7_3339)

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 51e0f8a

openshift-bot pushed a commit that referenced this pull request Feb 5, 2016
Merge pull request #6868 from nak3/fix-role-scc-arg
840aaa5 
Merged by openshift-bot
@openshift-bot openshift-bot merged commit 840aaa5 into openshift:master Feb 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@liggitt liggitt

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@nak3 @liggitt @openshift-bot