-
-
Notifications
You must be signed in to change notification settings - Fork 727
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
chore: SCIM guard for users #6836
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
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.
❌ Code Health Quality Gates: FAILED
- Declining Code Health: 1 findings(s) 🚩
handleNotFound, | ||
handleUnauthorized, | ||
handleForbidden, | ||
propagateErrors: true, |
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.
The reason for this change was to have proper errors in the UI toast, otherwise we would get cannot read property "msg" of undefined
errors. I think this is a safe change. Can you please confirm @FredrikOseberg and @Tymek?
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.
Double checked with @FredrikOseberg and this should be safe as long as the calls to this hook's methods are wrapped in a try
/catch
, which they seem to be.
frontend/src/component/admin/users/UsersList/ChangePassword/ChangePassword.tsx
Show resolved
Hide resolved
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 like it. It'd suggest double checking my comment on moving the boolean check to avoid an API call before you merge but this looks quite nice!
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.
❌ Code Health Quality Gates: FAILED
- Declining Code Health: 1 findings(s) 🚩
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.
❌ Code Health Quality Gates: FAILED
- Declining Code Health: 1 findings(s) 🚩
https://linear.app/unleash/issue/2-2093/api-should-not-allow-manual-management-of-scim-managed-users-in
Introduces a SCIM guard for SCIM users. SCIM users should be managed exclusively by the SCIM client, not Unleash.
We decided to be restrictive for now, completely covering all of the write methods, but may fine-tune some of this at a later stage.
Will eventually be followed up by a UI-centric PR.