-
Notifications
You must be signed in to change notification settings - Fork 8.6k
[performance] Apply minimal auth to the search route #257497
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
Changes from all commits
2fd34a0
a11b5ff
4d76173
069b265
16ba70c
f6b28c0
6b2251d
c34791e
37bb98c
84ada1b
555fb1d
9d006d2
2b46eb7
d3ea63a
84dda73
b6353bb
440e0bf
f384a3f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -72,6 +72,7 @@ interface TrackIdQueueEntry { | |
|
|
||
| searchInfo: SearchSessionRequestInfo; | ||
| requestHash: string; | ||
| skipRealmCheck: boolean; | ||
| } | ||
|
|
||
| export class SearchSessionService implements ISearchSessionService { | ||
|
|
@@ -163,14 +164,16 @@ export class SearchSessionService implements ISearchSessionService { | |
| public get = async ( | ||
| { savedObjectsClient }: SearchSessionDependencies, | ||
| user: AuthenticatedUser | null, | ||
| sessionId: string | ||
| sessionId: string, | ||
| skipRealmCheck: boolean = false | ||
| ) => { | ||
| this.logger.debug(`get | ${sessionId}`); | ||
| const session = await savedObjectsClient.get<SearchSessionSavedObjectAttributes>( | ||
| SEARCH_SESSION_TYPE, | ||
| sessionId | ||
| ); | ||
| this.throwOnUserConflict(user, session); | ||
|
|
||
| this.throwOnUserConflict(user, session, skipRealmCheck); | ||
|
|
||
| return session; | ||
| }; | ||
|
|
@@ -226,11 +229,12 @@ export class SearchSessionService implements ISearchSessionService { | |
| deps: SearchSessionDependencies, | ||
| user: AuthenticatedUser | null, | ||
| sessionId: string, | ||
| attributes: Partial<SearchSessionSavedObjectAttributes> | ||
| attributes: Partial<SearchSessionSavedObjectAttributes>, | ||
| skipRealmCheck: boolean = false | ||
| ) => { | ||
| this.logger.debug(`SearchSessionService: update | ${sessionId}`); | ||
| if (!this.sessionConfig.enabled) throw new Error('Search sessions are disabled'); | ||
| await this.get(deps, user, sessionId); // Verify correct user | ||
| await this.get(deps, user, sessionId, skipRealmCheck); // Verify correct user | ||
| return deps.savedObjectsClient.update<SearchSessionSavedObjectAttributes>( | ||
| SEARCH_SESSION_TYPE, | ||
| sessionId, | ||
|
|
@@ -293,7 +297,8 @@ export class SearchSessionService implements ISearchSessionService { | |
| deps: SearchSessionDependencies, | ||
| user: AuthenticatedUser | null, | ||
| searchId: string, | ||
| options: ISearchOptions | ||
| options: ISearchOptions, | ||
| skipRealmCheck: boolean = false | ||
|
Contributor
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. Since
Contributor
Author
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. lol, well I got the opposite feedback above #257497 (comment) |
||
| ) => { | ||
| const { sessionId, strategy = ENHANCED_ES_SEARCH_STRATEGY, requestHash } = options; | ||
| if (!this.sessionConfig.enabled || !sessionId || !searchId) return; | ||
|
|
@@ -328,7 +333,13 @@ export class SearchSessionService implements ISearchSessionService { | |
| }, | ||
| {} | ||
| ); | ||
| this.update(queue[0].deps, queue[0].user, sessionId, { idMapping: batchedIdMapping }) | ||
| this.update( | ||
| queue[0].deps, | ||
| queue[0].user, | ||
| sessionId, | ||
| { idMapping: batchedIdMapping }, | ||
| queue[0].skipRealmCheck | ||
| ) | ||
| .then(() => { | ||
| queue.forEach((q) => q.resolve()); | ||
| }) | ||
|
|
@@ -352,6 +363,7 @@ export class SearchSessionService implements ISearchSessionService { | |
| resolve: deferred.resolve, | ||
| reject: deferred.reject, | ||
| user, | ||
| skipRealmCheck, | ||
| }); | ||
|
|
||
| scheduleProcessQueue(); | ||
|
|
@@ -450,7 +462,8 @@ export class SearchSessionService implements ISearchSessionService { | |
| deps: SearchSessionDependencies, | ||
| user: AuthenticatedUser | null, | ||
| searchRequest: IKibanaSearchRequest, | ||
| { sessionId, isStored, isRestore, requestHash }: ISearchOptions | ||
| { sessionId, isStored, isRestore, requestHash }: ISearchOptions, | ||
| skipRealmCheck: boolean = false | ||
|
Contributor
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. Same question as above, isn't this always called with |
||
| ) => { | ||
| if (!this.sessionConfig.enabled) { | ||
| throw new Error('Background search is disabled'); | ||
|
|
@@ -464,7 +477,7 @@ export class SearchSessionService implements ISearchSessionService { | |
| throw new Error('Request hash is required to get search ID from session'); | ||
| } | ||
|
|
||
| const session = await this.get(deps, user, sessionId); | ||
| const session = await this.get(deps, user, sessionId, skipRealmCheck); | ||
| if (!Object.hasOwn(session.attributes.idMapping, requestHash)) { | ||
| this.logger.debug(`SearchSessionService: getId | ${sessionId} | ${requestHash} not found`); | ||
| this.logger.error( | ||
|
|
@@ -512,14 +525,32 @@ export class SearchSessionService implements ISearchSessionService { | |
|
|
||
| private throwOnUserConflict = ( | ||
| user: AuthenticatedUser | null, | ||
| session?: SavedObject<SearchSessionSavedObjectAttributes> | ||
| session?: SavedObject<SearchSessionSavedObjectAttributes>, | ||
| skipRealmCheck: boolean = false | ||
| ) => { | ||
| if (user === null || !session) return; | ||
|
|
||
| if (skipRealmCheck) { | ||
| this.logger.debug( | ||
| `Skipping realm check for user ${user.username} when accessing search session ${session.attributes.sessionId}.` | ||
| ); | ||
| } | ||
|
|
||
| let matchesUser = true; | ||
|
|
||
| if (user.username !== session.attributes.username) { | ||
| matchesUser = false; | ||
| } | ||
|
|
||
| if ( | ||
| user.authentication_realm.type !== session.attributes.realmType || | ||
| user.authentication_realm.name !== session.attributes.realmName || | ||
| user.username !== session.attributes.username | ||
| !skipRealmCheck && | ||
| (user.authentication_realm.type !== session.attributes.realmType || | ||
| user.authentication_realm.name !== session.attributes.realmName) | ||
| ) { | ||
| matchesUser = false; | ||
| } | ||
|
|
||
| if (!matchesUser) { | ||
| this.logger.debug( | ||
| `User ${user.username} has no access to search session ${session.attributes.sessionId}` | ||
| ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -183,13 +183,6 @@ export class AuthorizationService { | |
|
|
||
| http.registerOnPreResponse(async (request, preResponse, toolkit) => { | ||
| if (preResponse.statusCode === 403) { | ||
| const user = getCurrentUser(request); | ||
| if (user?.roles.length === 0) { | ||
| this.logger.warn( | ||
| `A user authenticated with the "${user.authentication_realm.name}" (${user.authentication_realm.type}) realm doesn't have any roles and isn't authorized to perform request.` | ||
| ); | ||
| } | ||
|
Comment on lines
-186
to
-191
Contributor
Author
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. This log statement was creating a server error in some cases:
Contributor
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. Thanks! Context: "With minimal auth, we won’t always have roles available, so this message will become more and more misleading when debugging issues." |
||
|
|
||
| if (canRedirectRequest(request)) { | ||
| const next = `${http.basePath.get(request)}${request.url.pathname}${request.url.search}`; | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.