-
Notifications
You must be signed in to change notification settings - Fork 8.6k
[User profiles] Strict user profile setting keys #241213
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
Changes from all commits
29fb45d
fe38561
2cb6b15
29bd445
8962ee6
9858b59
b98b96e
ab1f90c
dc6073e
7b6cddf
50558a2
cdbe839
25457b8
0f4fe06
d7d24f9
1f76e37
ba6c078
44c1ead
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,36 @@ const ALLOWED_KEYS_UPDATE_CLOUD = [ | |
| 'solutionNavigationTour:completed', // TODO: remove with https://github.com/elastic/kibana/issues/239313 | ||
| ]; | ||
|
|
||
| const MAX_STRING_FIELD_LENGTH = 1024; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question/suggestion: Is there any particular reason or use case why we want to support 1024 characters for If we have a user that has anything in the user profile that's larger than these limits, nothing would break except they'll have to obey the new limits if they decide to update the data, right?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That's right. It shouldn't affect any existing data since it's just validation on save. No particular reason for 1024. Matching the UI limits fromm the form field works for me too. |
||
|
|
||
| const MAX_USER_PROFILE_DATA_SIZE_BYTES = 1000 * 1024; | ||
|
|
||
| const userProfileUpdateSchema = schema.object({ | ||
| avatar: schema.maybe( | ||
| schema.object({ | ||
| initials: schema.nullable(schema.string({ maxLength: MAX_STRING_FIELD_LENGTH })), | ||
| color: schema.nullable(schema.string({ maxLength: MAX_STRING_FIELD_LENGTH })), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting, i hadn't come across this. I'll create an issue for us to look into.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Issue created: #245085 |
||
| imageUrl: schema.nullable(schema.string()), | ||
|
Comment on lines
+30
to
+33
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've added some defaults for maxLength for these fields. @azasypkin (tagging you since i saw you assigned to the review 😅) - I'm not sure how to approach restricting imageUrl length. |
||
| }) | ||
| ), | ||
| userSettings: schema.maybe( | ||
| schema.object({ | ||
| darkMode: schema.maybe( | ||
| schema.oneOf([ | ||
| schema.literal('system'), | ||
| schema.literal('dark'), | ||
| schema.literal('light'), | ||
| schema.literal('space_default'), | ||
| ]) | ||
| ), | ||
| contrastMode: schema.maybe( | ||
| schema.oneOf([schema.literal('system'), schema.literal('standard'), schema.literal('high')]) | ||
| ), | ||
| 'solutionNavigationTour:completed': schema.maybe(schema.boolean()), | ||
| }) | ||
| ), | ||
| }); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Related to https://github.com/elastic/kibana/pull/241213/files#r2503396102 We could add special validation logic here to disallow falsy values. |
||
|
|
||
| export function defineUpdateUserProfileDataRoute({ | ||
| router, | ||
| getSession, | ||
|
|
@@ -39,7 +69,12 @@ export function defineUpdateUserProfileDataRoute({ | |
| }, | ||
| }, | ||
| validate: { | ||
| body: schema.recordOf(schema.string(), schema.any()), | ||
| body: userProfileUpdateSchema, | ||
| }, | ||
| options: { | ||
| body: { | ||
| maxBytes: MAX_USER_PROFILE_DATA_SIZE_BYTES, | ||
| }, | ||
| }, | ||
| }, | ||
| createLicensedRouteHandler(async (context, request, response) => { | ||
|
|
||

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.
note: can we also add tests for newly added constraints (enums, string length and body size)?
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.
Updated in d7d24f9