- Notifications
You must be signed in to change notification settings - Fork1.1k
feat: implement patch and get api methods for role sync#14692
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
4a7bfcf to2c9420dCompare| // Mapping maps from an OIDC group --> Coder organization role | ||
| Mappingmap[string][]string`json:"mapping"` | ||
| } | ||
| typeRoleSyncSettings codersdk.RoleSyncSettings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@f0ssel techincally it's a type definition.
Aliasing istype 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 theidpsync 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.
Ideallycodersdk would type alias of the theidpsync.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Will this break any existing usages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 thepatch 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Follow up would be fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.