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

Migrate authorization subsystem to the new platform. #46145

Merged
merged 17 commits into from
Nov 12, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 4 additions & 1 deletion src/core/server/http/http_server.mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ interface RequestFixtureOptions {
path?: string;
method?: RouteMethod;
socket?: Socket;
routeTags?: string[];
}

function createKibanaRequestMock({
Expand All @@ -49,6 +50,7 @@ function createKibanaRequestMock({
query = {},
method = 'get',
socket = new Socket(),
routeTags,
}: RequestFixtureOptions = {}) {
const queryString = querystring.stringify(query);
return KibanaRequest.from(
Expand All @@ -61,10 +63,11 @@ function createKibanaRequestMock({
method,
url: {
path,
pathname: path,
Copy link
Member Author

Choose a reason for hiding this comment

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

note: theoretically we should separate path and pathname, but I thought it'd be okay to not do that for mock.

query: queryString,
search: queryString ? `?${queryString}` : queryString,
},
route: { settings: {} },
route: { settings: { tags: routeTags } },
raw: {
req: { socket },
},
Expand Down
1 change: 1 addition & 0 deletions src/core/server/saved_objects/service/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ export interface SavedObjectsLegacyService<Request = any> {
addScopedSavedObjectsClientWrapperFactory: SavedObjectsClientProvider<
Request
>['addClientWrapperFactory'];
setScopedSavedObjectsClientFactory: SavedObjectsClientProvider<Request>['setClientFactory'];
getScopedSavedObjectsClient: SavedObjectsClientProvider<Request>['getClient'];
SavedObjectsClient: typeof SavedObjectsClient;
types: string[];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ export class SavedObjectsClientProvider<Request = unknown> {
this._wrapperFactories.add(priority, { id, factory });
}

setClientFactory(customClientFactory: SavedObjectsClientFactory) {
setClientFactory(customClientFactory: SavedObjectsClientFactory<Request>) {
if (this._clientFactory !== this._originalClientFactory) {
throw new Error(`custom client factory is already set, unable to replace the current one`);
}
Expand Down
2 changes: 2 additions & 0 deletions src/core/server/server.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -1492,6 +1492,8 @@ export interface SavedObjectsLegacyService<Request = any> {
// (undocumented)
schema: SavedObjectsSchema;
// (undocumented)
setScopedSavedObjectsClientFactory: SavedObjectsClientProvider<Request>['setClientFactory'];
// (undocumented)
types: string[];
}

Expand Down
2 changes: 1 addition & 1 deletion x-pack/legacy/plugins/actions/server/shim.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ export interface KibanaConfig {
*/
export type TaskManagerStartContract = Pick<TaskManager, 'schedule' | 'fetch' | 'remove'>;
export type XPackMainPluginSetupContract = Pick<XPackMainPlugin, 'registerFeature'>;
export type SecurityPluginSetupContract = Pick<SecurityPlugin, 'config' | 'registerLegacyAPI'>;
export type SecurityPluginSetupContract = Pick<SecurityPlugin, '__legacyCompat'>;
Copy link
Member Author

@azasypkin azasypkin Oct 29, 2019

Choose a reason for hiding this comment

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

note: not completely sure we need this type at all.

export type SecurityPluginStartContract = Pick<SecurityPlugin, 'authc'>;
export type TaskManagerSetupContract = Pick<
TaskManager,
Expand Down
2 changes: 1 addition & 1 deletion x-pack/legacy/plugins/alerting/server/shim.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ export interface Server extends Legacy.Server {
* Shim what we're thinking setup and start contracts will look like
*/
export type TaskManagerStartContract = Pick<TaskManager, 'schedule' | 'fetch' | 'remove'>;
export type SecurityPluginSetupContract = Pick<SecurityPlugin, 'config' | 'registerLegacyAPI'>;
export type SecurityPluginSetupContract = Pick<SecurityPlugin, '__legacyCompat'>;
export type SecurityPluginStartContract = Pick<SecurityPlugin, 'authc'>;
export type XPackMainPluginSetupContract = Pick<XPackMainPlugin, 'registerFeature'>;
export type TaskManagerSetupContract = Pick<
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,16 @@ export const getCustomLogo = async ({
// We use the basePath from the saved job, which we'll have post spaces being implemented;
// or we use the server base path, which uses the default space
getBasePath: () => job.basePath || serverBasePath,
path: '/',
Copy link
Member Author

Choose a reason for hiding this comment

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

note: this is the ugly hack copied from other plugin. There are two points why I still prefer to have it though:

  • It allows Security plugin to completely migrate to KibanaRequest, convert LegacyRequest to KibanaRequest only at the LP <-> NP boundaries and completely get rid of dependency on FakeRequest
  • The fakeRequest is already super hacky, even without this change

Copy link
Contributor

Choose a reason for hiding this comment

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

🤷‍♂ :ostrich_head_in_sand:

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I know 😞 But I'd rather be radical and fail if we have another place where we provide less "advanced" fake request or if it happens that these set of params isn't enough to construct KibanaRequest anymore. It feels this way we can get rid of this fake/legacy requests earlier.

The major risk here is that if we have some code that conditionally constructs fake requests that can't be converted into KibanaRequest. And if there is no enough test coverage the problem may manifest itself only in production. To partially mitigate this problem and since core still supports FakeRequests I'm wondering if core could expose utility to construct fake requests that's fully compatible with its very own KibanaRequest.from and protected by the unit test. @elastic/kibana-platform what do you think?

route: { settings: {} },
url: {
href: '/',
},
raw: {
req: {
url: '/',
},
},
};

const savedObjects = server.savedObjects;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,16 @@ function executeJobFn(server) {
// We use the basePath from the saved job, which we'll have post spaces being implemented;
// or we use the server base path, which uses the default space
getBasePath: () => basePath || serverBasePath,
path: '/',
route: { settings: {} },
url: {
href: '/',
},
raw: {
req: {
url: '/',
},
},
};

const callEndpoint = (endpoint, clientParams = {}, options = {}) => {
Expand Down
22 changes: 9 additions & 13 deletions x-pack/legacy/plugins/reporting/server/lib/get_user.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,23 +5,19 @@
*/

import { oncePerServer } from './once_per_server';
import { getClient as getShieldClient } from '../../../../server/lib/get_client_shield';

function getUserFn(server) {
const callShieldWithRequest = getShieldClient(server).callWithRequest;

return async function getUser(request) {
const xpackInfo = server.plugins.xpack_main.info;
if (xpackInfo && xpackInfo.isAvailable() && xpackInfo.feature('security').isEnabled()) {
try {
return await callShieldWithRequest(request, 'shield.authenticate');
} catch (err) {
server.log(['reporting', 'getUser', 'debug'], err);
return null;
}
return async request => {
if (!server.plugins.security) {
return null;
}

return null;
try {
return await server.plugins.security.getUser(request);
} catch (err) {
server.log(['reporting', 'getUser', 'debug'], err);
return null;
}
};
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ describe('authorized_user_pre_routing', function () {
// so createMockServer reuses the same 'instance' of the server and overwrites
// the properties to contain different values
const createMockServer = (function () {
const callWithRequestStub = sinon.stub();
const getUserStub = sinon.stub();
let mockConfig;

const mockServer = {
Expand All @@ -30,13 +30,7 @@ describe('authorized_user_pre_routing', function () {
log: function () {},
plugins: {
xpack_main: {},
elasticsearch: {
createCluster: function () {
return {
callWithRequest: callWithRequestStub
};
}
}
security: { getUser: getUserStub },
}
};

Expand All @@ -57,8 +51,8 @@ describe('authorized_user_pre_routing', function () {
}
};

callWithRequestStub.resetHistory();
callWithRequestStub.returns(Promise.resolve(user));
getUserStub.resetHistory();
getUserStub.resolves(user);
return mockServer;
};
}());
Expand Down
4 changes: 0 additions & 4 deletions x-pack/legacy/plugins/security/common/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,4 @@
* you may not use this file except in compliance with the Elastic License.
*/

export const GLOBAL_RESOURCE = '*';
export const IGNORED_TYPES = ['space'];
azasypkin marked this conversation as resolved.
Show resolved Hide resolved
export const APPLICATION_PREFIX = 'kibana-';
export const RESERVED_PRIVILEGES_APPLICATION_WILDCARD = 'kibana-*';
export const INTERNAL_API_BASE_PATH = '/internal/security';
1 change: 0 additions & 1 deletion x-pack/legacy/plugins/security/common/login_state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,4 @@ export type LoginLayout = 'form' | 'error-es-unavailable' | 'error-xpack-unavail
export interface LoginState {
layout: LoginLayout;
allowLogin: boolean;
loginMessage: string;
azasypkin marked this conversation as resolved.
Show resolved Hide resolved
}
17 changes: 11 additions & 6 deletions x-pack/legacy/plugins/security/common/model/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,19 @@
* you may not use this file except in compliance with the Elastic License.
*/

export { Role, RoleIndexPrivilege, RoleKibanaPrivilege } from './role';
export { FeaturesPrivileges } from './features_privileges';
export { RawKibanaPrivileges, RawKibanaFeaturePrivileges } from './raw_kibana_privileges';
export { KibanaPrivileges } from './kibana_privileges';
export { ApiKey } from './api_key';
export { User, EditUser, getUserDisplayName } from '../../../../../plugins/security/common/model';
export {
AuthenticatedUser,
BuiltinESPrivileges,
EditUser,
FeaturesPrivileges,
KibanaPrivileges,
RawKibanaFeaturePrivileges,
RawKibanaPrivileges,
Role,
RoleIndexPrivilege,
RoleKibanaPrivilege,
User,
canUserChangePassword,
getUserDisplayName,
} from '../../../../../plugins/security/common/model';
export { BuiltinESPrivileges } from './builtin_es_privileges';
2 changes: 0 additions & 2 deletions x-pack/legacy/plugins/security/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,10 @@

import { Legacy } from 'kibana';
import { AuthenticatedUser } from './common/model';
import { AuthorizationService } from './server/lib/authorization/service';

/**
* Public interface of the security plugin.
*/
export interface SecurityPlugin {
authorization: Readonly<AuthorizationService>;
azasypkin marked this conversation as resolved.
Show resolved Hide resolved
getUser: (request: Legacy.Request) => Promise<AuthenticatedUser>;
}
Loading