Skip to content

feat: Rbac more coderd endpoints, unit test to confirm #1406

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

Closed
wants to merge 13 commits into from
Closed

feat: Rbac more coderd endpoints, unit test to confirm #1406

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
bed0f8f
feat: Enforce authorize call on all endpoints
Emyrk May 11, 2022
af6dc5f
Add more endpoints to the unit test
Emyrk May 12, 2022
01b2c94
Merge remote-tracking branch 'origin/main' into stevenmasley/rbac_end…
Emyrk May 12, 2022
be5b0b3
Rbac users endpoints
Emyrk May 12, 2022
970e345
Make test pass by skipping missed endpoints
Emyrk May 12, 2022
945e9fa
Fix broken tests
Emyrk May 12, 2022
fdfef88
Import order
Emyrk May 12, 2022
89a3678
PR comment fixes
Emyrk May 12, 2022
29da9aa
Merge remote-tracking branch 'origin/main' into stevenmasley/rbac_end…
Emyrk May 13, 2022
63727e0
omit another endpoint
Emyrk May 13, 2022
96a5727
Cleanup comments
Emyrk May 13, 2022
4b6c9b0
Do not leak if an organization name exists
Emyrk May 13, 2022
cd2fda7
Update comment
Emyrk May 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Make test pass by skipping missed endpoints
  • Loading branch information
@Emyrk
Emyrk committed May 12, 2022
commit 970e34539fa3c26e7cee1f38bb8a1a3785d02dcd
4 changes: 2 additions & 2 deletions coderd/coderd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ func New(options *Options) (http.Handler, func()) {
)
r.Group(func(r chi.Router) {
// Site wide, all users
r.Use(httpmw.WithRBACObject(rbac.ResourceUser))
r.Use(httpmw.WithRBACObject(rbac.ResourceUser.All()))
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't rbac.ResourceUser.All() identical to rbac.ResourceUser due to zero values?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea it is. I added that when walking through the code to illustrate what it's doing. I forgot to remove it.

r.Post("/", authorize(api.postUser, rbac.ActionCreate))
r.Get("/", authorize(api.users, rbac.ActionRead))
})
Expand Down Expand Up @@ -274,6 +274,7 @@ func New(options *Options) (http.Handler, func()) {
})

r.With(httpmw.WithRBACObject(rbac.ResourceAPIKey)).Post("/keys", authorize(api.postAPIKey, rbac.ActionCreate))
r.Get("/workspaces", api.workspacesByUser)

r.Route("/organizations", func(r chi.Router) {
// TODO: @emyrk This creates an organization, so why is it nested under {user}?
Expand All @@ -286,7 +287,6 @@ func New(options *Options) (http.Handler, func()) {
// TODO: @emyrk why is this nested under {user} when the user param is not used?
r.Get("/{organizationname}", api.organizationByUserAndName)
})
r.Get("/workspaces", api.workspacesByUser)
})
})
})
Expand Down
72 changes: 65 additions & 7 deletions coderd/coderd_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 +39,20 @@ func TestAuthorizeAllEndpoints(t *testing.T) {
Authorizer: authorizer,
})
admin := coderdtest.CreateFirstUser(t, client)
var _ = admin
organization, err := client.Organization(context.Background(), admin.OrganizationID)
require.NoError(t, err, "fetch org")

// Always fail auth from this point forward
authorizer.AlwaysReturn = rbac.ForbiddenWithInternal(fmt.Errorf("fake implementation"), nil, nil)

// skipRoutes allows skipping routes from being checked.
type routeCheck struct {
NoAuthorize bool
AssertObject rbac.Object
StatusCode int
}
assertRoute := map[string]routeCheck{
// These endpoints do not require auth
"GET:/api/v2": {NoAuthorize: true},
"GET:/api/v2/buildinfo": {NoAuthorize: true},
"GET:/api/v2/users/first": {NoAuthorize: true},
Expand All @@ -72,12 +78,59 @@ func TestAuthorizeAllEndpoints(t *testing.T) {
"GET:/api/v2/workspaceagents/{workspaceagent}/turn": {NoAuthorize: true},

// TODO: @emyrk these need to be fixed by adding authorize calls
"/api/v2/organizations/{organization}/provisionerdaemons": {NoAuthorize: true},
"GET:/api/v2/organizations/{organization}": {AssertObject: rbac.ResourceOrganization.InOrg(admin.OrganizationID)},
"GET:/api/v2/workspaceresources/{workspaceresource}": {NoAuthorize: true},
"GET:/api/v2/workspacebuilds/{workspacebuild}": {NoAuthorize: true},
"GET:/api/v2/workspacebuilds/{workspacebuild}/logs": {NoAuthorize: true},
"GET:/api/v2/workspacebuilds/{workspacebuild}/resources": {NoAuthorize: true},
"GET:/api/v2/workspacebuilds/{workspacebuild}/state": {NoAuthorize: true},
"PATCH:/api/v2/workspacebuilds/{workspacebuild}/cancel": {NoAuthorize: true},
"GET:/api/v2/workspaces/{workspace}/builds/{workspacebuildname}": {NoAuthorize: true},

"GET:/api/v2/users/oauth2/github/callback": {NoAuthorize: true},

"POST:/api/v2/users/{user}/organizations/": {NoAuthorize: true},
"PUT:/api/v2/organizations/{organization}/members/{user}/roles": {NoAuthorize: true},
"GET:/api/v2/organizations/{organization}/provisionerdaemons": {NoAuthorize: true},
"POST:/api/v2/organizations/{organization}/templates": {NoAuthorize: true},
"GET:/api/v2/organizations/{organization}/templates": {NoAuthorize: true},
"GET:/api/v2/organizations/{organization}/templates/{templatename}": {NoAuthorize: true},
"POST:/api/v2/organizations/{organization}/templateversions": {NoAuthorize: true},

"POST:/api/v2/parameters/{scope}/{id}": {NoAuthorize: true},
"GET:/api/v2/parameters/{scope}/{id}": {NoAuthorize: true},
"DELETE:/api/v2/parameters/{scope}/{id}/{name}": {NoAuthorize: true},

"GET:/api/v2/provisionerdaemons/me/listen": {NoAuthorize: true},

"DELETE:/api/v2/templates/{template}": {NoAuthorize: true},
"GET:/api/v2/templates/{template}": {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}": {NoAuthorize: true},
"PUT:/api/v2/workspaces/{workspace}/autostart": {NoAuthorize: true},
"PUT:/api/v2/workspaces/{workspace}/autostop": {NoAuthorize: true},
"GET:/api/v2/workspaces/{workspace}/builds": {NoAuthorize: true},
"POST:/api/v2/workspaces/{workspace}/builds": {NoAuthorize: true},

// These endpoints have more assertions. This is good, add more endpoints to assert if you can!
"GET:/api/v2/organizations/{organization}": {AssertObject: rbac.ResourceOrganization.InOrg(admin.OrganizationID)},
"GET:/api/v2/users/{user}/organizations": {StatusCode: http.StatusOK, AssertObject: rbac.ResourceOrganization},
"GET:/api/v2/users/{user}/workspaces": {StatusCode: http.StatusOK, AssertObject: rbac.ResourceWorkspace},
}

c := srv.Config.Handler.(*chi.Mux)
err := chi.Walk(c, func(method string, route string, handler http.Handler, middlewares ...func(http.Handler) http.Handler) error {
err = chi.Walk(c, func(method string, route string, handler http.Handler, middlewares ...func(http.Handler) http.Handler) error {
name := method + ":" + route
t.Run(name, func(t *testing.T) {
authorizer.reset()
Expand All @@ -86,18 +139,22 @@ func TestAuthorizeAllEndpoints(t *testing.T) {
// By default, all omitted routes check for just "authorize" called
routeAssertions = routeCheck{}
}
if routeAssertions.StatusCode == 0 {
routeAssertions.StatusCode = http.StatusUnauthorized
}

// Replace all url params with known values
route = strings.ReplaceAll(route, "{organization}", admin.OrganizationID.String())
route = strings.ReplaceAll(route, "{user}", admin.UserID.String())
route = strings.ReplaceAll(route, "{organizationname}", organization.Name)

resp, err := client.Request(context.Background(), method, route, nil)
require.NoError(t, err, "do req")
_ = resp.Body.Close()

if !routeAssertions.NoAuthorize {
assert.NotNil(t, authorizer.Called, "authorizer expected")
assert.Equal(t, http.StatusUnauthorized, resp.StatusCode, "expect unauthorized")
assert.Equal(t, routeAssertions.StatusCode, resp.StatusCode, "expect unauthorized")
if routeAssertions.AssertObject.Type != "" {
assert.Equal(t, routeAssertions.AssertObject.Type, authorizer.Called.Object.Type, "resource type")
}
Expand Down Expand Up @@ -127,7 +184,8 @@ type authCall struct {
}

type fakeAuthorizer struct {
Called *authCall
Called *authCall
AlwaysReturn error
}

func (f *fakeAuthorizer) AuthorizeByRoleName(ctx context.Context, subjectID string, roleNames []string, action rbac.Action, object rbac.Object) error {
Expand All @@ -137,7 +195,7 @@ func (f *fakeAuthorizer) AuthorizeByRoleName(ctx context.Context, subjectID stri
Action: action,
Object: object,
}
return rbac.ForbiddenWithInternal(fmt.Errorf("fake implementation"), nil, nil)
return f.AlwaysReturn
}

func (f *fakeAuthorizer) reset() {
Expand Down
1 change: 1 addition & 0 deletions coderd/users.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -811,6 +811,7 @@ func (api *api) createUser(ctx context.Context, req codersdk.CreateUserRequest)
})
}

//
func (api *api) workspacesByUser(rw http.ResponseWriter, r *http.Request) {
user := httpmw.UserParam(r)
roles := httpmw.UserRoles(r)
Expand Down