-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Security - remove auth scope provider #36998
Changes from all commits
3532524
f3bb7cc
962fdad
caa6fdd
35a5c7c
d5c1392
42f9c57
0b429ad
39d6757
909bcfe
d4a51cf
bf408a2
d6fc719
f79257a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,12 +7,14 @@ | |
import Boom from 'boom'; | ||
|
||
import { | ||
AUTH_SCOPE_DASHBORD_ONLY_MODE | ||
CONFIG_DASHBOARD_ONLY_MODE_ROLES, | ||
} from '../common'; | ||
|
||
const superuserRole = 'superuser'; | ||
|
||
/** | ||
* Intercept all requests after auth has completed and apply filtering | ||
* logic to enforce `xpack:dashboardMode` scope | ||
* logic to enforce dashboard only mode. | ||
* | ||
* @type {Hapi.RequestExtension} | ||
*/ | ||
|
@@ -25,14 +27,34 @@ export function createDashboardModeRequestInterceptor(dashboardViewerApp) { | |
type: 'onPostAuth', | ||
async method(request, h) { | ||
const { auth, url } = request; | ||
const user = auth.credentials; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As you can tell from the commit history in this PR, the ordering of operations in this function is rather fragile. Previously, this logic was part of The existing (and current) logic uses the UI Settings service to retrieve the list of dashboard-only-mode roles. The act of retrieving this setting ends up creating the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That's a bummer :/ How many (approximately) of such weird tests we have? I'm fine with leaving it as is, just curious. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It's hard to quantify this since the FTR doesn't run test suites to completion once it finds a test failure. I know we had failures in infra and canvas, but I'm sure there are a number of other impacted suites. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, thanks, that's what I wanted to know. I was just curious whether tests were failing within one "area" or not. |
||
const roles = user ? user.roles : []; | ||
|
||
if (!user) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: since we create // Clarifying comment goes here ....
const roles = (auth.credentials && auth.credentials.roles) || [];
if (roles.length === 0) {
return h.continue;
} Or the way it's done also related to these weird functional tests that depend on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yep, it was done this way because of the weird functional tests |
||
return h.continue; | ||
} | ||
|
||
const isAppRequest = url.path.startsWith('/app/'); | ||
|
||
if (isAppRequest && auth.credentials.scope && auth.credentials.scope.includes(AUTH_SCOPE_DASHBORD_ONLY_MODE)) { | ||
// The act of retrieving this setting ends up creating the config document if it doesn't already exist. | ||
// Various functional tests have come to indirectly rely on this behavior, so changing this is non-trivial. | ||
// This will be addressed once dashboard-only-mode is removed altogether. | ||
const uiSettings = request.getUiSettingsService(); | ||
const dashboardOnlyModeRoles = await uiSettings.get(CONFIG_DASHBOARD_ONLY_MODE_ROLES); | ||
|
||
if (!isAppRequest || !dashboardOnlyModeRoles || !roles || roles.length === 0) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. optional nit: I'd say the perf impact of const dashboardOnlyModeRoles = await uiSettings.get(CONFIG_DASHBOARD_ONLY_MODE_ROLES) || []; nit: also can you please leave a comment in code explaining why we always call |
||
return h.continue; | ||
} | ||
|
||
const isDashboardOnlyModeUser = user.roles.find(role => dashboardOnlyModeRoles.includes(role)); | ||
const isSuperUser = user.roles.find(role => role === superuserRole); | ||
|
||
const enforceDashboardOnlyMode = isDashboardOnlyModeUser && !isSuperUser; | ||
if (enforceDashboardOnlyMode) { | ||
if (url.path.startsWith('/app/kibana')) { | ||
// If the user is in "Dashboard only mode" they should only be allowed to see | ||
// that app and none others. Here we are intercepting all other routing and ensuring the viewer | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. uh oh: we have a link to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ha, good catch, will remove |
||
// app is the only one ever rendered. | ||
// Read more about Dashboard Only Mode here: https://github.com/elastic/x-pack-kibana/issues/180 | ||
const response = await h.renderApp(dashboardViewerApp); | ||
return response.takeover(); | ||
} | ||
|
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.
nit: can you please also get rid of
AUTH_SCOPE_DASHBORD_ONLY_MODE
inx-pack/plugins/dashboard_mode/common/constants.js
?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 can't believe I missed that! will do!