Merge pull request kubesphere#3286 from LinuxSuRen/fix-pip-input-approve-check

Fix the incorrect approvable check of Pipeline input

Author: ks-ci-bot

hack/docker_build.sh

@@ -17,8 +17,16 @@ tag_for_branch() {
 }
 
 get_repo() {
-    local repo=$1
+    local repo=${REPO} # read from env
     repo=${repo:-kubespheredev}
+    if [[ "$1" != "" ]]; then
+      repo="$1"
+    fi
+
+    # set the default value if there's no user defined
+    if [[ "${repo}" == "" ]]; then
+      repo="kubespheredev"
+    fi
     echo "$repo"
 }
 

pkg/apiserver/apiserver.go

@@ -254,7 +254,7 @@ func (s *APIServer) installKubeSphereAPIs() {
 		s.KubernetesClient.KubeSphere(),
 		s.S3Client,
 		s.Config.DevopsOptions.Host,
-		amOperator))
+		rbacAuthorizer))
 	urlruntime.Must(devopsv1alpha3.AddToContainer(s.container,
 		s.DevopsClient,
 		s.KubernetesClient.Kubernetes(),

pkg/apiserver/authorization/authorizer/interfaces.go

@@ -133,7 +133,7 @@ func (a AttributesRecord) GetVerb() string {
 }
 
 func (a AttributesRecord) IsReadOnly() bool {
-	return a.Verb == "get" || a.Verb == "list" || a.Verb == "watch"
+	return a.Verb == VerbGet || a.Verb == VerbList || a.Verb == VerbWatch
 }
 
 func (a AttributesRecord) GetCluster() string {
@@ -199,3 +199,14 @@ const (
 	// to allow or deny an action.
 	DecisionNoOpinion
 )
+
+const (
+	// VerbList represents the verb of listing resources
+	VerbList = "list"
+	// VerbCreate represents the verb of creating a resource
+	VerbCreate = "create"
+	// VerbGet represents the verb of getting a resource or resources
+	VerbGet = "get"
+	// VerbWatch represents the verb of watching a resource
+	VerbWatch = "watch"
+)

pkg/kapis/devops/v1alpha2/devops.go

@@ -23,9 +23,10 @@ import (
 	"github.com/emicklei/go-restful"
 	"k8s.io/apiserver/pkg/authentication/user"
 	log "k8s.io/klog"
+	"k8s.io/klog/v2"
 	"kubesphere.io/kubesphere/pkg/api"
 	"kubesphere.io/kubesphere/pkg/apis/devops/v1alpha3"
-	iamv1alpha2 "kubesphere.io/kubesphere/pkg/apis/iam/v1alpha2"
+	"kubesphere.io/kubesphere/pkg/apiserver/authorization/authorizer"
 	"kubesphere.io/kubesphere/pkg/apiserver/request"
 	"kubesphere.io/kubesphere/pkg/constants"
 	"kubesphere.io/kubesphere/pkg/models/devops"
@@ -279,11 +280,53 @@ func (h *ProjectPipelineHandler) GetPipelineRunNodes(req *restful.Request, resp
 	resp.WriteAsJson(res)
 }
 
-func (h *ProjectPipelineHandler) approvableCheck(nodes []clientDevOps.NodesDetail, req *restful.Request) {
-	currentUserName, roleName := h.getCurrentUser(req)
+// there're two situation here:
+// 1. the particular submitters exist
+// the users who are the owner of this Pipeline or the submitter of this Pipeline, or has the auth to create a DevOps project
+// 2. no particular submitters
+// only the owner of this Pipeline can approve or reject it
+func (h *ProjectPipelineHandler) approvableCheck(nodes []clientDevOps.NodesDetail, pipe pipelineParam) {
+	var userInfo user.Info
+	var ok bool
+	var isAdmin bool
 	// check if current user belong to the admin group, grant it if it's true
-	isAdmin := roleName == iamv1alpha2.PlatformAdmin
+	if userInfo, ok = request.UserFrom(pipe.Context); ok {
+		createAuth := authorizer.AttributesRecord{
+			User:            userInfo,
+			Verb:            authorizer.VerbCreate,
+			Workspace:       pipe.Workspace,
+			Resource:        "devopsprojects",
+			ResourceRequest: true,
+			ResourceScope:   request.DevOpsScope,
+		}
+
+		if decision, _, err := h.authorizer.Authorize(createAuth); err == nil {
+			isAdmin = decision == authorizer.DecisionAllow
+		} else {
+			// this is an expected case, printing the debug info for troubleshooting
+			klog.V(8).Infof("authorize failed with '%v', error is '%v'",
+				createAuth, err)
+		}
+	} else {
+		klog.V(6).Infof("cannot get the current user when checking the approvable with pipeline '%s/%s'",
+			pipe.ProjectName, pipe.Name)
+		return
+	}
 
+	var createdByCurrentUser bool // indicate if the current user is the owner
+	if pipeline, err := h.devopsOperator.GetPipelineObj(pipe.ProjectName, pipe.Name); err == nil {
+		if creator, ok := pipeline.GetAnnotations()[constants.CreatorAnnotationKey]; ok {
+			createdByCurrentUser = userInfo.GetName() == creator
+		} else {
+			klog.V(6).Infof("annotation '%s' is necessary but it is missing from '%s/%s'",
+				constants.CreatorAnnotationKey, pipe.ProjectName, pipe.Name)
+		}
+	} else {
+		klog.V(6).Infof("cannot find pipeline '%s/%s', error is '%v'", pipe.ProjectName, pipe.Name, err)
+		return
+	}
+
+	// check every input steps if it's approvable
 	for i, node := range nodes {
 		if node.State != clientDevOps.StatePaused {
 			continue
@@ -294,7 +337,7 @@ func (h *ProjectPipelineHandler) approvableCheck(nodes []clientDevOps.NodesDetai
 				continue
 			}
 
-			nodes[i].Steps[j].Approvable = isAdmin || step.Input.Approvable(currentUserName)
+			nodes[i].Steps[j].Approvable = isAdmin || createdByCurrentUser || step.Input.Approvable(userInfo.GetName())
 		}
 	}
 }
@@ -310,33 +353,8 @@ func (h *ProjectPipelineHandler) createdBy(projectName string, pipelineName stri
 	return false
 }
 
-func (h *ProjectPipelineHandler) getCurrentUser(req *restful.Request) (username, roleName string) {
-	var userInfo user.Info
-	var ok bool
-	var err error
-
-	ctx := req.Request.Context()
-	if userInfo, ok = request.UserFrom(ctx); ok {
-		var role *iamv1alpha2.GlobalRole
-		username = userInfo.GetName()
-		if role, err = h.amInterface.GetGlobalRoleOfUser(username); err == nil {
-			roleName = role.Name
-		}
-	}
-	return
-}
-
 func (h *ProjectPipelineHandler) hasSubmitPermission(req *restful.Request) (hasPermit bool, err error) {
-	currentUserName, roleName := h.getCurrentUser(req)
-	projectName := req.PathParameter("devops")
-	pipelineName := req.PathParameter("pipeline")
-	// check if current user belong to the admin group or he's the owner, grant it if it's true
-	if roleName == iamv1alpha2.PlatformAdmin || h.createdBy(projectName, pipelineName, currentUserName) {
-		hasPermit = true
-		return
-	}
-
-	// step 2, check if current user if was addressed
+	pipeParam := parsePipelineParam(req)
 	httpReq := &http.Request{
 		URL:      req.Request.URL,
 		Header:   req.Request.Header,
@@ -350,7 +368,9 @@ func (h *ProjectPipelineHandler) hasSubmitPermission(req *restful.Request) (hasP
 
 	// check if current user can approve this input
 	var res []clientDevOps.NodesDetail
-	if res, err = h.devopsOperator.GetNodesDetail(projectName, pipelineName, runId, httpReq); err == nil {
+	if res, err = h.devopsOperator.GetNodesDetail(pipeParam.ProjectName, pipeParam.Name, runId, httpReq); err == nil {
+		h.approvableCheck(res, parsePipelineParam(req))
+
 		for _, node := range res {
 			if node.ID != nodeId {
 				continue
@@ -361,7 +381,7 @@ func (h *ProjectPipelineHandler) hasSubmitPermission(req *restful.Request) (hasP
 					continue
 				}
 
-				hasPermit = step.Input.Approvable(currentUserName)
+				hasPermit = step.Approvable
 				break
 			}
 			break
@@ -412,7 +432,7 @@ func (h *ProjectPipelineHandler) GetNodesDetail(req *restful.Request, resp *rest
 		parseErr(err, resp)
 		return
 	}
-	h.approvableCheck(res, req)
+	h.approvableCheck(res, parsePipelineParam(req))
 
 	resp.WriteAsJson(res)
 }
@@ -620,10 +640,19 @@ func (h *ProjectPipelineHandler) GetBranchNodesDetail(req *restful.Request, resp
 		parseErr(err, resp)
 		return
 	}
-	h.approvableCheck(res, req)
+	h.approvableCheck(res, parsePipelineParam(req))
 	resp.WriteAsJson(res)
 }
 
+func parsePipelineParam(req *restful.Request) pipelineParam {
+	return pipelineParam{
+		Workspace:   req.PathParameter("workspace"),
+		ProjectName: req.PathParameter("devops"),
+		Name:        req.PathParameter("pipeline"),
+		Context:     req.Request.Context(),
+	}
+}
+
 func (h *ProjectPipelineHandler) GetPipelineBranch(req *restful.Request, resp *restful.Response) {
 	projectName := req.PathParameter("devops")
 	pipelineName := req.PathParameter("pipeline")

pkg/kapis/devops/v1alpha2/handler.go

@@ -17,10 +17,10 @@ limitations under the License.
 package v1alpha2
 
 import (
+	"kubesphere.io/kubesphere/pkg/apiserver/authorization/authorizer"
 	"kubesphere.io/kubesphere/pkg/client/clientset/versioned"
 	"kubesphere.io/kubesphere/pkg/client/informers/externalversions"
 	"kubesphere.io/kubesphere/pkg/models/devops"
-	"kubesphere.io/kubesphere/pkg/models/iam/am"
 	devopsClient "kubesphere.io/kubesphere/pkg/simple/client/devops"
 	"kubesphere.io/kubesphere/pkg/simple/client/s3"
 	"kubesphere.io/kubesphere/pkg/simple/client/sonarqube"
@@ -29,18 +29,19 @@ import (
 type ProjectPipelineHandler struct {
 	devopsOperator          devops.DevopsOperator
 	projectCredentialGetter devops.ProjectCredentialGetter
-	amInterface             am.AccessManagementInterface
+	authorizer              authorizer.Authorizer
 }
 
 type PipelineSonarHandler struct {
 	pipelineSonarGetter devops.PipelineSonarGetter
 }
 
-func NewProjectPipelineHandler(devopsClient devopsClient.Interface, ksInformers externalversions.SharedInformerFactory, amInterface am.AccessManagementInterface) ProjectPipelineHandler {
+func NewProjectPipelineHandler(devopsClient devopsClient.Interface, ksInformers externalversions.SharedInformerFactory,
+	authorizer authorizer.Authorizer) ProjectPipelineHandler {
 	return ProjectPipelineHandler{
 		devopsOperator:          devops.NewDevopsOperator(devopsClient, nil, nil, ksInformers, nil),
 		projectCredentialGetter: devops.NewProjectCredentialOperator(devopsClient),
-		amInterface:             amInterface,
+		authorizer:              authorizer,
 	}
 }
 

pkg/kapis/devops/v1alpha2/register.go

@@ -17,25 +17,25 @@ limitations under the License.
 package v1alpha2
 
 import (
+	"context"
 	"fmt"
 	"github.com/emicklei/go-restful"
 	"github.com/emicklei/go-restful-openapi"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/util/proxy"
 	"k8s.io/klog"
 	devopsv1alpha1 "kubesphere.io/kubesphere/pkg/apis/devops/v1alpha1"
+	"kubesphere.io/kubesphere/pkg/apiserver/authorization/authorizer"
 	"kubesphere.io/kubesphere/pkg/apiserver/runtime"
 	"kubesphere.io/kubesphere/pkg/client/clientset/versioned"
 	"kubesphere.io/kubesphere/pkg/client/informers/externalversions"
 	"kubesphere.io/kubesphere/pkg/constants"
-	"kubesphere.io/kubesphere/pkg/models/iam/am"
 	"kubesphere.io/kubesphere/pkg/simple/client/devops/jenkins"
 	"kubesphere.io/kubesphere/pkg/simple/client/s3"
 	"kubesphere.io/kubesphere/pkg/simple/client/sonarqube"
 	"net/url"
 	"strings"
 
-	//"kubesphere.io/kubesphere/pkg/models/devops"
 	"kubesphere.io/kubesphere/pkg/simple/client/devops"
 	"net/http"
 )
@@ -47,10 +47,12 @@ const (
 
 var GroupVersion = schema.GroupVersion{Group: GroupName, Version: "v1alpha2"}
 
-func AddToContainer(container *restful.Container, ksInformers externalversions.SharedInformerFactory, devopsClient devops.Interface, sonarqubeClient sonarqube.SonarInterface, ksClient versioned.Interface, s3Client s3.Interface, endpoint string, amInterface am.AccessManagementInterface) error {
+func AddToContainer(container *restful.Container, ksInformers externalversions.SharedInformerFactory, devopsClient devops.Interface,
+	sonarqubeClient sonarqube.SonarInterface, ksClient versioned.Interface, s3Client s3.Interface, endpoint string,
+	authorizer authorizer.Authorizer) error {
 	ws := runtime.NewWebService(GroupVersion)
 
-	err := AddPipelineToWebService(ws, devopsClient, ksInformers, amInterface)
+	err := AddPipelineToWebService(ws, devopsClient, ksInformers, authorizer)
 	if err != nil {
 		return err
 	}
@@ -75,12 +77,13 @@ func AddToContainer(container *restful.Container, ksInformers externalversions.S
 	return nil
 }
 
-func AddPipelineToWebService(webservice *restful.WebService, devopsClient devops.Interface, ksInformers externalversions.SharedInformerFactory, amInterface am.AccessManagementInterface) error {
+func AddPipelineToWebService(webservice *restful.WebService, devopsClient devops.Interface, ksInformers externalversions.SharedInformerFactory,
+	authorizer authorizer.Authorizer) error {
 
 	projectPipelineEnable := devopsClient != nil
 
 	if projectPipelineEnable {
-		projectPipelineHandler := NewProjectPipelineHandler(devopsClient, ksInformers, amInterface)
+		projectPipelineHandler := NewProjectPipelineHandler(devopsClient, ksInformers, authorizer)
 
 		webservice.Route(webservice.GET("/devops/{devops}/credentials/{credential}/usage").
 			To(projectPipelineHandler.GetProjectCredentialUsage).
@@ -732,6 +735,14 @@ func AddJenkinsToContainer(webservice *restful.WebService, devopsClient devops.I
 	return nil
 }
 
+type pipelineParam struct {
+	Workspace   string
+	ProjectName string
+	Name        string
+
+	Context context.Context
+}
+
 type errorResponder struct{}
 
 func (e *errorResponder) Error(w http.ResponseWriter, req *http.Request, err error) {

pkg/simple/client/devops/pipeline.go

@@ -1122,14 +1122,10 @@ func (i *Input) GetSubmitters() (submitters []string) {
 func (i *Input) Approvable(identify string) (ok bool) {
 	submitters := i.GetSubmitters()
 
-	// it means anyone can approve this if there's no specific one
-	if len(submitters) == 0 {
-		ok = true
-	} else {
-		for _, submitter := range submitters {
-			if submitter == identify {
-				ok = true
-			}
+	for _, submitter := range submitters {
+		if submitter == identify {
+			ok = true
+			break
 		}
 	}
 	return

pkg/simple/client/devops/pipeline_test.go

@@ -19,8 +19,8 @@ func TestGetSubmitters(t *testing.T) {
 func TestApprovable(t *testing.T) {
 	input := &Input{}
 
-	assert.Equal(t, input.Approvable(""), true, "should allow anyone to approve it if there's no submitter given")
-	assert.Equal(t, input.Approvable("fake"), true, "should allow anyone to approve it if there's no submitter given")
+	assert.Equal(t, input.Approvable(""), false, "should allow anyone to approve it if there's no submitter given")
+	assert.Equal(t, input.Approvable("fake"), false, "should allow anyone to approve it if there's no submitter given")
 
 	input.Submitter = "fake"
 	assert.Equal(t, input.Approvable(""), false, "should not approve by nobody if there's a particular submitter")