[Saved Objects] Adds config flag to toggle hiddenFromHttpApis SO types conditionally #151512

TinaHeiligers · 2023-02-16T20:28:59Z

This PR adds a config option to set if the Saved Objects HTTP APIs should be accessible to types registered with hiddenFromHttpApis: true.

allowHttpApiAccess: true --------> grants access
allowHttpApiAccess: false --------> throws in Saved Objects HTTP APIs

allowHttpApiAccess can be configured in kibana.yml but we don't plan to document the option as it is not intended for public use.

The configuration prevents immediate breaking changes caused by saved objects adopting the option to be hidden from the HTTP APIs and is a stepping stone toward removing the Saved Objects HTTP APIs.

The option is only applicable to saved objects registered as hiddenFromHttpApis:true.

Checklist

Documentation was added for features that require explanation or tutorials. Code comments should be sufficient to describe the internal behavior.
Unit or functional tests were updated or added to match the most common scenarios
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list Docker list updated, we don't intend to add user settings in cloud.

Risk Matrix

Risk	Probability	Severity	Mitigation/Notes
Code should gracefully handle cases when the HTTP APIs are disabled.	Medium	High	Unit tests will verify that any feature flag or plugin combination still results in our service behaving the way intended.

For maintainers

This was checked for breaking API changes and was labeled appropriately

How to test this

Ensure that allowHttpApiAccess can be set in kibana.yml to override the default:

Add the following debug log line to the SavedObjectsService setup contract after the config is set:

log config value

public async setup(setupDeps: SavedObjectsSetupDeps): Promise<InternalSavedObjectsServiceSetup> {
    this.logger.debug('Setting up SavedObjects service');

    this.setupDeps = setupDeps;
    const { http, elasticsearch, coreUsageData, deprecations } = setupDeps;

    const savedObjectsConfig = await firstValueFrom(
      this.coreContext.configService.atPath<SavedObjectsConfigType>('savedObjects')
    );
    const savedObjectsMigrationConfig = await firstValueFrom(
      this.coreContext.configService.atPath<SavedObjectsMigrationConfigType>('migrations')
    );
    this.config = new SavedObjectConfig(savedObjectsConfig, savedObjectsMigrationConfig);
   // add the debug log:
    this.logger.debug(
      `SavedObjects default allowHttpApiAccess configuration: ${this.config.allowHttpApiAccess}`
    );
    deprecations.getRegistry('savedObjects').registerDeprecations(
     <...rest>

Configure logging to include the saved objects service logs:
For example:

kibana.yml

logging:
  appenders:
    so-console:
      type: console
      layout:
        type: pattern
        highlight: true
        pattern: "SAVED_OBJECTS_SERVICE--[%date][%level][%logger]---%message"
  loggers:
    - name: savedobjects-service
      level: debug
      appenders: [so-console]

Start kibana (any version of es is fine) and ensure that allowHttpApiAccess is set. For example, when run from dev the log should read something like:
SAVED_OBJECTS_SERVICE--[2023-02-21T12:06:28.561-07:00][DEBUG][savedobjects-service]---SavedObjects default allowHttpApiAccess configuration: false

In kibana.yml, specifically set savedObjects.allowHttpApiAccess: true and recheck the logs. Using the same logging config as above, the log line should now read:
SAVED_OBJECTS_SERVICE--[2023-02-21T12:06:28.561-07:00][DEBUG][savedobjects-service]---SavedObjects default allowHttpApiAccess configuration: true

TinaHeiligers

Self review on draft

TinaHeiligers · 2023-02-16T20:32:31Z

packages/core/saved-objects/core-saved-objects-base-server-internal/src/saved_objects_config.ts

@@ -42,6 +42,8 @@ export const savedObjectsMigrationConfig: ServiceConfigDescriptor<SavedObjectsMi
 const soSchema = schema.object({
  maxImportPayloadBytes: schema.byteSize({ defaultValue: 26_214_400 }),
  maxImportExportSize: schema.number({ defaultValue: 10_000 }),
+  // Conditionally set within config, dependening on the env. In prod: allow all access(default), in dev: false
+  allowHttpApiAccess: schema.boolean({ defaultValue: true }),


I've gone with defaulting to the "old" behavior where all visible types (those not hidden) use the HTTP APIs. We also need to allow access for self-managed & current cloud offering & cloud tests

Is there a specific reason to depend on the environment and not distribution?

Instead of doing the override logic between the config and the env inside the code and having to inject the env into the classes, why not just do it from the config defaults here (like the example below). This makes the logic a lot more resilient and easier to understand. wdyt?

schema.conditional( schema.contextRef('dist'), true, schema.boolean({ defaultValue: false }), schema.boolean({ defaultValue: true }) )

Is there a specific reason to depend on the environment and not distribution?

I didn't think to use the distro rather, great idea thanks!
I'll change it and see how it improves the logic.

@Bamieh I've made the change you requested in 96bb95a (#151512).
Could you take another look, please?

TinaHeiligers · 2023-02-16T20:33:39Z

packages/core/saved-objects/core-saved-objects-base-server-internal/src/saved_objects_config.ts

  public migration: SavedObjectsMigrationConfigType;

  constructor(
    rawConfig: SavedObjectsConfigType,
-    rawMigrationConfig: SavedObjectsMigrationConfigType
+    rawMigrationConfig: SavedObjectsMigrationConfigType,
+    allowHttpApiAccess: boolean


Naming is hard and hideFromHttpApis or denyHttpApiAccess sound rather negative, hence the more "positive" choice.

TinaHeiligers · 2023-02-16T20:36:09Z

packages/core/saved-objects/core-saved-objects-server-internal/src/saved_objects_service.ts

+    this.config = new SavedObjectConfig(
+      savedObjectsConfig,
+      savedObjectsMigrationConfig,
+      this.allowHttpApiAccess


I need to inject if Kibana's running in dev or prod mode from env and set the new config options from within the SO service to avoid instantiating SavedObjectConfig with:

new SavedObjectConfig(savedObjectsConfig, savedObjectsMigrationConfig, coreContext.env.mode.prod)

TinaHeiligers · 2023-02-16T20:40:02Z

src/core/server/integration_tests/saved_objects/routes/allow_api_access/bulk_create.test.ts

+  let savedObjectsClient: ReturnType<typeof savedObjectsClientMock.create>;
+  let coreUsageStatsClient: jest.Mocked<ICoreUsageStatsClient>;
+
+  beforeEach(async () => {


This test and the others in the same folder test the route behavior in a "dev" environment, were we enforce API blocking.

They're verbose to setup (copies of the main route api_integration tests) and ideally should be condensed. I tried but haven't been able to just yet.

TinaHeiligers · 2023-02-16T20:50:43Z

src/core/server/integration_tests/saved_objects/routes/routes_test_utils.ts

+
+import { SavedObjectConfig } from '@kbn/core-saved-objects-base-server-internal';
+
+export function setupConfig(allowAccess: boolean = false) {


Strictly not necessary but I'm reusing the util in almost all the route's tests

TinaHeiligers · 2023-02-16T20:51:17Z

src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker

@@ -139,6 +139,7 @@ kibana_vars=(
    regionmap
    savedObjects.maxImportExportSize
    savedObjects.maxImportPayloadBytes
+    savedObjects.allowHttpApiAccess


New SO config option

elasticmachine · 2023-02-16T22:52:13Z

Pinging @elastic/kibana-core (Team:Core)

jbudz

kibana-docker

TinaHeiligers · 2023-02-20T00:16:53Z

@elasticmachine merge upstream

…bility

TinaHeiligers · 2023-02-20T20:55:11Z

@rudolf I added you as a reviewer since Pierre is out. Could you 👁️ when you get a chance? cc @jloleysens

Bamieh

i have a couple of comments/questions

rudolf · 2023-02-21T12:04:39Z

src/core/server/integration_tests/saved_objects/routes/allow_api_access/bulk_resolve.test.ts

+    await server.stop();
+  });
+
+  it('returns with status 400 when a type is hidden from the HTTP APIs', async () => {


Suggested change

it('returns with status 400 when a type is hidden from the HTTP APIs', async () => {

it('returns with status 200 when a type is hidden from the HTTP APIs', async () => {

rudolf · 2023-02-21T12:05:46Z

src/core/server/integration_tests/saved_objects/routes/allow_api_access/bulk_get.test.ts

+    await server.stop();
+  });
+
+  it('returns with status 200 when a type is hidden from the HTTP APIs', async () => {


nit: these tests have inconsistent names across the different methods
uses config option allowHttpApiAccess to grant hiddenFromHttpApis types access
vs returns with status 200 when a type is hidden from the HTTP APIs

I prefer the latter

@rudolf change is made: 75e11c7 (#151512)

TinaHeiligers · 2023-02-21T15:46:10Z

@elasticmachine merge upstream

…bility

… from a dist or not

kibana-ci · 2023-02-21T20:23:01Z

💚 Build Succeeded

Buildkite Build
Commit: 75e11c7

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/core-saved-objects-base-server-internal`	35	36	+1

Unknown metric groups

API count

id	before	after	diff
`@kbn/core-saved-objects-base-server-internal`	46	47	+1

History

💚 Build #109298 succeeded b61d065
💚 Build #109068 succeeded 879d33b
💚 Build #108873 succeeded 497ec8a
💚 Build #108749 succeeded 99b4dcf

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Bamieh

LGTM

Adds config flag to toggle hiddenFromHttpApis SO types conditionally

76a77b4

TinaHeiligers added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:Saved Objects release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting v8.8.0 labels Feb 16, 2023

TinaHeiligers commented Feb 16, 2023

View reviewed changes

cleanup draft

99b4dcf

TinaHeiligers marked this pull request as ready for review February 16, 2023 22:52

TinaHeiligers requested review from a team as code owners February 16, 2023 22:52

jbudz approved these changes Feb 16, 2023

View reviewed changes

Merge branch 'main' into kbn-150471-toggle-hiddenFromHttpApis-accessi…

497ec8a

…bility

TinaHeiligers requested review from rudolf and jloleysens February 20, 2023 20:53

marks allowHttpApiAccess as internal

879d33b

Bamieh requested changes Feb 21, 2023

View reviewed changes

rudolf reviewed Feb 21, 2023

View reviewed changes

kibanamachine and others added 3 commits February 21, 2023 10:46

Merge branch 'main' into kbn-150471-toggle-hiddenFromHttpApis-accessi…

b61d065

…bility

default allowHttpApiAccess conditionally depending on if Kibana's run…

96bb95a

… from a dist or not

Consistently name integration tests

75e11c7

TinaHeiligers requested a review from Bamieh February 21, 2023 19:26

Bamieh approved these changes Feb 21, 2023

View reviewed changes

TinaHeiligers merged commit a8f10ed into elastic:main Feb 22, 2023

TinaHeiligers deleted the kbn-150471-toggle-hiddenFromHttpApis-accessibility branch February 22, 2023 18:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Saved Objects] Adds config flag to toggle hiddenFromHttpApis SO types conditionally #151512

[Saved Objects] Adds config flag to toggle hiddenFromHttpApis SO types conditionally #151512

TinaHeiligers commented Feb 16, 2023 •

edited

Loading

TinaHeiligers left a comment

TinaHeiligers Feb 16, 2023 •

edited

Loading

Bamieh Feb 21, 2023 •

edited

Loading

TinaHeiligers Feb 21, 2023

TinaHeiligers Feb 21, 2023

TinaHeiligers Feb 16, 2023

TinaHeiligers Feb 16, 2023 •

edited

Loading

TinaHeiligers Feb 16, 2023

TinaHeiligers Feb 16, 2023

TinaHeiligers Feb 16, 2023

elasticmachine commented Feb 16, 2023

jbudz left a comment

TinaHeiligers commented Feb 20, 2023

TinaHeiligers commented Feb 20, 2023

Bamieh left a comment

rudolf Feb 21, 2023

rudolf Feb 21, 2023

TinaHeiligers Feb 21, 2023

TinaHeiligers commented Feb 21, 2023

kibana-ci commented Feb 21, 2023

API count

Bamieh left a comment


		import { SavedObjectConfig } from '@kbn/core-saved-objects-base-server-internal';

		export function setupConfig(allowAccess: boolean = false) {

	it('returns with status 400 when a type is hidden from the HTTP APIs', async () => {
	it('returns with status 200 when a type is hidden from the HTTP APIs', async () => {

[Saved Objects] Adds config flag to toggle hiddenFromHttpApis SO types conditionally #151512

[Saved Objects] Adds config flag to toggle hiddenFromHttpApis SO types conditionally #151512

Conversation

TinaHeiligers commented Feb 16, 2023 • edited Loading

Checklist

Risk Matrix

For maintainers

How to test this

TinaHeiligers left a comment

Choose a reason for hiding this comment

TinaHeiligers Feb 16, 2023 • edited Loading

Choose a reason for hiding this comment

Bamieh Feb 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TinaHeiligers Feb 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Feb 16, 2023

jbudz left a comment

Choose a reason for hiding this comment

TinaHeiligers commented Feb 20, 2023

TinaHeiligers commented Feb 20, 2023

Bamieh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TinaHeiligers commented Feb 21, 2023

kibana-ci commented Feb 21, 2023

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

API count

History

Bamieh left a comment

Choose a reason for hiding this comment

TinaHeiligers commented Feb 16, 2023 •

edited

Loading

TinaHeiligers Feb 16, 2023 •

edited

Loading

Bamieh Feb 21, 2023 •

edited

Loading

TinaHeiligers Feb 16, 2023 •

edited

Loading