Leaving some TODOs to come back to as I add authorize to more endpoints. There are some routes I might want to move around or scope differently.

Might add more resources for granular control of user settings, but for now, it's all clumped under update for the user resource.

This is my TODO homework for my next PR. You can see that I would rather get some approvals before I continue the grind.

This was a bug that was causing panics in unit tests.

admin updating a user's roles, cool!

I had to export this to use it generically in my unit test.

This brings sadness

Could be helpful though as now we can make unit tests that send malformed payloads 🤷‍♂️

I'm not opposed to having both in package and out of package tests for different kinds of testing (black box/white box)

praise: This is a really useful check to have. Nice one.

💯

I'd tried something before where I returned a coderdtest.API struct to which we could add extra fields, but it touched basically everything. This is probably a way nicer/better way of doing it.

Also, having control of the test server is definitely something we want to have available in unit tests.

Yea, I really didn't want to touch much. I just needed access to the server. If you pass things into the Options, you do have some of those fields you'd include in that API type.

This indent makes this harder to read. Can we not push this middleware into the r.Use() block?

It's just for 1 endpoint. I can put it inside a group block by itself with use?

Can we make it a middleware? I feel like calling authorize in the handler is a sign of a failure in what we originally want.

It would be a middleware made for just this endpoint. The authorize check has to be called on each role assigned/deleted. So it depends on the payload of the request body.

Some endpoints will likely have this because it's really difficult to determine the authorize input at the middleware level, as it might be dependent on the request body.

One good thing to note is that my unit test still ensures if we don't use the middleware, an authorize check is still called. We still need to add tests for the endpoint to test auth though and make sure a user can't give themselves admin for example.

Also, is this the only endpoint where we require multiple RBAC checks for a specific action? Or, do we need to make things more general?

Because the test consuming this is just using the HTTP handler returned and not the actual server, it might be clearer to just call coderd.New directly in that test instead of changing this.

Considering none of our other tests have required access to the HTTP server object, it seems like that could be cleaner.

The thing is, if I don't use the coderdtest for the setup, I hit a lot of random issues with endpoints because the underlying infrastructure is not implemented. I do use the server, as I am using the sdk Request function to ensure the proper auth, and I have to do setup for the unit tests.

I will need to make a template & workspace for certain endpoints to not return 404's.

It is much easier to know the coderd I am interacting with works when I use coderdtest.

The only thing it should need to check for permissions is Database and Authorize!

Yes, but the unit test calls all possible routes, and checks if authorize was called since I fake the Authorizer and can check if it was called. So I need to setup all the routes on a server.

I could do this just on the handler returned by coderd.New(), but it does make it kinda annoying to make the setup. I need to make a first user for the api key middleware to work, which requires a client for all the helper funcs. I need to make an organization for some of the routes. I probably need to make a template & workspace too. All this setup means if I only have the DB, I have to make the DB calls directly. It's much easier to to just the sdk.

So I could probably make it work without calling codertest.New(), but I don't see the value in doing all this work to prop up all this "fake" stuff, when I can just use coderdtest.New(). It does all that for me and I have the same tools as all the other tests 🤷.

The only downside is that I need to expose the server to the unit test to grab the handler to walk it.

I'm confused why the reflect.ValueOf solves this, but the nil check doesn't.

Because of how we pass the nil value. We pass it from a struct, and Go is being Go.

See this:
https://go.dev/play/p/9mgJ58zg7kn

It sucks, I was trying to rework it to fix it, and it's kinda a PIA

One way we can fix this is avoid passing in nil in the first place.

Very interesting

I'm not opposed to having both in package and out of package tests for different kinds of testing (black box/white box)

it's very strange to me to have owner, castOK := unknownOwner.(database.User) appear in this if statement when neither value is evaluated by the if. I think it would be less confusing on the next line inside the block

I see how it is confusing, I can rework this logic flow

I find this very confusing. What exactly is an httpmw.RBACObject and how is it different from an rbac.Object?

rbac.Object is what is used by the rbac package. When making a middleware to construct this object, there are times where the fields of the object are not static, and need to be determined based on the request. So a middleware can set a WithOwner function, instead of a static field.

(I added a comment)

This design seems to leave us in a place where we're calling Authorize before we've actually constructed the full object, so the first part of this method is collecting bits of information about the object itself. Is there a way that we can construct the object first, then call authorize?

I don't think I follow this comment.

This Authorize middleware builds the object from the following:

• First grabs the object set by httpmw.WithRBACObject(...). This sets all the static fields of the rbac object to check (likely just the type)
• Any endpoint that has {organization} or {user} we assume the resource is owned by the organization and/or user in the url params. (This is currently mostly always true)
• In some cases the above is not true, you can use httpmw.WithOwner(...) to set the owner of the object dynamically. You cannot set it statically because it is dependent on the request's context. So it is a function that takes the request, and outputs some uuid.

The actual auth call is done at the bottom:

this leaks whether an organization with this name exists, since if it doesn't, you get NotFound, but if it does, and you're just not allowed to see it, you get Unauthorized. To avoid this we should probably return NotFound in both cases.

Doesn't the 404 above leak that if that is a concern? I think we should change the 404 to unauthorized rather than the other way around.

I'll make the 404 return unauthorized

Is that true? I'm used to systems where there are roles that don't grant the power to create others at the same level. Like, if you're a contributor to a project, you don't get to add additional contributors.

That is not what this comment is. I'll add more clarity.

At first there was an endpoint to "AddRole" and "RemoveRole". But it was replaced by a generic "SetRoles". When you call "SetRoles", I only check the difference in the sets. If a member tries to assign themselves the roles they already have, there is no difference and this for loop just "continues".

I'm concerned that you need update privilege on a user to be able to assign a role to that user. It feels like that would limit role assignment to essentially site admins because you don't want just org owners to be able to make arbitrary changes to a user.

Good point.

My goal was to have this first draft just lump everything into user resource. As we get some more variety, then we can have things like user, user_password, user_roles, user_profile, etc. This allows more granular control like you mentioned.

For this PR though, we only have admin and org-admin who can assign roles. No one else can. So I'd like to keep it as is for now. Once RBAC is on all the routes (which I have not finished), we can go back in and add more granularity where needed.

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

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