Skip to content

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

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

Merged
merged 30 commits into from
May 17, 2022
Merged

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

Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
30 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
62ec87e
feat: Implement authorize for each endpoint
Emyrk May 13, 2022
452c72d
Authorize per endpoint
Emyrk May 13, 2022
307f6c0
Merge remote-tracking branch 'origin/main' into stevenmasley/rbac_end…
Emyrk May 16, 2022
32af1e6
feat: Move all authorize calls into each handler
Emyrk May 16, 2022
28a099f
Delete unused code
Emyrk May 16, 2022
ff7bd81
feat: Add some perms to users
Emyrk May 16, 2022
d123b9f
Drop comment
Emyrk May 16, 2022
186eb5f
Fix 401 -> 403
Emyrk May 16, 2022
5d32d9d
Fix using User over UserData
Emyrk May 16, 2022
301d42a
Rename UserRole to RoleAssignment
Emyrk May 16, 2022
a989224
Refactor workspacesByUser
Emyrk May 16, 2022
ed9be78
Merge remote-tracking branch 'origin/main' into stevenmasley/rbac_end…
Emyrk May 16, 2022
1047391
Fix some routes
Emyrk May 16, 2022
7ad069e
Drop update User auth check from assign roles
Emyrk May 16, 2022
e68fdbf
Correct unit tests
Emyrk May 17, 2022
1a4e7e1
Unit tests use 403 vs 401
Emyrk May 17, 2022
f250fbe
401 -> 403 in unit test
Emyrk May 17, 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
42 changes: 42 additions & 0 deletions coderd/authorize.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
package coderd

import (
"net/http"

"cdr.dev/slog"
"golang.org/x/xerrors"

"github.com/coder/coder/coderd/httpapi"
"github.com/coder/coder/coderd/httpmw"
"github.com/coder/coder/coderd/rbac"
)

func (api *api) Authorize(rw http.ResponseWriter, r *http.Request, action rbac.Action, object rbac.Object) bool {
roles := httpmw.UserRoles(r)
err := api.Authorizer.ByRoleName(r.Context(), roles.ID.String(), roles.Roles, action, object)
if err != nil {
httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{
Message: err.Error(),
})

// Log the errors for debugging
internalError := new(rbac.UnauthorizedError)
Copy link
Member

Choose a reason for hiding this comment

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

minor: should we just define a single var rbac.UnauthorizedError and use that instead in the check below?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure what you mean?

logger := api.Logger
if xerrors.As(err, internalError) {
logger = api.Logger.With(slog.F("internal", internalError.Internal()))
}
// Log information for debugging. This will be very helpful
// in the early days
logger.Warn(r.Context(), "unauthorized",
slog.F("roles", roles.Roles),
slog.F("user_id", roles.ID),
slog.F("username", roles.Username),
slog.F("route", r.URL.Path),
slog.F("action", action),
slog.F("object", object),
)

return false
}
return true
}
17 changes: 4 additions & 13 deletions coderd/coderd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ type Options struct {
SecureAuthCookie bool
SSHKeygenAlgorithm gitsshkey.Algorithm
TURNServer *turnconn.Server
Authorizer *rbac.RegoAuthorizer
Authorizer rbac.Authorizer
}

// New constructs the Coder API into an HTTP handler.
Expand Down Expand Up @@ -83,10 +83,6 @@ func New(options *Options) (http.Handler, func()) {
// TODO: @emyrk we should just move this into 'ExtractAPIKey'.
authRolesMiddleware := httpmw.ExtractUserRoles(options.Database)

authorize := func(f http.HandlerFunc, actions rbac.Action) http.HandlerFunc {
return httpmw.Authorize(api.Logger, api.Authorizer, actions)(f).ServeHTTP
}

r := chi.NewRouter()

r.Use(
Expand Down Expand Up @@ -158,10 +154,7 @@ func New(options *Options) (http.Handler, func()) {
})
})
r.Route("/members", func(r chi.Router) {
r.Route("/roles", func(r chi.Router) {
r.Use(httpmw.WithRBACObject(rbac.ResourceUserRole))
r.Get("/", authorize(api.assignableOrgRoles, rbac.ActionRead))
})
r.Get("/roles", api.assignableOrgRoles)
r.Route("/{user}", func(r chi.Router) {
r.Use(
httpmw.ExtractUserParam(options.Database),
Expand Down Expand Up @@ -232,17 +225,15 @@ func New(options *Options) (http.Handler, func()) {
r.Get("/", api.users)
// These routes query information about site wide roles.
r.Route("/roles", func(r chi.Router) {
r.Use(httpmw.WithRBACObject(rbac.ResourceUserRole))
r.Get("/", authorize(api.assignableSiteRoles, rbac.ActionRead))
r.Get("/", api.assignableSiteRoles)
})
r.Route("/{user}", func(r chi.Router) {
r.Use(httpmw.ExtractUserParam(options.Database))
r.Get("/", api.userByName)
r.Put("/profile", api.putUserProfile)
r.Put("/suspend", api.putUserSuspend)
r.Route("/password", func(r chi.Router) {
r.Use(httpmw.WithRBACObject(rbac.ResourceUserPasswordRole))
r.Put("/", authorize(api.putUserPassword, rbac.ActionUpdate))
r.Put("/", api.putUserPassword)
})
r.Get("/organizations", api.organizationsByUser)
r.Post("/organizations", api.postOrganizationsByUser)
Expand Down
195 changes: 193 additions & 2 deletions coderd/coderd_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,19 @@ package coderd_test

import (
"context"
"net/http"
"strings"
"testing"

"go.uber.org/goleak"

"github.com/go-chi/chi/v5"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.uber.org/goleak"
"golang.org/x/xerrors"

"github.com/coder/coder/buildinfo"
"github.com/coder/coder/coderd/coderdtest"
"github.com/coder/coder/coderd/rbac"
)

func TestMain(m *testing.M) {
Expand All @@ -24,3 +29,189 @@ func TestBuildInfo(t *testing.T) {
require.Equal(t, buildinfo.ExternalURL(), buildInfo.ExternalURL, "external URL")
require.Equal(t, buildinfo.Version(), buildInfo.Version, "version")
}

// TestAuthorizeAllEndpoints will check `authorize` is called on every endpoint registered.
func TestAuthorizeAllEndpoints(t *testing.T) {
t.Parallel()

authorizer := &fakeAuthorizer{}
srv, client := coderdtest.NewMemoryCoderd(t, &coderdtest.Options{
Authorizer: authorizer,
})
admin := coderdtest.CreateFirstUser(t, client)
organization, err := client.Organization(context.Background(), admin.OrganizationID)
require.NoError(t, err, "fetch org")

// Setup some data in the database.
coderdtest.NewProvisionerDaemon(t, client)
version := coderdtest.CreateTemplateVersion(t, client, admin.OrganizationID, nil)
coderdtest.AwaitTemplateVersionJob(t, client, version.ID)
template := coderdtest.CreateTemplate(t, client, admin.OrganizationID, version.ID)
coderdtest.CreateWorkspace(t, client, admin.OrganizationID, template.ID)

// Always fail auth from this point forward
authorizer.AlwaysReturn = rbac.ForbiddenWithInternal(xerrors.New("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},
"POST:/api/v2/users/first": {NoAuthorize: true},
"POST:/api/v2/users/login": {NoAuthorize: true},
"POST:/api/v2/users/logout": {NoAuthorize: true},
"GET:/api/v2/users/authmethods": {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},
"POST:/api/v2/workspaceagents/google-instance-identity": {NoAuthorize: true},
"GET:/api/v2/workspaceagents/me/gitsshkey": {NoAuthorize: true},
"GET:/api/v2/workspaceagents/me/iceservers": {NoAuthorize: true},
"GET:/api/v2/workspaceagents/me/listen": {NoAuthorize: true},
"GET:/api/v2/workspaceagents/me/metadata": {NoAuthorize: true},
"GET:/api/v2/workspaceagents/me/turn": {NoAuthorize: true},
"GET:/api/v2/workspaceagents/{workspaceagent}": {NoAuthorize: true},
"GET:/api/v2/workspaceagents/{workspaceagent}/": {NoAuthorize: true},
"GET:/api/v2/workspaceagents/{workspaceagent}/dial": {NoAuthorize: true},
"GET:/api/v2/workspaceagents/{workspaceagent}/iceservers": {NoAuthorize: true},
"GET:/api/v2/workspaceagents/{workspaceagent}/pty": {NoAuthorize: true},
"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/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/organizations/{organization}/workspaces": {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},

"POST:/api/v2/files": {NoAuthorize: true},
"GET:/api/v2/files/{hash}": {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},
"GET:/api/v2/organizations/{organization}/workspaces/{user}": {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 {
name := method + ":" + route
t.Run(name, func(t *testing.T) {
authorizer.reset()
routeAssertions, ok := assertRoute[strings.TrimRight(name, "/")]
if !ok {
// 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, routeAssertions.StatusCode, resp.StatusCode, "expect unauthorized")
if authorizer.Called != nil {
if routeAssertions.AssertObject.Type != "" {
assert.Equal(t, routeAssertions.AssertObject.Type, authorizer.Called.Object.Type, "resource type")
}
if routeAssertions.AssertObject.Owner != "" {
assert.Equal(t, routeAssertions.AssertObject.Owner, authorizer.Called.Object.Owner, "resource owner")
}
if routeAssertions.AssertObject.OrgID != "" {
assert.Equal(t, routeAssertions.AssertObject.OrgID, authorizer.Called.Object.OrgID, "resource org")
}
if routeAssertions.AssertObject.ResourceID != "" {
assert.Equal(t, routeAssertions.AssertObject.ResourceID, authorizer.Called.Object.ResourceID, "resource ID")
}
}
} else {
assert.Nil(t, authorizer.Called, "authorize not expected")
}
})
return nil
})
require.NoError(t, err)
}

type authCall struct {
SubjectID string
Roles []string
Action rbac.Action
Object rbac.Object
}

type fakeAuthorizer struct {
Called *authCall
Copy link
Contributor

Choose a reason for hiding this comment

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

you only capture the most recent authorization call, but many endpoints call this multiple times with different objects. This should be captured and asserted in testing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes this is true. Most cases though the first call returns an error and the request terminates.

As I add more endpoints and see more cases, I should increase the assertions. I intend to follow this PR up with more testing and endpoints. I just want to be on the right path first.

AlwaysReturn error
}

func (f *fakeAuthorizer) ByRoleName(_ context.Context, subjectID string, roleNames []string, action rbac.Action, object rbac.Object) error {
f.Called = &authCall{
SubjectID: subjectID,
Roles: roleNames,
Action: action,
Object: object,
}
return f.AlwaysReturn
}

func (f *fakeAuthorizer) reset() {
f.Called = nil
}
18 changes: 13 additions & 5 deletions coderd/coderdtest/coderdtest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ import (

type Options struct {
AWSCertificates awsidentity.Certificates
Authorizer rbac.Authorizer
AzureCertificates x509.VerifyOptions
GithubOAuth2Config *coderd.GithubOAuth2Config
GoogleTokenValidator *idtoken.Validator
Expand All @@ -66,7 +67,7 @@ type Options struct {

// New constructs an in-memory coderd instance and returns
// the connected client.
func New(t *testing.T, options *Options) *codersdk.Client {
func NewMemoryCoderd(t *testing.T, options *Options) (*httptest.Server, *codersdk.Client) {
if options == nil {
options = &Options{}
}
Expand Down Expand Up @@ -147,6 +148,7 @@ func New(t *testing.T, options *Options) *codersdk.Client {
SSHKeygenAlgorithm: options.SSHKeygenAlgorithm,
TURNServer: turnServer,
APIRateLimit: options.APIRateLimit,
Authorizer: options.Authorizer,
})
t.Cleanup(func() {
cancelFunc()
Expand All @@ -155,7 +157,14 @@ func New(t *testing.T, options *Options) *codersdk.Client {
closeWait()
})

return codersdk.New(serverURL)
return srv, codersdk.New(serverURL)
}

// New constructs an in-memory coderd instance and returns
// the connected client.
func New(t *testing.T, options *Options) *codersdk.Client {
_, cli := NewMemoryCoderd(t, options)
return cli
}

// NewProvisionerDaemon launches a provisionerd instance configured to work
Expand Down Expand Up @@ -252,9 +261,8 @@ func CreateAnotherUser(t *testing.T, client *codersdk.Client, organizationID uui
for _, r := range user.Roles {
siteRoles = append(siteRoles, r.Name)
}
// TODO: @emyrk switch "other" to "client" when we support updating other
// users.
_, err := other.UpdateUserRoles(context.Background(), user.ID, codersdk.UpdateRoles{Roles: siteRoles})

_, err := client.UpdateUserRoles(context.Background(), user.ID, codersdk.UpdateRoles{Roles: siteRoles})
require.NoError(t, err, "update site roles")

// Update org roles
Expand Down
11 changes: 11 additions & 0 deletions coderd/gitsshkey.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,17 @@ import (
"github.com/coder/coder/coderd/gitsshkey"
"github.com/coder/coder/coderd/httpapi"
"github.com/coder/coder/coderd/httpmw"
"github.com/coder/coder/coderd/rbac"
"github.com/coder/coder/codersdk"
)

func (api *api) regenerateGitSSHKey(rw http.ResponseWriter, r *http.Request) {
user := httpmw.UserParam(r)

if !api.Authorize(rw, r, rbac.ActionUpdate, rbac.ResourceUserData.WithOwner(user.ID.String())) {
return
}

privateKey, publicKey, err := gitsshkey.Generate(api.SSHKeygenAlgorithm)
if err != nil {
httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
Expand Down Expand Up @@ -53,6 +59,11 @@ func (api *api) regenerateGitSSHKey(rw http.ResponseWriter, r *http.Request) {

func (api *api) gitSSHKey(rw http.ResponseWriter, r *http.Request) {
user := httpmw.UserParam(r)

if !api.Authorize(rw, r, rbac.ActionRead, rbac.ResourceUserData.WithOwner(user.ID.String())) {
return
}

gitSSHKey, err := api.Database.GetGitSSHKey(r.Context(), user.ID)
if err != nil {
httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
Expand Down
Loading