diff --git a/DEVELOPER_GUIDE.md b/DEVELOPER_GUIDE.md index c2388af7d..8facc02ec 100644 --- a/DEVELOPER_GUIDE.md +++ b/DEVELOPER_GUIDE.md @@ -65,6 +65,12 @@ opensearch.requestHeadersWhitelist: [ authorization,securitytenant ] opensearch_security.multitenancy.enabled: true opensearch_security.multitenancy.tenants.preferred: ["Private", "Global"] opensearch_security.readonly_mode.roles: ["kibana_read_only"] +# Optional — opt in to letting read-only users access the Discover app. +# Defaults to false to preserve existing behavior on upgrade. +# opensearch_security.readonly_mode.allow_discover: true +# Optional — opt in to letting read-only users access the Visualize app. +# Defaults to false to preserve existing behavior on upgrade. +# opensearch_security.readonly_mode.allow_visualize: true # Use this setting if you are running opensearch-dashboards without https opensearch_security.cookie.secure: false diff --git a/public/plugin.ts b/public/plugin.ts index 3bd02784d..282041348 100644 --- a/public/plugin.ts +++ b/public/plugin.ts @@ -88,6 +88,20 @@ async function hasApiPermission(core: CoreSetup): Promise { const DEFAULT_READONLY_ROLES = ['kibana_read_only']; const APP_ID_HOME = 'home'; const APP_ID_DASHBOARDS = 'dashboards'; +// Modern OSD registers Discover twice, on purpose, and both ids must be +// allowlisted together to avoid breaking the UI: +// - `data-explorer` hosts the real Discover UI at /app/data-explorer/discover. +// - `discover` is a thin shim that client-side-redirects to data-explorer +// (see src/plugins/discover/public/plugin.ts). Its mount function fires +// navigateToApp('data-explorer', ...) — but the AppUpdater status check +// runs BEFORE mount, so if `discover` is marked inaccessible the user +// sees "Application not found" and the redirect never executes. +// - The nav menu's "Discover" entry points at the `discover` id, not +// `data-explorer`, so blocking `discover` also breaks the nav link. +// Dropping either id produces a visibly broken Discover for the read-only user. +const APP_ID_DISCOVER = 'discover'; +const APP_ID_DATA_EXPLORER = 'data-explorer'; +const APP_ID_VISUALIZE = 'visualize'; // OpenSearchDashboards app is for legacy url migration const APP_ID_OPENSEARCH_DASHBOARDS = 'kibana'; const APP_LIST_FOR_READONLY_ROLE = [APP_ID_HOME, APP_ID_DASHBOARDS, APP_ID_OPENSEARCH_DASHBOARDS]; @@ -431,9 +445,15 @@ export class SecurityPlugin }); } + const readonlyAppAllowlist = [ + ...APP_LIST_FOR_READONLY_ROLE, + ...(config.readonly_mode?.allow_discover ? [APP_ID_DISCOVER, APP_ID_DATA_EXPLORER] : []), + ...(config.readonly_mode?.allow_visualize ? [APP_ID_VISUALIZE] : []), + ]; + core.application.registerAppUpdater( new BehaviorSubject((app) => { - if (!apiPermission && isReadonly && !APP_LIST_FOR_READONLY_ROLE.includes(app.id)) { + if (!apiPermission && isReadonly && !readonlyAppAllowlist.includes(app.id)) { return { status: AppStatus.inaccessible, }; diff --git a/public/test/plugin.test.ts b/public/test/plugin.test.ts index e2bb1a836..21bfcb307 100644 --- a/public/test/plugin.test.ts +++ b/public/test/plugin.test.ts @@ -40,6 +40,17 @@ import { PLUGIN_USERS_APP_ID, } from '../../common/index.ts'; +jest.mock('../apps/account/utils', () => ({ + fetchAccountInfoSafe: jest.fn(), +})); +jest.mock('../utils/dashboards-info-utils', () => ({ + getDashboardsInfoSafe: jest.fn(), +})); +// eslint-disable-next-line @typescript-eslint/no-var-requires +const { fetchAccountInfoSafe } = require('../apps/account/utils'); +// eslint-disable-next-line @typescript-eslint/no-var-requires +const { getDashboardsInfoSafe } = require('../utils/dashboards-info-utils'); + // Mock the hasApiPermission function jest.mock('../plugin', () => { const originalModule = jest.requireActual('../plugin'); @@ -153,6 +164,142 @@ describe('SecurityPlugin', () => { }); }); + describe('readonly app allowlist — opt-in flags', () => { + const makeContext = (flags: { allow_discover?: boolean; allow_visualize?: boolean }) => ({ + config: { + get: jest.fn().mockReturnValue({ + readonly_mode: { + roles: ['kibana_read_only'], + ...flags, + }, + multitenancy: { enabled: true, enable_aggregation_view: false }, + clusterPermissions: { include: [] }, + indexPermissions: { include: [] }, + disabledTransportCategories: { exclude: [] }, + disabledRestCategories: { exclude: [] }, + ui: { autologout: false }, + }), + }, + }); + + const runAndGetUpdater = async (flags: { + allow_discover?: boolean; + allow_visualize?: boolean; + }) => { + // Force hasApiPermission() to resolve false so the readonly updater engages. + coreSetup.http.get = jest.fn().mockResolvedValue({ has_api_access: false }); + (fetchAccountInfoSafe as jest.Mock).mockResolvedValue({ + data: { roles: ['kibana_read_only'] }, + }); + (getDashboardsInfoSafe as jest.Mock).mockResolvedValue({ + multitenancy_enabled: true, + }); + coreSetup.chrome.navGroup = { + ...coreSetup.chrome.navGroup, + getNavGroupEnabled: () => false, + }; + + const registerAppUpdaterSpy = jest.spyOn(coreSetup.application, 'registerAppUpdater'); + const callsBefore = registerAppUpdaterSpy.mock.calls.length; + plugin = new SecurityPlugin(makeContext(flags)); + await plugin.setup(coreSetup, deps); + + // Pick the subject registered during THIS setup — coreSetup is reused across + // runAndGetUpdater() calls within a single test, so spy.mock.calls accumulates. + const subject = registerAppUpdaterSpy.mock.calls[callsBefore][0]; + return subject.getValue(); + }; + + // The updater returns an object with a `status` field (AppStatus.inaccessible) + // when blocking an app, and `undefined` when allowing. Assert on shape so this + // test file does not need to import from `src/core/public` (which pulls in + // monaco-editor and fails under jsdom at module-load time). + const expectBlocked = (result: unknown) => { + expect(result).toEqual(expect.objectContaining({ status: expect.anything() })); + }; + + describe('allow_discover flag', () => { + it('blocks Discover (both ids) when the flag is unset (default off)', async () => { + const updater = await runAndGetUpdater({}); + expectBlocked(updater({ id: 'discover' })); + expectBlocked(updater({ id: 'data-explorer' })); + }); + + it('blocks Discover (both ids) when the flag is explicitly false', async () => { + const updater = await runAndGetUpdater({ allow_discover: false }); + expectBlocked(updater({ id: 'discover' })); + expectBlocked(updater({ id: 'data-explorer' })); + }); + + it('allows Discover (both legacy and data-explorer ids) when the flag is true', async () => { + const updater = await runAndGetUpdater({ allow_discover: true }); + expect(updater({ id: 'discover' })).toBeUndefined(); + expect(updater({ id: 'data-explorer' })).toBeUndefined(); + }); + + it('still blocks other non-allowlisted apps when the flag is true', async () => { + const updater = await runAndGetUpdater({ allow_discover: true }); + expectBlocked(updater({ id: 'visualize' })); + expectBlocked(updater({ id: 'management' })); + }); + }); + + describe('allow_visualize flag', () => { + it('blocks Visualize when the flag is unset (default off)', async () => { + const updater = await runAndGetUpdater({}); + expectBlocked(updater({ id: 'visualize' })); + }); + + it('blocks Visualize when the flag is explicitly false', async () => { + const updater = await runAndGetUpdater({ allow_visualize: false }); + expectBlocked(updater({ id: 'visualize' })); + }); + + it('allows Visualize when the flag is true', async () => { + const updater = await runAndGetUpdater({ allow_visualize: true }); + expect(updater({ id: 'visualize' })).toBeUndefined(); + }); + + it('still blocks other non-allowlisted apps when the flag is true', async () => { + const updater = await runAndGetUpdater({ allow_visualize: true }); + expectBlocked(updater({ id: 'discover' })); + expectBlocked(updater({ id: 'data-explorer' })); + expectBlocked(updater({ id: 'management' })); + }); + + it('does not affect Discover gating (flags are independent)', async () => { + const discoverOnly = await runAndGetUpdater({ allow_discover: true }); + expectBlocked(discoverOnly({ id: 'visualize' })); + + const visualizeOnly = await runAndGetUpdater({ allow_visualize: true }); + expectBlocked(visualizeOnly({ id: 'discover' })); + }); + + it('allows both apps when both flags are true', async () => { + const updater = await runAndGetUpdater({ allow_discover: true, allow_visualize: true }); + expect(updater({ id: 'discover' })).toBeUndefined(); + expect(updater({ id: 'data-explorer' })).toBeUndefined(); + expect(updater({ id: 'visualize' })).toBeUndefined(); + }); + }); + + it('keeps baseline allowlist members (home, dashboards) accessible regardless of flags', async () => { + const combos = [ + {}, + { allow_discover: false }, + { allow_discover: true }, + { allow_visualize: false }, + { allow_visualize: true }, + { allow_discover: true, allow_visualize: true }, + ]; + for (const flags of combos) { + const updater = await runAndGetUpdater(flags); + expect(updater({ id: 'home' })).toBeUndefined(); + expect(updater({ id: 'dashboards' })).toBeUndefined(); + } + }); + }); + it('does not call register function for tenant app when multitenancy is off', async () => { // Mock hasApiPermission to return true pluginModule.hasApiPermission.mockResolvedValue(true); diff --git a/public/types.ts b/public/types.ts index 93d2f3514..f54434935 100644 --- a/public/types.ts +++ b/public/types.ts @@ -63,6 +63,8 @@ export interface DashboardsInfo { export interface ClientConfigType { readonly_mode: { roles: string[]; + allow_discover?: boolean; + allow_visualize?: boolean; }; ui: { basicauth: { diff --git a/server/index.ts b/server/index.ts index 51397735d..dda929319 100644 --- a/server/index.ts +++ b/server/index.ts @@ -45,6 +45,8 @@ export const configSchema = schema.object({ allow_client_certificates: schema.boolean({ defaultValue: false }), readonly_mode: schema.object({ roles: schema.arrayOf(schema.string(), { defaultValue: [] }), + allow_discover: schema.boolean({ defaultValue: false }), + allow_visualize: schema.boolean({ defaultValue: false }), }), clusterPermissions: schema.object({ include: schema.arrayOf(schema.string(), { defaultValue: [] }),