[Write restricted dashboards] Implements import and export changes#228416
Conversation
…dashboards-crud-api
| } from '@kbn/core-saved-objects-server'; | ||
| import { createFilterStream, createMapStream } from '@kbn/utils'; | ||
|
|
||
| export function getImportTransformsFactory(): AccessControlImportTransformsFactory { |
There was a problem hiding this comment.
Opted to leave this as a factory to support phase 2, when we might need to pass in security functions/services again.
|
@SiddharthMantri The CI failure was just the post-build failure. All tests passed 🎉 |
| ); | ||
| }); | ||
|
|
||
| // describe('access control', () => { |
There was a problem hiding this comment.
see my previous comment on removing data on export
| }); | ||
| return false; | ||
| }), | ||
| ...(accessControlTransforms?.filterStream ? [accessControlTransforms.filterStream] : []), |
There was a problem hiding this comment.
since we are passing it down to a function, isn't it the same if we pass it just as is?
| ...(accessControlTransforms?.filterStream ? [accessControlTransforms.filterStream] : []), | |
| accessControlTransforms?.filterStream |
...([]) -> will end up as undefined passed to a function
There was a problem hiding this comment.
I think there's a type mismatch when passed as just accessControlTransforms?.filterStream as the stream promise function is expecting the resolve value to have an iterator. Example error:
I thought of doing something like below to maybe improve readability but not sure if it makes sense. What do you think?
const optionalStream = <T>(stream: T | undefined): T[] => ...( stream ? [stream] : [] ) ;
// ...
optionalStream(accessControlTransforms?.filterStream),
optionalStream(accessControlTransforms?.mapStream),
There was a problem hiding this comment.
we can then leave it as it is right now 👍 no need to change
There was a problem hiding this comment.
We're kinda blind to implementation changes with the factory function, and spreading unknowns makes it worse. We can cover some of the unknowns with testing. Maybe wrapping with some validation checks might be better in the long run. Let's mark that for Phase 2.
elena-shostak
left a comment
There was a problem hiding this comment.
Great job! left a couple of nits/questions I wanted to clarify before merging
|
@elasticmachine merge upstream |
💔 Build Failed
Failed CI StepsMetrics [docs]Public APIs missing comments
Public APIs missing exports
Unknown metric groupsAPI count
History
|
dplumlee
left a comment
There was a problem hiding this comment.
Rule management changes LGTM, thanks @jeramysoucy!
There was a problem hiding this comment.
Reading through the info on this and the code, I don't see rule actions as being supported for access based controls (at least for now, correct me if I'm wrong) so don't think this is necessarily reachable, but good to have this for future proofing errors.
TinaHeiligers
left a comment
There was a problem hiding this comment.
Almost done reviewing, there's a few things I want to get your thoughts on before doing one more round.
"If an export file contains access control metadata and is imported when the feature flag is disabled, import will surface an error for each object that contains the metadata, stating that the object contains unexpected metadata. While we do not expect this to happen, if it ever does, removing the metadata from an export file is fairly simple and mitigates the issue."
I can almost 100% guarantee this will happen. We have no control over where the exported saved objects come from.
src/core/packages/saved-objects/api-server-internal/src/lib/apis/create.test.ts
Show resolved
Hide resolved
| }); | ||
| return false; | ||
| }), | ||
| ...(accessControlTransforms?.filterStream ? [accessControlTransforms.filterStream] : []), |
There was a problem hiding this comment.
We're kinda blind to implementation changes with the factory function, and spreading unknowns makes it worse. We can cover some of the unknowns with testing. Maybe wrapping with some validation checks might be better in the long run. Let's mark that for Phase 2.
| }); | ||
|
|
||
| test('does not apply the access control transform to types that do not support access control', async () => { | ||
| // const accessControlExportTransform: SavedObjectsExportTransform = (ctx, objects) => { |
There was a problem hiding this comment.
uncomment the transform. Without it, we're not actually testing what it claims to.
| // Unskip after: https://github.com/elastic/kibana/issues/242671 | ||
| describe.skip('access control', () => { | ||
| test('applies the access control transform to supporting types if defined', async () => { | ||
| // const accessControlExportTransform: SavedObjectsExportTransform = (ctx, objects) => { |
| logger, | ||
| savedObjectsClient, | ||
| typeRegistry, | ||
| // accessControlExportTransform, |
| const setup = await soService.setup(createSetupDeps()); | ||
|
|
||
| const accessControlTransforms: SavedObjectsAccessControlTransforms = { | ||
| // exportTransform: jest.fn(), |
There was a problem hiding this comment.
remove or comment on why exportTransform is commented out
| const typeSupportsAccessControl = typeRegistry.supportsAccessControl(obj.type); | ||
|
|
||
| if (typeSupportsAccessControl && obj.accessControl) { | ||
| delete obj.accessControl; |
There was a problem hiding this comment.
How are we thinking about surfacing this to users?
I'm thinking along the lines of a warning to either proceed or cancel the import.
| objects: Array<SavedObject<T>> | ||
| ) => SavedObjectsImportHookResult | Promise<SavedObjectsImportHookResult>; | ||
|
|
||
| export interface AccessControlImportTransforms { |
There was a problem hiding this comment.
nit: Missing .ts docs ;-)
| mapStream: Transform; | ||
| } | ||
|
|
||
| export type AccessControlImportTransformsFactory = ( |
There was a problem hiding this comment.
nit: Missing .ts docs ;-)
| supportedTypes, | ||
| managed, | ||
| typeRegistry, | ||
| createAccessControlImportTransforms, |
There was a problem hiding this comment.
Handling access control in the importStateMap means that the ownership will already be decided by the time the objects are passed to createSavedObjects, which will work for Phase 1. However, the ownership will become tricky if we want to offer an intermittent step to change ownership per object being imported. Just a thought for later on.
a74e038
into
elastic:security/read-only-dashboards
Closes elastic/kibana-team#808 The respective teams have been raising PRs against this feature branch. Approved PRs merged so far: - #221916 - #224411 - #239973 - #241101 - #238468 - #233552 - #228416 - #241168 - #244746 - #244830 ## Summary This pull request overhauls the saved object management workflow by introducing the concept of ownership for SOs - specifically enabled for dashboards only at the moment. Owners and administrators can now control a new write-restricted flag on their objects, allowing them to keep work draft/uneditable state before publishing. This change enables users to define who can modify shared objects, providing a crucial capability to manage and share dashboards. ## Release note Kibana Dashboards now support ownership and "write_restricted" mode. Users can now keep dashboards publicly editable or in a write-restricted state until they are ready to publish, giving them more control over who can edit their dashboards, regardless of broader space permissions. ## How to test ### Serverless Please reach out to me via slack or in the project channel (#read-only-dashboards) to be invited to the serverless environment where this feature has been enabled. ### Local - Clone this PR - Enable the feature by editing kibana.yml to include ``` savedObjects.enableAccessControl: true ``` - Start ES and Kibana as you would - Once started, seed Kibana with sample data. This should create a few dashboards. - Navigate to dashboards and create a new one. - In the share modal, change the view mode `Everybody in the space Can View`, <img width="500" height="410" alt="image" src="https://github.com/user-attachments/assets/b895442f-cce3-41a6-8b47-d206a9afbf43" /> - Now create a new role which grants access to indices and dashboards all. Create a new user and then assign that role to the newly created user. <img width="500" height="410" alt="image" src="https://github.com/user-attachments/assets/dd5251e1-a3b5-41a8-abc1-7e67399d65d2" /> - Login as the new user and navigate to the dashboard you had initially set as `Can view`. You'll see that you're not able to edit the dashboard and a warning like <img width="500" height="410" alt="Screenshot 2025-11-28 at 12 30 50" src="https://github.com/user-attachments/assets/1f71ccc7-9dc6-4a68-9a2c-540aa74e4f03" /> ### Local (2nd option) You can also follow the instructions in #224411 that detail how to use the funtional test runner to test this using the test plugin created for this feature. ### Risk matrix - What happens when your feature is used in a non-default space or a custom space? Works as expected - What happens when there are multiple Kibana nodes using the same Elasticsearch cluster? Does not depend on functionality of kibana nodes - What happens when a plugin you depend on is disabled? Changes are in core and security - both are always available - What happens when a feature you depend on is disabled? No dependency - What happens when a third party integration you depend on is not responding? No third party inregration - Does the feature work in Elastic Cloud? Yes - Does the feature create a setting that needs to be exposed, or configured differently than the default, on the Elastic Cloud? No - Is there a significant performance impact that may affect Cloud Kibana instances? No - Does your feature need to be aware of running in a container? No - Does the feature Work with security disabled, or fails gracefully? If disabled, fails gracefully. - Are there performance risks associated with your feature? No - Could this cause memory to leak in either the browser or server? No - Will your feature still work if Kibana is run behind a reverse proxy? Yes - Does your feature affect other plugins? No, other plugins could choose to use it if registering a SO with ownable types - Are migrations handled gracefully? Does the feature affect old indices or saved objects? Yes, migrations taken care of. - Are you using any technologies, protocols, techniques, conventions, libraries, NPM modules, etc. that may be new or unprecedented in Kibana? No --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Jeramy Soucy <jeramy.soucy@elastic.co> Co-authored-by: Aleh Zasypkin <aleh.zasypkin@gmail.com> Co-authored-by: Christiane (Tina) Heiligers <christiane.heiligers@elastic.co> Co-authored-by: Krzysztof Kowalczyk <krzysztof.kowalczyk@elastic.co> Co-authored-by: Gerard Soldevila <gerard.soldevila@elastic.co>
Closes elastic/kibana-team#808 The respective teams have been raising PRs against this feature branch. Approved PRs merged so far: - elastic#221916 - elastic#224411 - elastic#239973 - elastic#241101 - elastic#238468 - elastic#233552 - elastic#228416 - elastic#241168 - elastic#244746 - elastic#244830 ## Summary This pull request overhauls the saved object management workflow by introducing the concept of ownership for SOs - specifically enabled for dashboards only at the moment. Owners and administrators can now control a new write-restricted flag on their objects, allowing them to keep work draft/uneditable state before publishing. This change enables users to define who can modify shared objects, providing a crucial capability to manage and share dashboards. ## Release note Kibana Dashboards now support ownership and "write_restricted" mode. Users can now keep dashboards publicly editable or in a write-restricted state until they are ready to publish, giving them more control over who can edit their dashboards, regardless of broader space permissions. ## How to test ### Serverless Please reach out to me via slack or in the project channel (#read-only-dashboards) to be invited to the serverless environment where this feature has been enabled. ### Local - Clone this PR - Enable the feature by editing kibana.yml to include ``` savedObjects.enableAccessControl: true ``` - Start ES and Kibana as you would - Once started, seed Kibana with sample data. This should create a few dashboards. - Navigate to dashboards and create a new one. - In the share modal, change the view mode `Everybody in the space Can View`, <img width="500" height="410" alt="image" src="https://github.com/user-attachments/assets/b895442f-cce3-41a6-8b47-d206a9afbf43" /> - Now create a new role which grants access to indices and dashboards all. Create a new user and then assign that role to the newly created user. <img width="500" height="410" alt="image" src="https://github.com/user-attachments/assets/dd5251e1-a3b5-41a8-abc1-7e67399d65d2" /> - Login as the new user and navigate to the dashboard you had initially set as `Can view`. You'll see that you're not able to edit the dashboard and a warning like <img width="500" height="410" alt="Screenshot 2025-11-28 at 12 30 50" src="https://github.com/user-attachments/assets/1f71ccc7-9dc6-4a68-9a2c-540aa74e4f03" /> ### Local (2nd option) You can also follow the instructions in elastic#224411 that detail how to use the funtional test runner to test this using the test plugin created for this feature. ### Risk matrix - What happens when your feature is used in a non-default space or a custom space? Works as expected - What happens when there are multiple Kibana nodes using the same Elasticsearch cluster? Does not depend on functionality of kibana nodes - What happens when a plugin you depend on is disabled? Changes are in core and security - both are always available - What happens when a feature you depend on is disabled? No dependency - What happens when a third party integration you depend on is not responding? No third party inregration - Does the feature work in Elastic Cloud? Yes - Does the feature create a setting that needs to be exposed, or configured differently than the default, on the Elastic Cloud? No - Is there a significant performance impact that may affect Cloud Kibana instances? No - Does your feature need to be aware of running in a container? No - Does the feature Work with security disabled, or fails gracefully? If disabled, fails gracefully. - Are there performance risks associated with your feature? No - Could this cause memory to leak in either the browser or server? No - Will your feature still work if Kibana is run behind a reverse proxy? Yes - Does your feature affect other plugins? No, other plugins could choose to use it if registering a SO with ownable types - Are migrations handled gracefully? Does the feature affect old indices or saved objects? Yes, migrations taken care of. - Are you using any technologies, protocols, techniques, conventions, libraries, NPM modules, etc. that may be new or unprecedented in Kibana? No --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Jeramy Soucy <jeramy.soucy@elastic.co> Co-authored-by: Aleh Zasypkin <aleh.zasypkin@gmail.com> Co-authored-by: Christiane (Tina) Heiligers <christiane.heiligers@elastic.co> Co-authored-by: Krzysztof Kowalczyk <krzysztof.kowalczyk@elastic.co> Co-authored-by: Gerard Soldevila <gerard.soldevila@elastic.co>
Closes #221754
Summary
This PR implements changes to support import and export of saved objects that support access control features - namely, "write-restricted" dashboards (formerly "read-only" dashboards).
This effort is broken down into 2 phases:
Phase 1
Import will not apply the access control metadata from the imported file. This metadata contains the owner and access mode for the object. Instead, any new objects (that support access control) created as a result of import will be owned by the importing user and will be in default mode. If there is no active user profile (import called via API), the object will be created with no access control metadata (no owner or mode), but this can be assigned later by an admin. Any objects being overwritten as a result of import will maintain existing ownership and mode, but this can be changed later by the owner or an admin if needed.
Phase 2
Import will support applying the access control metadata from the imported file, but only for "Admin" users (users with the
manage_access_controlprivilege). Admins will be able to specify an option to enable this behavior. If there is no access control metadata in the file for an existing object that will be overwritten, it will maintain existing ownership and mode.This PR handles Phase 1, and Phase 2 will be handled in a follow-up effort.
Feature Flag Note
The changes here do not check the
savedObjects.enableAccessControlconfig feature flag. The reasoning is that we still want to intercept unexpected access control metadata that may be present in export/import files and reject the import for any applicable objects. Alternatively, we could check the feature flag, and if disabled, always strip any incoming access control metadata.Details
Import
Import relies on existing saved object bulk create functionality, which is handled by #224411 and #238468. Prior to bulk create, the import operation filters invalid objects and responds with this information to surface in the UI. Bulk create already handles object ID clashes and authorization for overwriting access control objects.
When importing objects with access control, the access control metadata from the import file will be ignored in order to preserve the owner and access mode for any existing objects being overwritten.
Access control requires some new filtering capability -
Since Phase 1 does not support importing access control metadata, this metadata will be stripped from incoming objects via another transform. Because of that, the filter transform is not critical in Phase 1, however, it implements the baseline for filtering needed in Phase 2, so it was worth staging here.
To achieve the filtering and stripping, I added the ability to set "access control transforms" with the saved object service. I created an import transform factory that will create a filter stream and a map streams (the transforms). This factory is set in the saved objects service and passed to the of
SavedObjectsImporterconstructor. This loosely follows the paradigm used to set the saved object repository extensions.The filter stream handles all of the filtering of invalid objects, specific to access control, while the map stream handles removal of access control metadata (in Phase 1 for all users, in Phase 2 for non-admin users or when an admin has not specified the option to import access control metadata).
The end result - if the factory is defined, the import transform streams are generated during import, and executed inline with the other filter streams during the import process. After the invalid access control objects are filtered out, we rely on the bulk create operation to handle the rest.
Export
Originally during export we had planned to strip the access control metadata, but the design was revised. Keeping access control metadata in the export files means that they will contain everything necessary for import after Phase 2. Because of this, not export transforms are currently required.
We could implement a transform to strip the access control metadata when the access control feature flag is disabled, but this is not necessary - the additional metadata is not harmful or affective in Phase 1. We do not expect objects to contain metadata until the feature flag is enabled, and we do not expect the flag to be toggled back and forth.
Testing
This PR include both unit and integration tests for import, export, and resolve import errors APIs.
Risk
This functionality will sit behind a feature flag (to be implemented). If the feature is disabled, the transforms will not get set with the saved object service, leaving import and export to function as they did prior to this PR - verified by the existing suites of unit and integration tests.
Known issue
If an export file contains access control metadata and is imported when the feature flag is disabled, import will surface an error for each object that contains the metadata, stating that the object contains unexpected metadata. While we do not expect this to happen, if it ever does, removing the metadata from an export file is fairly simple and mitigates the issue.
Approach
As an alternative, the transforms can be moved inline with the rest of the importer and exporter codebase without significant effort.