Skip to content

chore: protect organization endpoints with license #14001

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.

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 17 commits into from
Jul 25, 2024

Conversation

Emyrk
Copy link
Member

@Emyrk Emyrk commented Jul 24, 2024

What this does

Closes #13969

Creating, editing, and deleting an organization now require a license (and experiment). Endpoints are in the /enterprise directory now.

Any tests that created an organization in the AGPL directory didn't make sense anyway. They had no means of having a provisioner daemon, so had no functionality besides adding/removing members.

This PR is 99% moving tests from AGPL to Enterprise. And changing

client := coderdtest.New(t, nil)
coderdtest.CreateFirstUser(t, client)

To

dv := coderdtest.DeploymentValues(t)
dv.Experiments = []string{string(codersdk.ExperimentMultiOrganization)}
client, user := coderdenttest.New(t, &coderdenttest.Options{
	Options: &coderdtest.Options{
		DeploymentValues: dv,
	},
	LicenseOptions: &coderdenttest.LicenseOptions{
		Features: license.Features{
			codersdk.FeatureMultipleOrganizations: 1,
		},
	},
})

Test Coverage

There is a chance our test coverage will go down since we probably do not track cross pkg test coverage. To try and maintain the coverage and move the tests sounds like a massive headache. We can refactor some of these tests and bring them back into AGPL if it is an issue. Refactoring all the tests now would not be ideal since we need multi-org tests.

I am sure we have redundant tests, but there is no easy way to identify them.

Ignore, text before I found a good solution

Enterprise has an issue overriding sub endpoints. The only way to properly extend them is to save them somewhere, and reference the sub routers directly.

When trying to "merge" routes, you either overwrite the endpoint, or get a panic.

https://goplay.tools/snippet/6JXkSIdJ997


Alternatives

Duplicate routes

One idea is to just replace the entire sub router in Enterprise. Essentially copying the routes to both places, and using a unit test to ensure Enterprise is a super set.

This unfortunately cannot work because you cannot overwrite a route in chi.

panic: chi: attempting to Mount() a handler on an existing path, '/organizations'

Write some r.Merge(r1, r2) or r.Extend("/organizations") function

The idea is to fetch the router dynamically, and extend it. Rather than hard coding it in some field in the API struct. Unfortunately traversing the router is only supported by r.Routes() and chi.Walk(). Both expose the http.Handler but not the router. So you cannot fetch the chi.Mux dynamically without some serious changes 😢

Write a MergeRouters(a, b chi.Routes)

This is possible, but such a pain. Essentially you can call r.Routes() to traverse all the sub routers and endpoints. If you assume no typecasting (route.(*chi.Mux)), then you cannot extend these routes, only read them.

So to merge them, we'd have to build our own /pattern tree (handling things like r.Route("/nested/stuff") being depth=2 in the tree.

Then merge 2 route trees that we build.

Then convert that route tree back to a chi.Mux by doing a depth first search and doing the

chi.Route("/pattern", func(r chi.Router) {
 // When you do `r.Group` it creates a fresh middleware stack.
 // I'm not sure how this looks in the `.Routes()` response. Our tree
 // would have to be able to have different middleware stacks for the
 // same pattern nodes.
 r.Use(treeNode.Middlewares...)

 for _, subRouter := range treeNode.Routers {
   dfs(r, subRouter)
 }
 for _, endpoint := range treeNode.Endpoints {
   r.Method(endpoint.Method, endpoint.Pattern, endpoint.Handler)
 }
})

This really feels like we'd just writing our own http.Mux. It can be done, and it's not that challenging, it's just a lot of leg work that I don't find very useful at this time.

@Emyrk Emyrk changed the title Stevenmasley/protect multi org chore: move multi-org endpoints into enterprise directory Jul 24, 2024
@Emyrk Emyrk force-pushed the stevenmasley/protect_multi_org branch from 01cdd55 to d602947 Compare July 24, 2024 22:13
@Emyrk Emyrk changed the title chore: move multi-org endpoints into enterprise directory chore: move multi-org endpoints behind multi-org feature Jul 24, 2024
@Emyrk Emyrk changed the title chore: move multi-org endpoints behind multi-org feature chore: move multi org endpoints behind multi org feature and experiment Jul 25, 2024
@Emyrk Emyrk marked this pull request as ready for review July 25, 2024 00:32
@Emyrk Emyrk changed the title chore: move multi org endpoints behind multi org feature and experiment chore: organization CRUD endpoints gated by license entitlement Jul 25, 2024
@Emyrk Emyrk changed the title chore: organization CRUD endpoints gated by license entitlement chore: protect organization CRUD endpoints with license Jul 25, 2024
@Emyrk Emyrk requested a review from f0ssel July 25, 2024 00:36
@Emyrk Emyrk changed the title chore: protect organization CRUD endpoints with license chore: protect organization endpoints with license Jul 25, 2024
Copy link
Contributor

@f0ssel f0ssel left a comment

Choose a reason for hiding this comment

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

Glad we are doing this. The fact that none of the golden files, API docs, or codersdk code changed with all of this gives me confidence this should be pretty safe.

@Emyrk Emyrk force-pushed the stevenmasley/protect_multi_org branch from 018aa1d to ca00727 Compare July 25, 2024 20:32
@Emyrk
Copy link
Member Author

Emyrk commented Jul 25, 2024

Rebasing in case any new tests were added that I need to move

@Emyrk Emyrk merged commit 7ea1a4c into main Jul 25, 2024
60 checks passed
@Emyrk Emyrk deleted the stevenmasley/protect_multi_org branch July 25, 2024 21:07
@github-actions github-actions bot locked and limited conversation to collaborators Jul 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Protect org CRUD endpoints with multi-organization experiment
2 participants