Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Next Next commit
feat: Check permissions endpoint
Allows FE to query backend for permission capabilities.
Batch requests supported
  • Loading branch information
Emyrk committed May 11, 2022
commit fb7adf5580ce6bf0d14055cadc098a0f37a41150
4 changes: 4 additions & 0 deletions coderd/coderd.go
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,10 @@ func New(options *Options) (http.Handler, func()) {
r.Put("/roles", api.putUserRoles)
r.Get("/roles", api.userRoles)

r.Route("/permissions", func(r chi.Router) {
r.Post("/check", api.checkPermissions)
})

r.Post("/keys", api.postAPIKey)
r.Route("/organizations", func(r chi.Router) {
r.Post("/", api.postOrganizationsByUser)
Expand Down
43 changes: 43 additions & 0 deletions coderd/roles.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,49 @@ func (*api) assignableOrgRoles(rw http.ResponseWriter, r *http.Request) {
httpapi.Write(rw, http.StatusOK, convertRoles(roles))
}

func (api *api) checkPermissions(rw http.ResponseWriter, r *http.Request) {
roles := httpmw.UserRoles(r)
user := httpmw.UserParam(r)
if user.ID != roles.ID {
httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{
// TODO: @Emyrk in the future we could have an rbac check here.
// If the user can masquerade/impersonate as the user passed in,
// we could allow this or something like that.
Message: "only allowed to check permissions on yourself",
})
return
}

var params codersdk.UserPermissionCheckRequest
if !httpapi.Read(rw, r, &params) {
return
}

response := make(codersdk.UserPermissionCheckResponse)
for k, v := range params.Checks {
Copy link
Member

Choose a reason for hiding this comment

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

It seems odd to echo user-specified keys back in the response. Do we require this endpoint to check for N permissions?

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. It's a batch request, so the map key is to return the right true/false for each permission check. So the keys will likely be human friendly names such as read-my-workspace, the response will return "read-my-workspace": true

Copy link
Member

Choose a reason for hiding this comment

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

I understand, but what is the use-case that attempting to fetch the workspace and checking the status code wouldn't catch?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is primarily for deciding which UI options to show.

Currently the admin Manage dropdown would use this to determine if it should show, and what buttons.

This is probably not going to be exactly how it's done, but something like this. The manage dropdown has 4 different options, you can do a permission check for each one and conditionally show the ui elements the user can access. If any return true, you show the Manage button.

{
   "manage-all-organizations": true,
   "manage-all-users": true,
   "read-all-auditlogs": true,
   "modify-deployment-settings: false,
}

In the past, the UI used the user's roles, but that means the UI has to know what an admin can and cannot do. In this world, if we ever support custom roles, that becomes impossible. This is the new approach, where the UI comes up with actions associated with UI elements, and queries the backend to handle the Authorize() check. Keeping it centralized in 1 place where RBAC logic is done.

Copy link
Member

Choose a reason for hiding this comment

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

Is this standard amongst other products with RBAC? Something feels fishy to me which isn't enough to prevent this, but maybe is sign to look for validation with other products.

GitHub appears to display UI options based off role, but I know that diverges from our abstraction a bit.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What I like most on not being attached to the role itself but the permission is in the future, if we want, we can create custom roles without having to update the front end at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this standard amongst other products with RBAC?

Kubernetes has SubjectAccessReview and SelfSubjectAccessReview API endpoints, for example: https://kubernetes.io/docs/reference/access-authn-authz/authorization/#checking-api-access

Copy link
Member Author

Choose a reason for hiding this comment

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

@spikecurtis good example that is exactly what we are doing here.

Copy link
Member Author

Choose a reason for hiding this comment

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

@kylecarbs What do you think now?

Copy link
Member

Choose a reason for hiding this comment

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

I still feel it's odd to echo the key-value pairs, but I don't think it should block progress! Seems fine to me.

if v.Object.ResourceType == "" {
httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{
Message: "'resource_type' must be defined",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is resource type required? It has a ? in the typescript

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is. @Emyrk is that possible to have this reflected in the TS types?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, fixed.Ah

})
return
}

if v.Object.OwnerID == "me" {
v.Object.OwnerID = roles.ID.String()
}
err := api.Authorizer.AuthorizeByRoleName(r.Context(), roles.ID.String(), roles.Roles, v.Action,
rbac.Object{
ResourceID: v.Object.ResourceID,
Owner: v.Object.OwnerID,
OrgID: v.Object.OrganizationID,
Type: v.Object.ResourceType,
})
response[k] = err == nil
}

httpapi.Write(rw, http.StatusOK, response)
}

func convertRole(role rbac.Role) codersdk.Role {
return codersdk.Role{
DisplayName: role.DisplayName,
Expand Down
97 changes: 97 additions & 0 deletions coderd/roles_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,103 @@ import (
"github.com/coder/coder/codersdk"
)

func TestPermissionCheck(t *testing.T) {
t.Parallel()

ctx := context.Background()
client := coderdtest.New(t, nil)
// Create admin, member, and org admin
admin := coderdtest.CreateFirstUser(t, client)
member := coderdtest.CreateAnotherUser(t, client, admin.OrganizationID)

orgAdmin := coderdtest.CreateAnotherUser(t, client, admin.OrganizationID)
orgAdminUser, err := orgAdmin.User(ctx, codersdk.Me)
require.NoError(t, err)

// TODO: @emyrk switch this to the admin when getting non-personal users is
// supported. `client.UpdateOrganizationMemberRoles(...)`
_, err = orgAdmin.UpdateOrganizationMemberRoles(ctx, admin.OrganizationID, orgAdminUser.ID,
codersdk.UpdateRoles{
Roles: []string{rbac.RoleOrgMember(admin.OrganizationID), rbac.RoleOrgAdmin(admin.OrganizationID)},
},
)
require.NoError(t, err, "update org member roles")

// With admin, member, and org admin
const (
allUsers = "read-all-users"
readOrgWorkspaces = "read-org-workspaces"
myself = "read-myself"
myWorkspace = "read-my-workspace"
)
params := map[string]codersdk.UserPermissionCheck{
allUsers: {
Object: codersdk.UserPermissionCheckObject{
ResourceType: "users",
},
Action: "read",
},
myself: {
Object: codersdk.UserPermissionCheckObject{
ResourceType: "users",
OwnerID: "me",
},
Action: "read",
},
myWorkspace: {
Object: codersdk.UserPermissionCheckObject{
ResourceType: "workspaces",
OwnerID: "me",
},
Action: "read",
},
readOrgWorkspaces: {
Object: codersdk.UserPermissionCheckObject{
ResourceType: "workspaces",
OrganizationID: admin.OrganizationID.String(),
},
Action: "read",
},
}

testCases := []struct {
Name string
Client *codersdk.Client
Check codersdk.UserPermissionCheckResponse
}{
{
Name: "Admin",
Client: client,
Check: map[string]bool{
allUsers: true, myself: true, myWorkspace: true, readOrgWorkspaces: true,
},
},
{
Name: "Member",
Client: member,
Check: map[string]bool{
allUsers: false, myself: true, myWorkspace: true, readOrgWorkspaces: false,
},
},
{
Name: "OrgAdmin",
Client: orgAdmin,
Check: map[string]bool{
allUsers: false, myself: true, myWorkspace: true, readOrgWorkspaces: true,
},
},
}

for _, c := range testCases {
c := c
t.Run(c.Name, func(t *testing.T) {
resp, err := c.Client.CheckPermissions(context.Background(), codersdk.UserPermissionCheckRequest{Checks: params})
require.NoError(t, err, "check perms")
require.Equal(t, resp, c.Check)
})
}
}

func TestListRoles(t *testing.T) {
t.Parallel()

Expand Down
13 changes: 13 additions & 0 deletions codersdk/roles.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,3 +43,16 @@ func (c *Client) ListOrganizationRoles(ctx context.Context, org uuid.UUID) ([]Ro
var roles []Role
return roles, json.NewDecoder(res.Body).Decode(&roles)
}

func (c *Client) CheckPermissions(ctx context.Context, checks UserPermissionCheckRequest) (UserPermissionCheckResponse, error) {
res, err := c.request(ctx, http.MethodPost, fmt.Sprintf("/api/v2/users/%s/permissions/check", uuidOrMe(Me)), checks)
if err != nil {
return nil, err
}
defer res.Body.Close()
if res.StatusCode != http.StatusOK {
return nil, readBodyAsError(res)
}
var roles UserPermissionCheckResponse
return roles, json.NewDecoder(res.Body).Decode(&roles)
}
23 changes: 23 additions & 0 deletions codersdk/users.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import (
"time"

"github.com/google/uuid"

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

// Me is used as a replacement for your own ID.
Expand Down Expand Up @@ -76,6 +78,27 @@ type UserRoles struct {
OrganizationRoles map[uuid.UUID][]string `json:"organization_roles"`
}

type UserPermissionCheckObject struct {
ResourceType string `json:"resource_type,omitempty"`
OwnerID string `json:"owner_id,omitempty"`
OrganizationID string `json:"organization_id,omitempty"`
ResourceID string `json:"resource_id,omitempty"`
}

type UserPermissionCheckResponse map[string]bool

// UserPermissionCheckRequest is a structure instead of a map because
// go-playground/validate can only validate structs. If you attempt to pass
// a map into 'httpapi.Read', you will get an invalid type error.
type UserPermissionCheckRequest struct {
Checks map[string]UserPermissionCheck `json:"checks"`
}

type UserPermissionCheck struct {
Object UserPermissionCheckObject `json:"object"`
Action rbac.Action `json:"action"`
}

// LoginWithPasswordRequest enables callers to authenticate with email and password.
type LoginWithPasswordRequest struct {
Email string `json:"email" validate:"required,email"`
Expand Down