-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Add created_by
to saved objects
#179344
Add created_by
to saved objects
#179344
Conversation
/ci |
1 similar comment
/ci |
…eated-by # Conflicts: # packages/core/saved-objects/core-saved-objects-migration-server-internal/src/__snapshots__/kibana_migrator.test.ts.snap # packages/core/saved-objects/core-saved-objects-migration-server-internal/src/core/__snapshots__/build_active_mappings.test.ts.snap
/ci |
…to d/2024-03-25-created-by # Conflicts: # packages/core/saved-objects/core-saved-objects-server/tsconfig.json
/ci |
/ci |
/ci |
/ci |
/ci |
created_by
to saved objects
created_by
to saved objectscreated_by
to saved objects
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.
Overall the PR looks fine. The changes at Core / the SOR level are what I expected, and test coverage was properly added.
My only real question is regarding the way we're passing down the getCurrentUser
API down to the SOR (see comment)
getCurrentUserId(): string | null { | ||
return this.securityExtension?.getCurrentUser()?.profile_uid ?? null; | ||
} |
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.
Question to @azasypkin regarding the naming. Are userId
and profile_uid
the same thing? Are we fine with getCurrentUserId
here?
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.
We don't have such a thing as userId
yet, and I'm somewhat hesitant to introduce a new concept at this point. Considering userProfiles
and uids
are already exposed through our public APIs, would getCurrentUserProfileUid
be that bad? If a larger API surface within SOR/SOC isn't an issue, we can also consider just re-exposing getCurrentUser
...
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'll update to getCurrentUserProfileUid
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.
LGTM
ACK: will review today |
Pinging @elastic/kibana-security (Team:Security) |
@@ -52,7 +52,7 @@ export const createUserActionFindSO = ( | |||
}); | |||
|
|||
export const createConnectorUserAction = ( | |||
overrides?: Partial<CaseUserActionWithoutReferenceIds> | |||
overrides?: Partial<SavedObject<CaseUserActionWithoutReferenceIds>> |
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.
@elastic/response-ops-cases, the overrides from these utils are unused, but adding a created_by
on a root saved object found a type issue with the types
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.
Cases changes LGTM!
@elasticmachine merge upstream |
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.
Re-approving for core to unblock progress, read through the code and LGTM.
@elasticmachine merge upstream |
buildkite test this |
…eated-by # Conflicts: # x-pack/plugins/security/tsconfig.json
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
Page load bundle
Saved Objects .kibana field count
Unknown metric groupsAPI count
ESLint disabled line counts
References to deprecated APIs
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @Dosant |
) ## Summary Align V2 behavior with ZDT after #179595 Under the assumption that whenever we want to add new root fields (e.g. [created_by](#179344)), we will systematically add mappings for them, we can skip the `updateAndPickupMappings` operation iif ONLY root fields' mappings have changed during an upgrade (aka if none of the SO types have updated their mappings). Up until now, the logic was updating ALL SO types whenever a `root` field was added / updated. This is expensive and unnecessary, and can cause a noticeable impact to large customers during migrations.
Part of elastic/kibana-team#769 ## Summary This PR adds a "created by" filter to the dashboard listing page. We've added the created_by field recently [here](#179344) so only newly created by interactive users dashboards will be populated with `created_by` ### Demo <img width="1510" alt="Screenshot 2024-04-25 at 16 32 03" src="https://github.com/elastic/kibana/assets/7784120/ac70ffea-05be-4d70-bbfa-c1cc9d5dd747"> <img width="1510" alt="Screenshot 2024-04-25 at 16 32 11" src="https://github.com/elastic/kibana/assets/7784120/9889ccae-08d0-4fb6-977e-c42e85337e0e"> ### Implementation notes #### Suggesting users We suggest only the users that have at least one dashboard. We do this by aggregating dashboard saved objects by `created_by`. Because we don't have server-side pagination or sorting yet, in this initial version, I decided to do this client-side to keep it as simple as possible for now. Initially, I did this server-side inside a dashboard plugin as an internal API route. In the long term, this should probably be moved to content management in some form, but it is not clear how it should look yet. This is the commit where I reverted and moved it to client-side 06316ee #### Clint side filtering The user filter is simply a client-side filter because we anyway currently have an in-memory table with client-side sorting and pagination. There is no much practical value in server-side filter atm The filter allows to filter-in by multiple users. It also allows to filter items without creators #### `created_by` filter isn't part of the query string I introduced a separate state for this filter, similar to `tableSort` and `pagination`. The reason why the `created_by` filter isn't a part of the `searchQuery` is because we use `profile_id` which looks ugly for the user, e.g. `u_mGBROF_q5bmFCATLXAcCwKa0k8JvONAwSruelyKA5E_0` so we can't just add it to a user-facing search query like we do with tags. `profile_id` is the correct identified we should use, `username` might not be enough in some cases + `username` in cloud is unreadble account number. The best alternative could be email, but it isn't always available. This is why I decided to keep the `created_by` filter hidden from the user behind the user picker popover and not expose it to the search bar. The `created_by` filter is synced into the URL just like the rest of the table state. #### No distinguishing the current user There is no logic in place that somehow highlights or renames the current user in the filter. This can be added, but to limit the scope of the MVP I didn't do it. #### User filter component We use the `UsersProfilePopover` provided by the security team. We use it without any overrides, so the component decides how to show users in a popover. I noticed a small visual issue with an email truncation in a smaller popover. Might be worth a separate issue. ### Testing 1. Create multiple new users 2. Login for each of them 3. Created a dashboard for each of them 4. Play with the filter #### Next Steps The next step would be to add a column with the user: ![image (5)](https://github.com/elastic/kibana/assets/7784120/9a3fe30e-6a41-4718-a7aa-53fbe0affd22)
Part of https://github.com/elastic/kibana-team/issues/769 ## Summary This PR adds a "created by" filter to the dashboard listing page. We've added the created_by field recently [here](elastic#179344) so only newly created by interactive users dashboards will be populated with `created_by` ### Demo <img width="1510" alt="Screenshot 2024-04-25 at 16 32 03" src="https://github.com/elastic/kibana/assets/7784120/ac70ffea-05be-4d70-bbfa-c1cc9d5dd747"> <img width="1510" alt="Screenshot 2024-04-25 at 16 32 11" src="https://github.com/elastic/kibana/assets/7784120/9889ccae-08d0-4fb6-977e-c42e85337e0e"> ### Implementation notes #### Suggesting users We suggest only the users that have at least one dashboard. We do this by aggregating dashboard saved objects by `created_by`. Because we don't have server-side pagination or sorting yet, in this initial version, I decided to do this client-side to keep it as simple as possible for now. Initially, I did this server-side inside a dashboard plugin as an internal API route. In the long term, this should probably be moved to content management in some form, but it is not clear how it should look yet. This is the commit where I reverted and moved it to client-side elastic@06316ee #### Clint side filtering The user filter is simply a client-side filter because we anyway currently have an in-memory table with client-side sorting and pagination. There is no much practical value in server-side filter atm The filter allows to filter-in by multiple users. It also allows to filter items without creators #### `created_by` filter isn't part of the query string I introduced a separate state for this filter, similar to `tableSort` and `pagination`. The reason why the `created_by` filter isn't a part of the `searchQuery` is because we use `profile_id` which looks ugly for the user, e.g. `u_mGBROF_q5bmFCATLXAcCwKa0k8JvONAwSruelyKA5E_0` so we can't just add it to a user-facing search query like we do with tags. `profile_id` is the correct identified we should use, `username` might not be enough in some cases + `username` in cloud is unreadble account number. The best alternative could be email, but it isn't always available. This is why I decided to keep the `created_by` filter hidden from the user behind the user picker popover and not expose it to the search bar. The `created_by` filter is synced into the URL just like the rest of the table state. #### No distinguishing the current user There is no logic in place that somehow highlights or renames the current user in the filter. This can be added, but to limit the scope of the MVP I didn't do it. #### User filter component We use the `UsersProfilePopover` provided by the security team. We use it without any overrides, so the component decides how to show users in a popover. I noticed a small visual issue with an email truncation in a smaller popover. Might be worth a separate issue. ### Testing 1. Create multiple new users 2. Login for each of them 3. Created a dashboard for each of them 4. Play with the filter #### Next Steps The next step would be to add a column with the user: ![image (5)](https://github.com/elastic/kibana/assets/7784120/9a3fe30e-6a41-4718-a7aa-53fbe0affd22)
## Summary close elastic/kibana-team#899 - Adds `updated_by` to saved object, similar to recently added `created_by` #179344 - Fixes `created_by` / `created_at` should be set during upsert - Improves functional tests coverage
## Summary close elastic/kibana-team#899 - Adds `updated_by` to saved object, similar to recently added `created_by` elastic#179344 - Fixes `created_by` / `created_at` should be set during upsert - Improves functional tests coverage
Blocked by #179258Summary
This PR adds an optional
created_by
field to root saved object fields.We're doing for adding a filter by an author to the dashboard listing page (and then other listings)
Implementation
created_by: {type: keyword}
In this implementation, I added
created_by
as a simple keyword field assuming we will storeprofile_uid
onlyThe
profile_uid
is not always available, as @azasypkin described hereIt is not available for anonymous users, for users authenticated via proxy, and, in some cases, for API users authenticated with API keys.
But this is the best way to globally identify users and longer term we might get
profile_uid
for all the usersAccessing
getCurrentUser
from saved object repoAfter exploring different options and discussing with @pgayvallet we decided to provide
getCurrentUser
through existing security extensions as it's a better isolation of concern, and we avoid leaking the request down to the SOR.