coder · Emyrk · May 25, 2022 · May 24, 2022 · May 24, 2022 · May 24, 2022
diff --git a/coderd/coderd.go b/coderd/coderd.go
@@ -170,6 +170,7 @@ func newRouter(options *Options, a *api) chi.Router {
 				r.Route("/{user}", func(r chi.Router) {
 					r.Use(
 						httpmw.ExtractUserParam(options.Database),
+						httpmw.ExtractOrganizationMemberParam(options.Database),
 					)
 					r.Put("/roles", a.putMemberRoles)
 				})
@@ -201,6 +202,7 @@ func newRouter(options *Options, a *api) chi.Router {
 		r.Route("/templateversions/{templateversion}", func(r chi.Router) {
 			r.Use(
 				apiKeyMiddleware,
+				authRolesMiddleware,
 				httpmw.ExtractTemplateVersionParam(options.Database),
 			)
 
@@ -289,6 +291,7 @@ func newRouter(options *Options, a *api) chi.Router {
 		r.Route("/workspaceresources/{workspaceresource}", func(r chi.Router) {
 			r.Use(
 				apiKeyMiddleware,
+				authRolesMiddleware,
 				httpmw.ExtractWorkspaceResourceParam(options.Database),
 				httpmw.ExtractWorkspaceParam(options.Database),
 			)

diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go
@@ -17,6 +17,8 @@ import (
 	"github.com/coder/coder/coderd/coderdtest"
 	"github.com/coder/coder/coderd/rbac"
 	"github.com/coder/coder/codersdk"
+	"github.com/coder/coder/provisioner/echo"
+	"github.com/coder/coder/provisionersdk/proto"
 )
 
 func TestMain(m *testing.M) {
@@ -47,13 +49,32 @@ func TestAuthorizeAllEndpoints(t *testing.T) {
 	require.NoError(t, err, "fetch org")
 
 	// Setup some data in the database.
-	version := coderdtest.CreateTemplateVersion(t, client, admin.OrganizationID, nil)
+	version := coderdtest.CreateTemplateVersion(t, client, admin.OrganizationID, &echo.Responses{
+		Parse: echo.ParseComplete,
+		Provision: []*proto.Provision_Response{{
+			Type: &proto.Provision_Response_Complete{
+				Complete: &proto.Provision_Complete{
+					// Return a workspace resource
+					Resources: []*proto.Resource{{
+						Name: "some",
+						Type: "example",
+						Agents: []*proto.Agent{{
+							Id:   "something",
+							Auth: &proto.Agent_Token{},
+						}},
+					}},
+				},
+			},
+		}},
+	})
 	coderdtest.AwaitTemplateVersionJob(t, client, version.ID)
 	template := coderdtest.CreateTemplate(t, client, admin.OrganizationID, version.ID)
 	workspace := coderdtest.CreateWorkspace(t, client, admin.OrganizationID, template.ID)
 	coderdtest.AwaitWorkspaceBuildJob(t, client, workspace.LatestBuild.ID)
 	file, err := client.Upload(ctx, codersdk.ContentTypeTar, make([]byte, 1024))
 	require.NoError(t, err, "upload file")
+	workspaceResources, err := client.WorkspaceResourcesByBuild(ctx, workspace.LatestBuild.ID)
+	require.NoError(t, err, "workspace resources")
 
 	// Always fail auth from this point forward
 	authorizer.AlwaysReturn = rbac.ForbiddenWithInternal(xerrors.New("fake implementation"), nil, nil)
@@ -78,6 +99,9 @@ func TestAuthorizeAllEndpoints(t *testing.T) {
 		"POST:/api/v2/users/logout":     {NoAuthorize: true},
 		"GET:/api/v2/users/authmethods": {NoAuthorize: true},
 
+		// Has it's own auth
+		"GET:/api/v2/users/oauth2/github/callback": {NoAuthorize: true},
+
 		// All workspaceagents endpoints do not use rbac
 		"POST:/api/v2/workspaceagents/aws-instance-identity":      {NoAuthorize: true},
 		"POST:/api/v2/workspaceagents/azure-instance-identity":    {NoAuthorize: true},
@@ -94,11 +118,6 @@ func TestAuthorizeAllEndpoints(t *testing.T) {
 		"GET:/api/v2/workspaceagents/{workspaceagent}/turn":       {NoAuthorize: true},
 
 		// TODO: @emyrk these need to be fixed by adding authorize calls
-		"GET:/api/v2/workspaceresources/{workspaceresource}": {NoAuthorize: true},
-
-		"GET:/api/v2/users/oauth2/github/callback": {NoAuthorize: true},
-
-		"PUT:/api/v2/organizations/{organization}/members/{user}/roles":     {NoAuthorize: true},
 		"GET:/api/v2/organizations/{organization}/provisionerdaemons":       {NoAuthorize: true},
 		"GET:/api/v2/organizations/{organization}/templates/{templatename}": {NoAuthorize: true},
 		"POST:/api/v2/organizations/{organization}/templateversions":        {NoAuthorize: true},
@@ -108,17 +127,6 @@ func TestAuthorizeAllEndpoints(t *testing.T) {
 		"GET:/api/v2/parameters/{scope}/{id}":           {NoAuthorize: true},
 		"DELETE:/api/v2/parameters/{scope}/{id}/{name}": {NoAuthorize: true},
 
-		"GET:/api/v2/templates/{template}/versions":                       {NoAuthorize: true},
-		"PATCH:/api/v2/templates/{template}/versions":                     {NoAuthorize: true},
-		"GET:/api/v2/templates/{template}/versions/{templateversionname}": {NoAuthorize: true},
-
-		"GET:/api/v2/templateversions/{templateversion}":            {NoAuthorize: true},
-		"PATCH:/api/v2/templateversions/{templateversion}/cancel":   {NoAuthorize: true},
-		"GET:/api/v2/templateversions/{templateversion}/logs":       {NoAuthorize: true},
-		"GET:/api/v2/templateversions/{templateversion}/parameters": {NoAuthorize: true},
-		"GET:/api/v2/templateversions/{templateversion}/resources":  {NoAuthorize: true},
-		"GET:/api/v2/templateversions/{templateversion}/schema":     {NoAuthorize: true},
-
 		"POST:/api/v2/users/{user}/organizations": {NoAuthorize: true},
 
 		"GET:/api/v2/workspaces/{workspace}/watch": {NoAuthorize: true},
@@ -164,6 +172,10 @@ func TestAuthorizeAllEndpoints(t *testing.T) {
 			AssertAction: rbac.ActionUpdate,
 			AssertObject: workspaceRBACObj,
 		},
+		"GET:/api/v2/workspaceresources/{workspaceresource}": {
+			AssertAction: rbac.ActionRead,
+			AssertObject: workspaceRBACObj,
+		},
 		"PATCH:/api/v2/workspacebuilds/{workspacebuild}/cancel": {
 			AssertAction: rbac.ActionUpdate,
 			AssertObject: workspaceRBACObj,
@@ -199,12 +211,51 @@ func TestAuthorizeAllEndpoints(t *testing.T) {
 			AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()),
 		},
 		"POST:/api/v2/files": {AssertAction: rbac.ActionCreate, AssertObject: rbac.ResourceFile},
-		"GET:/api/v2/files/{fileHash}": {AssertAction: rbac.ActionRead,
-			AssertObject: rbac.ResourceFile.WithOwner(admin.UserID.String()).WithID(file.Hash)},
+		"GET:/api/v2/files/{fileHash}": {
+			AssertAction: rbac.ActionRead,
+			AssertObject: rbac.ResourceFile.WithOwner(admin.UserID.String()).WithID(file.Hash),
+		},
+		"GET:/api/v2/templates/{template}/versions": {
+			AssertAction: rbac.ActionRead,
+			AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()),
+		},
+		"PATCH:/api/v2/templates/{template}/versions": {
+			AssertAction: rbac.ActionUpdate,
+			AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()),
+		},
+		"GET:/api/v2/templates/{template}/versions/{templateversionname}": {
+			AssertAction: rbac.ActionRead,
+			AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()),
+		},
+		"GET:/api/v2/templateversions/{templateversion}": {
+			AssertAction: rbac.ActionRead,
+			AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()),
+		},
+		"PATCH:/api/v2/templateversions/{templateversion}/cancel": {
+			AssertAction: rbac.ActionUpdate,
+			AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()),
+		},
+		"GET:/api/v2/templateversions/{templateversion}/logs": {
+			AssertAction: rbac.ActionRead,
+			AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()),
+		},
+		"GET:/api/v2/templateversions/{templateversion}/parameters": {
+			AssertAction: rbac.ActionRead,
+			AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()),
+		},
+		"GET:/api/v2/templateversions/{templateversion}/resources": {
+			AssertAction: rbac.ActionRead,
+			AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()),
+		},
+		"GET:/api/v2/templateversions/{templateversion}/schema": {
+			AssertAction: rbac.ActionRead,
+			AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()),
+		},
 
 		// These endpoints need payloads to get to the auth part. Payloads will be required
-		"PUT:/api/v2/users/{user}/roles":             {StatusCode: http.StatusBadRequest, NoAuthorize: true},
-		"POST:/api/v2/workspaces/{workspace}/builds": {StatusCode: http.StatusBadRequest, NoAuthorize: true},
+		"PUT:/api/v2/users/{user}/roles":                                {StatusCode: http.StatusBadRequest, NoAuthorize: true},
+		"PUT:/api/v2/organizations/{organization}/members/{user}/roles": {NoAuthorize: true},
+		"POST:/api/v2/workspaces/{workspace}/builds":                    {StatusCode: http.StatusBadRequest, NoAuthorize: true},
 	}
 
 	for k, v := range assertRoute {
@@ -240,6 +291,8 @@ func TestAuthorizeAllEndpoints(t *testing.T) {
 			route = strings.ReplaceAll(route, "{workspacebuildname}", workspace.LatestBuild.Name)
 			route = strings.ReplaceAll(route, "{template}", template.ID.String())
 			route = strings.ReplaceAll(route, "{hash}", file.Hash)
+			route = strings.ReplaceAll(route, "{workspaceresource}", workspaceResources[0].ID.String())
+			route = strings.ReplaceAll(route, "{templateversion}", version.ID.String())
 
 			resp, err := client.Request(context.Background(), method, route, nil)
 			require.NoError(t, err, "do req")

diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go
@@ -276,8 +276,7 @@ func CreateAnotherUser(t *testing.T, client *codersdk.Client, organizationID uui
 		for orgID, roles := range orgRoles {
 			organizationID, err := uuid.Parse(orgID)
 			require.NoError(t, err, fmt.Sprintf("parse org id %q", orgID))
-			// TODO: @Emyrk add the member to the organization if they do not already belong.
-			_, err = other.UpdateOrganizationMemberRoles(context.Background(), organizationID, user.ID.String(),
+			_, err = client.UpdateOrganizationMemberRoles(context.Background(), organizationID, user.ID.String(),
 				codersdk.UpdateRoles{Roles: append(roles, rbac.RoleOrgMember(organizationID))})
 			require.NoError(t, err, "update org membership roles")
 		}

diff --git a/coderd/database/modelmethods.go b/coderd/database/modelmethods.go
@@ -6,6 +6,11 @@ func (t Template) RBACObject() rbac.Object {
 	return rbac.ResourceTemplate.InOrg(t.OrganizationID).WithID(t.ID.String())
 }
 
+func (t TemplateVersion) RBACObject() rbac.Object {
+	// Just use the parent template resource for controlling versions
+	return rbac.ResourceTemplate.InOrg(t.OrganizationID).WithID(t.TemplateID.UUID.String())
+}
+
 func (w Workspace) RBACObject() rbac.Object {
 	return rbac.ResourceWorkspace.InOrg(w.OrganizationID).WithID(w.ID.String()).WithOwner(w.OwnerID.String())
 }

diff --git a/coderd/httpmw/organizationparam.go b/coderd/httpmw/organizationparam.go
@@ -28,12 +28,12 @@ func OrganizationParam(r *http.Request) database.Organization {
 func OrganizationMemberParam(r *http.Request) database.OrganizationMember {
 	organizationMember, ok := r.Context().Value(organizationMemberParamContextKey{}).(database.OrganizationMember)
 	if !ok {
-		panic("developer error: organization param middleware not provided")
+		panic("developer error: organization member param middleware not provided")
 	}
 	return organizationMember
 }
 
-// ExtractOrganizationParam grabs an organization and user membership from the "organization" URL parameter.
+// ExtractOrganizationParam grabs an organization from the "organization" URL parameter.
 // This middleware requires the API key middleware higher in the call stack for authentication.
 func ExtractOrganizationParam(db database.Store) func(http.Handler) http.Handler {
 	return func(next http.Handler) http.Handler {
@@ -56,11 +56,23 @@ func ExtractOrganizationParam(db database.Store) func(http.Handler) http.Handler
 				})
 				return
 			}
+			ctx := context.WithValue(r.Context(), organizationParamContextKey{}, organization)
+			next.ServeHTTP(rw, r.WithContext(ctx))
+		})
+	}
+}
+
+// ExtractOrganizationMemberParam grabs a user membership from the "organization" and "user" URL parameter.
+// This middleware requires the ExtractUser and ExtractOrganization middleware higher in the stack
+func ExtractOrganizationMemberParam(db database.Store) func(http.Handler) http.Handler {
+	return func(next http.Handler) http.Handler {
+		return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
+			organization := OrganizationParam(r)
+			user := UserParam(r)
 
-			apiKey := APIKey(r)
 			organizationMember, err := db.GetOrganizationMemberByUserID(r.Context(), database.GetOrganizationMemberByUserIDParams{
 				OrganizationID: organization.ID,
-				UserID:         apiKey.UserID,
+				UserID:         user.ID,
 			})
 			if errors.Is(err, sql.ErrNoRows) {
 				httpapi.Write(rw, http.StatusForbidden, httpapi.Response{
@@ -74,9 +86,8 @@ func ExtractOrganizationParam(db database.Store) func(http.Handler) http.Handler
 				})
 				return
 			}
+			ctx := context.WithValue(r.Context(), organizationMemberParamContextKey{}, organizationMember)
 
-			ctx := context.WithValue(r.Context(), organizationParamContextKey{}, organization)
-			ctx = context.WithValue(ctx, organizationMemberParamContextKey{}, organizationMember)
 			next.ServeHTTP(rw, r.WithContext(ctx))
 		})
 	}

diff --git a/coderd/httpmw/organizationparam_test.go b/coderd/httpmw/organizationparam_test.go
@@ -122,7 +122,7 @@ func TestOrganizationParam(t *testing.T) {
 		var (
 			db   = databasefake.New()
 			rw   = httptest.NewRecorder()
-			r, _ = setupAuthentication(db)
+			r, u = setupAuthentication(db)
 			rtr  = chi.NewRouter()
 		)
 		organization, err := db.InsertOrganization(r.Context(), database.InsertOrganizationParams{
@@ -133,9 +133,12 @@ func TestOrganizationParam(t *testing.T) {
 		})
 		require.NoError(t, err)
 		chi.RouteContext(r.Context()).URLParams.Add("organization", organization.ID.String())
+		chi.RouteContext(r.Context()).URLParams.Add("user", u.ID.String())
 		rtr.Use(
 			httpmw.ExtractAPIKey(db, nil),
+			httpmw.ExtractUserParam(db),
 			httpmw.ExtractOrganizationParam(db),
+			httpmw.ExtractOrganizationMemberParam(db),
 		)
 		rtr.Get("/", nil)
 		rtr.ServeHTTP(rw, r)
@@ -167,9 +170,12 @@ func TestOrganizationParam(t *testing.T) {
 		})
 		require.NoError(t, err)
 		chi.RouteContext(r.Context()).URLParams.Add("organization", organization.ID.String())
+		chi.RouteContext(r.Context()).URLParams.Add("user", user.ID.String())
 		rtr.Use(
 			httpmw.ExtractAPIKey(db, nil),
 			httpmw.ExtractOrganizationParam(db),
+			httpmw.ExtractUserParam(db),
+			httpmw.ExtractOrganizationMemberParam(db),
 		)
 		rtr.Get("/", func(rw http.ResponseWriter, r *http.Request) {
 			_ = httpmw.OrganizationParam(r)

diff --git a/coderd/members.go b/coderd/members.go
@@ -17,27 +17,29 @@ import (
 )
 
 func (api *api) putMemberRoles(rw http.ResponseWriter, r *http.Request) {
-	// User is the user to modify
-	// TODO: Until rbac authorize is implemented, only be able to change your
-	//	own roles. This also means you can grant yourself whatever roles you want.
 	user := httpmw.UserParam(r)
-	apiKey := httpmw.APIKey(r)
 	organization := httpmw.OrganizationParam(r)
-	// TODO: @emyrk add proper `Authorize()` check here instead of a uuid match.
-	//	Proper authorize should check the granted roles are able to given within
-	//	the selected organization. Until then, allow anarchy
-	if apiKey.UserID != user.ID {
-		httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{
-			Message: "modifying other users is not supported at this time",
-		})
-		return
-	}
+	member := httpmw.OrganizationMemberParam(r)
 
 	var params codersdk.UpdateRoles
 	if !httpapi.Read(rw, r, &params) {
 		return
 	}
 
+	added, removed := rbac.ChangeRoleSet(member.Roles, params.Roles)
+	for _, roleName := range added {
+		// Assigning a role requires the create permission.
+		if !api.Authorize(rw, r, rbac.ActionCreate, rbac.ResourceOrgRoleAssignment.WithID(roleName).InOrg(organization.ID)) {
+			return
+		}
+	}
+	for _, roleName := range removed {
+		// Removing a role requires the delete permission.
+		if !api.Authorize(rw, r, rbac.ActionDelete, rbac.ResourceOrgRoleAssignment.WithID(roleName).InOrg(organization.ID)) {
+			return
+		}
+	}
+
 	updatedUser, err := api.updateOrganizationMemberRoles(r.Context(), database.UpdateMemberRolesParams{
 		GrantedRoles: params.Roles,
 		UserID:       user.ID,

diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go
@@ -3,6 +3,7 @@ package rbac
 import (
 	"context"
 	_ "embed"
+
 	"golang.org/x/xerrors"
 
 	"github.com/open-policy-agent/opa/rego"

diff --git a/coderd/rbac/builtin.go b/coderd/rbac/builtin.go
@@ -135,6 +135,12 @@ var (
 							Action:       ActionRead,
 							ResourceID:   "*",
 						},
+						{
+							// Can read available roles.
+							ResourceType: ResourceOrgRoleAssignment.Type,
+							ResourceID:   "*",
+							Action:       ActionRead,
+						},
 					},
 				},
 			}
@@ -217,6 +223,37 @@ func SiteRoles() []Role {
 	return roles
 }
 
+// ChangeRoleSet is a helper function that finds the difference of 2 sets of
+// roles. When setting a user's new roles, it is equivalent to adding and
+// removing roles. This set determines the changes, so that the appropriate
+// RBAC checks can be applied using "ActionCreate" and "ActionDelete" for
+// "added" and "removed" roles respectively.
+func ChangeRoleSet(from []string, to []string) (added []string, removed []string) {
+	has := make(map[string]struct{})
+	for _, exists := range from {
+		has[exists] = struct{}{}
+	}
+
+	for _, roleName := range to {
+		// If the user already has the role assigned, we don't need to check the permission
+		// to reassign it. Only run permission checks on the difference in the set of
+		// roles.
+		if _, ok := has[roleName]; ok {
+			delete(has, roleName)
+			continue
+		}
+
+		added = append(added, roleName)
+	}
+
+	// Remaining roles are the ones removed/deleted.
+	for roleName := range has {
+		removed = append(removed, roleName)
+	}
+
+	return added, removed
+}
+
 // roleName is a quick helper function to return
 // 	role_name:scopeID
 // If no scopeID is required, only 'role_name' is returned