Skip to content
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 support for sharing saved objects to all spaces #76132

Merged
merged 29 commits into from
Oct 5, 2020
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
8cdf171
Change `delete` to always delete the object
jportner Aug 27, 2020
d978bce
Change `SavedObjectsRepository` to support `'*'` namespace string
jportner Sep 10, 2020
966f826
Small refactor for SecureSavedObjectsClientWrapper unit tests
jportner Sep 11, 2020
69f6491
Change `create` and `bulkCreate` to allow initial namespaces
jportner Sep 18, 2020
2e1dabb
Change "share to space" initial warning callout
jportner Sep 21, 2020
6012e05
Clean up "Shared spaces" column code, add unit tests
jportner Sep 22, 2020
87ed4e8
Change "Shared spaces" column to display "All spaces" badge
jportner Sep 22, 2020
8385cdb
Clean up "Share to space" routes, add unit tests
jportner Sep 23, 2020
b79aff1
Remove dead code
jportner Sep 23, 2020
1deac86
Clean up saved object authorization unit tests / errors
jportner Sep 23, 2020
a15abff
Change "Share to space" flyout to support sharing to all spaces
jportner Sep 30, 2020
e68ecc4
Change saved objects table to use force-delete
jportner Sep 30, 2020
f855fcd
Fix "Copy to space" functional test
jportner Sep 30, 2020
1761d87
Fix unit tests that broke due to e68ecc4
jportner Sep 30, 2020
42b6541
Merge branch 'master' into issue-69808-share-to-all-spaces
jportner Oct 1, 2020
ebf7da0
PR review feedback
jportner Oct 1, 2020
48f0698
Tweak "no spaces available" text
jportner Oct 1, 2020
ebea8e4
Changes for additional PR review feedback
jportner Oct 1, 2020
bc9a57b
Address nits with SSOTAS authz
legrego Oct 1, 2020
c55dca3
Change `addToNamespaces` and `deleteFromNamespaces` authZ check
jportner Oct 1, 2020
64a5b84
Don't pass start services to all share-to-space components
jportner Oct 1, 2020
c95bd93
Fix type check
jportner Oct 2, 2020
cb006b1
Merge branch 'master' into issue-69808-share-to-all-spaces
jportner Oct 2, 2020
08eaf51
Fix API integration tests
jportner Oct 2, 2020
bdcf5fe
Merge branch 'master' into issue-69808-share-to-all-spaces
jportner Oct 2, 2020
e0ad362
More changes for PR review feedback
jportner Oct 2, 2020
790a437
Rename `initialNamespaces` to `namespaces`
jportner Oct 2, 2020
57ad704
Merge branch 'master' into issue-69808-share-to-all-spaces
jportner Oct 3, 2020
3c0aa6b
Fix API integration tests
jportner Oct 3, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions docs/api/saved-objects/bulk_create.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,10 @@ experimental[] Create multiple {kib} saved objects.
`references`::
(Optional, array) Objects with `name`, `id`, and `type` properties that describe the other saved objects in the referenced object. To refer to the other saved object, use `name` in the attributes. Never use `id` to refer to the other saved object. `id` can be automatically updated during migrations, import, or export.

`initialNamespaces`::
(Optional, string array) Identifiers for the <<xpack-spaces,spaces>> in which this object should be created. If this is not provided, the
object will be created in the current space.

`version`::
(Optional, number) Specifies the version.

Expand Down
4 changes: 4 additions & 0 deletions docs/api/saved-objects/create.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@ any data that you send to the API is properly formed.
`references`::
(Optional, array) Objects with `name`, `id`, and `type` properties that describe the other saved objects that this object references. Use `name` in attributes to refer to the other saved object, but never the `id`, which can update automatically during migrations or import/export.

`initialNamespaces`::
(Optional, string array) Identifiers for the <<xpack-spaces,spaces>> in which this object should be created. If this is not provided, the
object will be created in the current space.

[[saved-objects-api-create-request-codes]]
==== Response code

Expand Down
8 changes: 8 additions & 0 deletions docs/api/saved-objects/delete.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,14 @@ WARNING: Once you delete a saved object, _it cannot be recovered_.
`id`::
(Required, string) The object ID that you want to remove.

[[saved-objects-api-delete-query-params]]
==== Query parameters

`force`::
(Optional, boolean) When true, forces an object to be deleted if it exists in multiple namespaces.
+
TIP: Use this if you attempted to delete an object and received an HTTP 400 error with the following message: _"Unable to delete saved object that exists in multiple namespaces, use the `force` option to delete it anyway"_

[[saved-objects-api-delete-response-codes]]
==== Response code

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,5 @@ Deletes an object
<b>Signature:</b>

```typescript
delete: (type: string, id: string) => ReturnType<SavedObjectsApi['delete']>;
delete: (type: string, id: string, options?: SavedObjectsDeleteOptions | undefined) => ReturnType<SavedObjectsApi['delete']>;
```
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ The constructor for this class is marked as internal. Third-party code should no
| [bulkCreate](./kibana-plugin-core-public.savedobjectsclient.bulkcreate.md) | | <code>(objects?: SavedObjectsBulkCreateObject[], options?: SavedObjectsBulkCreateOptions) =&gt; Promise&lt;SavedObjectsBatchResponse&lt;unknown&gt;&gt;</code> | Creates multiple documents at once |
| [bulkGet](./kibana-plugin-core-public.savedobjectsclient.bulkget.md) | | <code>(objects?: Array&lt;{</code><br/><code> id: string;</code><br/><code> type: string;</code><br/><code> }&gt;) =&gt; Promise&lt;SavedObjectsBatchResponse&lt;unknown&gt;&gt;</code> | Returns an array of objects by id |
| [create](./kibana-plugin-core-public.savedobjectsclient.create.md) | | <code>&lt;T = unknown&gt;(type: string, attributes: T, options?: SavedObjectsCreateOptions) =&gt; Promise&lt;SimpleSavedObject&lt;T&gt;&gt;</code> | Persists an object |
| [delete](./kibana-plugin-core-public.savedobjectsclient.delete.md) | | <code>(type: string, id: string) =&gt; ReturnType&lt;SavedObjectsApi['delete']&gt;</code> | Deletes an object |
| [delete](./kibana-plugin-core-public.savedobjectsclient.delete.md) | | <code>(type: string, id: string, options?: SavedObjectsDeleteOptions &#124; undefined) =&gt; ReturnType&lt;SavedObjectsApi['delete']&gt;</code> | Deletes an object |
| [find](./kibana-plugin-core-public.savedobjectsclient.find.md) | | <code>&lt;T = unknown&gt;(options: SavedObjectsFindOptions) =&gt; Promise&lt;SavedObjectsFindResponsePublic&lt;T&gt;&gt;</code> | Search for objects |
| [get](./kibana-plugin-core-public.savedobjectsclient.get.md) | | <code>&lt;T = unknown&gt;(type: string, id: string) =&gt; Promise&lt;SimpleSavedObject&lt;T&gt;&gt;</code> | Fetches a single object |

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<!-- Do not edit this file. It is automatically generated by API Documenter. -->

[Home](./index.md) &gt; [kibana-plugin-core-server](./kibana-plugin-core-server.md) &gt; [SavedObjectsBulkCreateObject](./kibana-plugin-core-server.savedobjectsbulkcreateobject.md) &gt; [initialNamespaces](./kibana-plugin-core-server.savedobjectsbulkcreateobject.initialnamespaces.md)

## SavedObjectsBulkCreateObject.initialNamespaces property

Optional initial namespaces for the object to be created in. If this is defined, it will supersede the namespace ID that is in [SavedObjectsCreateOptions](./kibana-plugin-core-server.savedobjectscreateoptions.md)<!-- -->.

Note: this can only be used for multi-namespace object types.

<b>Signature:</b>

```typescript
initialNamespaces?: string[];
```
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ export interface SavedObjectsBulkCreateObject<T = unknown>
| --- | --- | --- |
| [attributes](./kibana-plugin-core-server.savedobjectsbulkcreateobject.attributes.md) | <code>T</code> | |
| [id](./kibana-plugin-core-server.savedobjectsbulkcreateobject.id.md) | <code>string</code> | |
| [initialNamespaces](./kibana-plugin-core-server.savedobjectsbulkcreateobject.initialnamespaces.md) | <code>string[]</code> | Optional initial namespaces for the object to be created in. If this is defined, it will supersede the namespace ID that is in [SavedObjectsCreateOptions](./kibana-plugin-core-server.savedobjectscreateoptions.md)<!-- -->.<!-- -->Note: this can only be used for multi-namespace object types. |
| [migrationVersion](./kibana-plugin-core-server.savedobjectsbulkcreateobject.migrationversion.md) | <code>SavedObjectsMigrationVersion</code> | Information about the migrations that have been applied to this SavedObject. When Kibana starts up, KibanaMigrator detects outdated documents and migrates them based on this value. For each migration that has been applied, the plugin's name is used as a key and the latest migration version as the value. |
| [originId](./kibana-plugin-core-server.savedobjectsbulkcreateobject.originid.md) | <code>string</code> | Optional ID of the original saved object, if this object's <code>id</code> was regenerated |
| [references](./kibana-plugin-core-server.savedobjectsbulkcreateobject.references.md) | <code>SavedObjectReference[]</code> | |
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<!-- Do not edit this file. It is automatically generated by API Documenter. -->

[Home](./index.md) &gt; [kibana-plugin-core-server](./kibana-plugin-core-server.md) &gt; [SavedObjectsCreateOptions](./kibana-plugin-core-server.savedobjectscreateoptions.md) &gt; [initialNamespaces](./kibana-plugin-core-server.savedobjectscreateoptions.initialnamespaces.md)

## SavedObjectsCreateOptions.initialNamespaces property

Optional initial namespaces for the object to be created in. If this is defined, it will supersede the namespace ID that is in [SavedObjectsCreateOptions](./kibana-plugin-core-server.savedobjectscreateoptions.md)<!-- -->.

Note: this can only be used for multi-namespace object types.

<b>Signature:</b>

```typescript
initialNamespaces?: string[];
```
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export interface SavedObjectsCreateOptions extends SavedObjectsBaseOptions
| Property | Type | Description |
| --- | --- | --- |
| [id](./kibana-plugin-core-server.savedobjectscreateoptions.id.md) | <code>string</code> | (not recommended) Specify an id for the document |
| [initialNamespaces](./kibana-plugin-core-server.savedobjectscreateoptions.initialnamespaces.md) | <code>string[]</code> | Optional initial namespaces for the object to be created in. If this is defined, it will supersede the namespace ID that is in [SavedObjectsCreateOptions](./kibana-plugin-core-server.savedobjectscreateoptions.md)<!-- -->.<!-- -->Note: this can only be used for multi-namespace object types. |
| [migrationVersion](./kibana-plugin-core-server.savedobjectscreateoptions.migrationversion.md) | <code>SavedObjectsMigrationVersion</code> | Information about the migrations that have been applied to this SavedObject. When Kibana starts up, KibanaMigrator detects outdated documents and migrates them based on this value. For each migration that has been applied, the plugin's name is used as a key and the latest migration version as the value. |
| [originId](./kibana-plugin-core-server.savedobjectscreateoptions.originid.md) | <code>string</code> | Optional ID of the original saved object, if this object's <code>id</code> was regenerated |
| [overwrite](./kibana-plugin-core-server.savedobjectscreateoptions.overwrite.md) | <code>boolean</code> | Overwrite existing documents (defaults to false) |
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<!-- Do not edit this file. It is automatically generated by API Documenter. -->

[Home](./index.md) &gt; [kibana-plugin-core-server](./kibana-plugin-core-server.md) &gt; [SavedObjectsDeleteOptions](./kibana-plugin-core-server.savedobjectsdeleteoptions.md) &gt; [force](./kibana-plugin-core-server.savedobjectsdeleteoptions.force.md)

## SavedObjectsDeleteOptions.force property

Force deletion of an object that exists in multiple namespaces

<b>Signature:</b>

```typescript
force?: boolean;
```
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,6 @@ export interface SavedObjectsDeleteOptions extends SavedObjectsBaseOptions
| Property | Type | Description |
| --- | --- | --- |
| [force](./kibana-plugin-core-server.savedobjectsdeleteoptions.force.md) | <code>boolean</code> | Force deletion of an object that exists in multiple namespaces |
| [refresh](./kibana-plugin-core-server.savedobjectsdeleteoptions.refresh.md) | <code>MutatingOperationRefreshSetting</code> | The Elasticsearch Refresh setting for this operation |
3 changes: 2 additions & 1 deletion src/core/public/public.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -1029,8 +1029,9 @@ export class SavedObjectsClient {
}>) => Promise<SavedObjectsBatchResponse<unknown>>;
bulkUpdate<T = unknown>(objects?: SavedObjectsBulkUpdateObject[]): Promise<SavedObjectsBatchResponse<unknown>>;
create: <T = unknown>(type: string, attributes: T, options?: SavedObjectsCreateOptions) => Promise<SimpleSavedObject<T>>;
// Warning: (ae-forgotten-export) The symbol "SavedObjectsDeleteOptions" needs to be exported by the entry point index.d.ts
// Warning: (ae-forgotten-export) The symbol "SavedObjectsClientContract" needs to be exported by the entry point index.d.ts
delete: (type: string, id: string) => ReturnType<SavedObjectsClientContract_2['delete']>;
delete: (type: string, id: string, options?: SavedObjectsDeleteOptions | undefined) => ReturnType<SavedObjectsClientContract_2['delete']>;
// Warning: (ae-forgotten-export) The symbol "SavedObjectsFindOptions" needs to be exported by the entry point index.d.ts
find: <T = unknown>(options: SavedObjectsFindOptions_2) => Promise<SavedObjectsFindResponsePublic<T>>;
get: <T = unknown>(type: string, id: string) => Promise<SimpleSavedObject<T>>;
Expand Down
4 changes: 3 additions & 1 deletion src/core/public/saved_objects/saved_objects_client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,9 @@ describe('SavedObjectsClient', () => {
Object {
"body": undefined,
"method": "DELETE",
"query": undefined,
"query": Object {
"force": false,
},
},
]
`);
Expand Down
18 changes: 16 additions & 2 deletions src/core/public/saved_objects/saved_objects_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,12 @@ export interface SavedObjectsBatchResponse<T = unknown> {
savedObjects: Array<SimpleSavedObject<T>>;
}

/** @public */
export interface SavedObjectsDeleteOptions {
/** Force deletion of an object that exists in multiple namespaces */
force?: boolean;
}

/**
* Return type of the Saved Objects `find()` method.
*
Expand Down Expand Up @@ -261,12 +267,20 @@ export class SavedObjectsClient {
* @param id
* @returns
*/
public delete = (type: string, id: string): ReturnType<SavedObjectsApi['delete']> => {
public delete = (
type: string,
id: string,
options?: SavedObjectsDeleteOptions
): ReturnType<SavedObjectsApi['delete']> => {
if (!type || !id) {
return Promise.reject(new Error('requires type and id'));
}

return this.savedObjectsFetch(this.getPath([type, id]), { method: 'DELETE' });
const query = {
force: !!options?.force,
};

return this.savedObjectsFetch(this.getPath([type, id]), { method: 'DELETE', query });
};

/**
Expand Down
1 change: 1 addition & 0 deletions src/core/server/saved_objects/routes/bulk_create.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ export const registerBulkCreateRoute = (router: IRouter) => {
})
)
),
initialNamespaces: schema.maybe(schema.arrayOf(schema.string(), { minSize: 1 })),
})
),
},
Expand Down
5 changes: 3 additions & 2 deletions src/core/server/saved_objects/routes/create.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,15 +44,16 @@ export const registerCreateRoute = (router: IRouter) => {
})
)
),
initialNamespaces: schema.maybe(schema.arrayOf(schema.string(), { minSize: 1 })),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question For searching across spaces, the /api/saved_objects/_find API accepts an array of namespace strings via the namespaces query parameter.

What do you think about using namespces here instead of initialNamespaces so that the APIs are a bit more consistent?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, everywhere else that we use the namespace or namespaces field, we are specifying what namespace(s) to search for an object in.

It seems to me that we should make this distinct, since this is what namespace(s) to create this object in. I know this is only used for the "create" and "bulkCreate" methods, but still.

I lean towards leaving it as-is, maybe @pgayvallet has an opinion to share?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I had [somehow] forgotten about the bulk_update changes we just made where we use namespace to denote where to locate the object, as opposed to listing the namespaces the updated object should be included in.

I'll buy that argument, but also happy to defer to platform since they own the APIs

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get the distinction between namespaces parameters that are used to locate the object(s) and parameters that are used to define the values to set, but as adding/removing namespaces is done with specific APIs. I doubt we will ever have two 'namespaces' parameters in a route / so API, so I would just use the namespaces name for create/bulkCreate.

But that's not a strong opinion. @rudolf, WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the behaviour of setting something on create and finding inside a namespace on get is intuitive enough.

But it seems like we can delete an object from an existing namespace when you set initialNamespaces: 'new space' and namespace: 'the space the object exists in' with overwrite=true.

I find it difficult to think what the behaviour of initialNamespaces with overwrite=true should be. It feels like you should only be able to overwrite if you're not touching the namespaces (initialNamespaces = []). I can't remember, why did we want to restrict namespace operations to namespace-specific methods like deleteFromNamespace? It feels like similar reasoning should apply to bulkCreate (even with overwrite=true)

Copy link
Contributor Author

@jportner jportner Oct 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the behaviour of setting something on create and finding inside a namespace on get is intuitive enough.

Fair enough!

But it seems like we can delete an object from an existing namespace when you set initialNamespaces: 'new space' and namespace: 'the space the object exists in' with overwrite=true.

Yes you can overwrite an object and change its namespace by using overwrite=true. This is by design.

I find it difficult to think what the behaviour of initialNamespaces with overwrite=true should be. It feels like you should only be able to overwrite if you're not touching the namespaces (initialNamespaces = []).

Well, Elasticsearch treats a create+overwrite as if you're creating a new document and blowing the old one away, so my opinion is we shouldn't artificially limit the behavior here -- there are potentially valid use cases where we might want to recreate an existing object in "all spaces", for instance.

This could result in a scenario that's a bit counter-intuitive: If I am a user who can only access the "Foo" space, I can overwrite a saved object that exists in the "Foo" and "Bar" spaces using initialNamespaces: ['foo'] (effectively unsharing it from the "Bar" space). But this outcome is no different than deleting and recreating the object, which the user is also allowed to do.

I can't remember, why did we want to restrict namespace operations to namespace-specific methods like deleteFromNamespace?

Several reasons: 1. At the time, namespace/namespaces was an implementation detail that was completely hidden from consumers, 2. I didn't think it was a good idea to conflate the concept of a namespace with normal object attributes that can be changed via an "update" operation, 3. Having dedicated "addToNamespaces" and "deleteFromNamespaces" APIs for this makes authorization checks a lot simpler to implement and easier to test / reason about, and 4. We may eventually want to allow creation of roles that are not allowed to share/unshare objects. (I guess we'd have to revisit the create+overwrite situation if we wanted to actually implement this)

Edit: renamed this field in 790a437.

Edit 2: opened a PR to undo this rename in #79682.

}),
},
},
router.handleLegacyErrors(async (context, req, res) => {
const { type, id } = req.params;
const { overwrite } = req.query;
const { attributes, migrationVersion, references } = req.body;
const { attributes, migrationVersion, references, initialNamespaces } = req.body;

const options = { id, overwrite, migrationVersion, references };
const options = { id, overwrite, migrationVersion, references, initialNamespaces };
const result = await context.core.savedObjects.client.create(type, attributes, options);
return res.ok({ body: result });
})
Expand Down
6 changes: 5 additions & 1 deletion src/core/server/saved_objects/routes/delete.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,15 @@ export const registerDeleteRoute = (router: IRouter) => {
type: schema.string(),
id: schema.string(),
}),
query: schema.object({
force: schema.maybe(schema.boolean()),
}),
},
},
router.handleLegacyErrors(async (context, req, res) => {
const { type, id } = req.params;
const result = await context.core.savedObjects.client.delete(type, id);
const { force } = req.query;
const result = await context.core.savedObjects.client.delete(type, id, { force });
return res.ok({ body: result });
})
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,19 @@ describe('DELETE /api/saved_objects/{type}/{id}', () => {
.delete('/api/saved_objects/index-pattern/logstash-*')
.expect(200);

expect(savedObjectsClient.delete).toHaveBeenCalledWith('index-pattern', 'logstash-*');
expect(savedObjectsClient.delete).toHaveBeenCalledWith('index-pattern', 'logstash-*', {
force: undefined,
});
});

it('can specify `force` option', async () => {
await supertest(httpSetup.server.listener)
.delete('/api/saved_objects/index-pattern/logstash-*')
.query({ force: true })
.expect(200);

expect(savedObjectsClient.delete).toHaveBeenCalledWith('index-pattern', 'logstash-*', {
force: true,
});
});
});
Loading