Skip to content

feat: Implement list roles & enforce authorize examples #1273

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 16 commits into from
May 3, 2022
Merged

feat: Implement list roles & enforce authorize examples #1273

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
30e2031
feat: First draft of adding authorize to an http endpoint
Emyrk May 3, 2022
2161f84
WIP: Using middleware to change auth object params
Emyrk May 3, 2022
54bc054
feat: Implement basic authorize and unit test
Emyrk May 3, 2022
d083a7c
Some cleanup
Emyrk May 3, 2022
95b9a14
Merge remote-tracking branch 'origin/main' into stevenmasley/list_roles
Emyrk May 3, 2022
1498dcd
Expand 'orgs' to 'organizations' in func namings
Emyrk May 3, 2022
f36ae37
Renamings
Emyrk May 3, 2022
b831260
Use rbac.object directly
Emyrk May 3, 2022
db04d67
Fix broken tests
Emyrk May 3, 2022
b76f373
Add some comments
Emyrk May 3, 2022
117f838
Linting
Emyrk May 3, 2022
42b42ab
Handle out of order lists
Emyrk May 3, 2022
0efe72c
Add unit test
Emyrk May 3, 2022
dba617d
Add unit test for mw
Emyrk May 3, 2022
190940f
parallel unit test
Emyrk May 3, 2022
c86c67c
style order
Emyrk May 3, 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
Some cleanup
  • Loading branch information
@Emyrk
Emyrk committed May 3, 2022
commit d083a7c87ecf363c009f61ec2674dff192cf28f9
5 changes: 3 additions & 2 deletions coderd/coderd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ func New(options *Options) (http.Handler, func()) {
Github: options.GithubOAuth2Config,
})

// TODO: @emyrk we should just move this into 'ExtractAPIKey'.
authRolesMiddleware := httpmw.ExtractUserRoles(options.Database)

authorize := func(f http.HandlerFunc, actions rbac.Action) http.HandlerFunc {
Expand Down Expand Up @@ -142,7 +143,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.RBACObject(rbac.ResourceUserRole))
r.Get("/", authorize(api.assignableOrgRoles, rbac.ActionCreate))
r.Get("/", authorize(api.assignableOrgRoles, rbac.ActionRead))
})
r.Route("/{user}", func(r chi.Router) {
r.Use(
Expand Down Expand Up @@ -215,7 +216,7 @@ func New(options *Options) (http.Handler, func()) {
// These routes query information about site wide roles.
r.Route("/roles", func(r chi.Router) {
r.Use(httpmw.RBACObject(rbac.ResourceUserRole))
r.Get("/", authorize(api.assignableSiteRoles, rbac.ActionCreate))
r.Get("/", authorize(api.assignableSiteRoles, rbac.ActionRead))
})
r.Route("/{user}", func(r chi.Router) {
r.Use(httpmw.ExtractUserParam(options.Database))
Expand Down
16 changes: 12 additions & 4 deletions coderd/httpmw/authorize.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,21 @@ type AuthObject struct {
Object rbac.Object
}

// Authorize allows for static object & action authorize checking. If the object is a static object, this is an easy way
// to enforce the route.
// Authorize will enforce if the user roles can complete the action on the AuthObject.
// The organization and owner are found using the ExtractOrganization and
// ExtractUser middleware if present.
func Authorize(logger slog.Logger, auth *rbac.RegoAuthorizer, action rbac.Action) func(http.Handler) http.Handler {
Copy link
Member

Choose a reason for hiding this comment

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

Authorize feels like action, not something that would return a handler.

What do you think about renaming this to EnforceRBAC?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think EnforceRBAC is also weak though. I was thinking the package httpmw provides enough context, and it does do the action Authorize().

Authorize is the correct word for what is happening, as it's not authentication. I feel EnforceRBAC doesn't indicate the object and action are included.

Another word that comes to mind is "Access". Idk, EnforceAccess, EnforcePermissions. Maybe EnforceRBAC isn't that bad, just felt odd to me at first.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough. I'm primarily trying to display that the RBAC package is being leveraged when calling this handle. Enforce is a bit sketchy too.

While it is authorizing, I'm nervous that this will get conflated with authentication really easily.

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 this is classic authorization vs authentication. If you aren't familiar with it, it's easy to mix up.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed agreed

return func(next http.Handler) http.Handler {
return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
roles := UserRoles(r)
args := GetAuthObject(r)

object := args.Object
if object.Type == "" {
panic("developer error: auth object has no type")
}

// First extract the object's owner and organization if present.
unknownOrg := r.Context().Value(organizationParamContextKey{})
if organization, castOK := unknownOrg.(database.Organization); unknownOrg != nil {
if !castOK {
Expand All @@ -46,13 +51,14 @@ func Authorize(logger slog.Logger, auth *rbac.RegoAuthorizer, action rbac.Action
object = object.WithOwner(owner.ID.String())
}

// Error on the first action that fails
err := auth.AuthorizeByRoleName(r.Context(), roles.ID.String(), roles.Roles, action, object)
if err != nil {
internalError := new(rbac.UnauthorizedError)
if xerrors.As(err, internalError) {
logger = logger.With(slog.F("internal", internalError.Internal()))
}
// Log information for debugging. This will be very helpful
// in the early days if we over secure endpoints.
logger.Warn(r.Context(), "unauthorized",
slog.F("roles", roles.Roles),
slog.F("user_id", roles.ID),
Expand All @@ -77,11 +83,13 @@ type authObjectKey struct{}
func GetAuthObject(r *http.Request) AuthObject {
obj, ok := r.Context().Value(authObjectKey{}).(AuthObject)
if !ok {
return AuthObject{}
panic("developer error: auth object middleware not provided")
}
return obj
}

// RBACObject sets the object for 'Authorize()' for all routes handled
// by this middleware. The important field to set is 'Type'
func RBACObject(object rbac.Object) func(http.Handler) http.Handler {
return func(next http.Handler) http.Handler {
return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
Expand Down
1 change: 0 additions & 1 deletion coderd/httpmw/organizationparam.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,6 @@ func ExtractOrganizationParam(db database.Store) func(http.Handler) http.Handler

ctx := context.WithValue(r.Context(), organizationParamContextKey{}, organization)
ctx = context.WithValue(ctx, organizationMemberParamContextKey{}, organizationMember)

next.ServeHTTP(rw, r.WithContext(ctx))
})
}
Expand Down
1 change: 0 additions & 1 deletion coderd/rbac/authz.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ func (a RegoAuthorizer) AuthorizeByRoleName(ctx context.Context, subjectID strin
}
roles = append(roles, r)
}

return a.Authorize(ctx, subjectID, roles, action, object)
}

Expand Down
8 changes: 2 additions & 6 deletions coderd/roles_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,11 @@ import (
"net/http"
"testing"

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

"github.com/google/uuid"

"github.com/coder/coder/codersdk"

"github.com/google/uuid"
"github.com/stretchr/testify/require"

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

func TestListRoles(t *testing.T) {
Expand Down