feat: implement patch and get api methods for role sync #14692
Conversation
4a7bfcf to
2c9420d
Compare
| // Mapping maps from an OIDC group --> Coder organization role | ||
| Mapping map[string][]string `json:"mapping"` | ||
| } | ||
| type RoleSyncSettings codersdk.RoleSyncSettings |
There was a problem hiding this comment.
I see why we alias (I think that's the correct terminology?) to add the additional methods, but I wonder if we could just implement those on the codersdk type? Or would that then be a dependency issue? It would just be nice to have a single type if possible, but not required.
There was a problem hiding this comment.
@f0ssel techincally it's a type definition.
Aliasing is type RoleSyncSettings = codersdk.RoleSyncSettings. If you alias, it means the types are equivalent, and the alias is essentially replaced with the source at compile time. With a type definition, you create a new type.
I agree the aliasing is a bit strange. It exists because of an import cycle, and it also made sense to export via CoderSDK since the UI needs to consume it.
It felt strange for the idpsync package to not fully self contain it's own types. It is strange it depends on codersdk, but it is a consequence of our import cycles and not wanting to copy + paste a struct definition.
Ideally codersdk would type alias of the the idpsync.
Maybe we can change this in the future, I just didn't spend a ton of time on this. Kayla implemented this idea first, and it felt good enough to me for now.
There was a problem hiding this comment.
Yeah the import cycle is the nail in the coffin then, no worries this is still good.
| r.Group(func(r chi.Router) { | ||
| r.Use( | ||
| apiKeyMiddleware, | ||
| httpmw.ExtractOrganizationParam(api.Database), |
There was a problem hiding this comment.
Will this break any existing usages?
There was a problem hiding this comment.
Nope, it makes it more permissive, rather then restrictive. So it used to be gated by our experiment and premium licensing, but sync is an enterprise feature.
It makes sense to allow even single org deployments to configure this at runtime, so I dropped the entitlement requirements.
There is an entitlement check on the actual sync code, so tbd if we should also gate the patch endpoint, or just defer to some warning like "These settings require enterprise to function".
| return | ||
| } | ||
|
|
||
| //nolint:gocritic // Requires system context to update runtime config |
There was a problem hiding this comment.
Can we change this to let org admins change this value? If this is a user action it feels weird to drop into system here. We usually just do that for non-user actors.
There was a problem hiding this comment.
Yes, I do want to. Just with the timelines, I did this as a stop gap.
I want to solve RBAC and runtime config fields in a more general reusable pattern. Until then, I just require a system context. And I use a system context so no one else can use these sync settings incorrectly. The dbauthz layer is currently pretty strict, and when this is fixed, we will make it more permissive. Always an easier way to move forward then the other way around.
No description provided.