Skip to content

Commit b8a316d

Browse files
nibixnibix
committed
Cleaned up use of PrivilegesEvaluatorResponse
Signed-off-by: Nils Bandener <[email protected]>
1 parent 5212eed commit b8a316d

14 files changed

+362
-253
lines changed

‎src/main/java/org/opensearch/security/filter/SecurityFilter.java‎

Lines changed: 8 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -402,18 +402,14 @@ private <Request extends ActionRequest, Response extends ActionResponse> void ap
402402
User finalUser = user;
403403
Consumer<PrivilegesEvaluatorResponse> handleUnauthorized = response -> {
404404
auditLog.logMissingPrivileges(action, request, task);
405-
String err;
406-
if (!response.getMissingSecurityRoles().isEmpty()) {
407-
err = String.format("No mapping for %s on roles %s", finalUser, response.getMissingSecurityRoles());
408-
} else {
409-
err = (injectedRoles != null)
410-
? String.format(
411-
"no permissions for %s and associated roles %s",
412-
response.getMissingPrivileges(),
413-
context.getMappedRoles()
414-
)
415-
: String.format("no permissions for %s and %s", response.getMissingPrivileges(), finalUser);
416-
}
405+
String err = (injectedRoles != null)
406+
? String.format(
407+
"no permissions for %s and associated roles %s",
408+
response.getMissingPrivileges(),
409+
context.getMappedRoles()
410+
)
411+
: String.format("no permissions for %s and %s", response.getMissingPrivileges(), finalUser);
412+
417413
log.debug(err);
418414
listener.onFailure(new OpenSearchSecurityException(err, RestStatus.FORBIDDEN));
419415
};

‎src/main/java/org/opensearch/security/filter/SecurityRestFilter.java‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -282,7 +282,7 @@ void authorizeRequest(RestHandler original, SecurityRequestChannel request, User
282282
.findFirst();
283283
final boolean routeSupportsRestAuthorization = handler.isPresent() && handler.get() instanceof NamedRoute;
284284
if (routeSupportsRestAuthorization) {
285-
PrivilegesEvaluatorResponse pres = new PrivilegesEvaluatorResponse();
285+
PrivilegesEvaluatorResponse pres;
286286
NamedRoute route = ((NamedRoute) handler.get());
287287
// Check both route.actionNames() and route.name(). The presence of either is sufficient.
288288
Set<String> actionNames = ImmutableSet.<String>builder()
@@ -299,12 +299,7 @@ void authorizeRequest(RestHandler original, SecurityRequestChannel request, User
299299
auditLog.logGrantedPrivileges(user.getName(), request);
300300
} else {
301301
auditLog.logMissingPrivileges(route.name(), user.getName(), request);
302-
String err;
303-
if (!pres.getMissingSecurityRoles().isEmpty()) {
304-
err = String.format("No mapping for %s on roles %s", user, pres.getMissingSecurityRoles());
305-
} else {
306-
err = String.format("no permissions for %s and %s", pres.getMissingPrivileges(), user);
307-
}
302+
String err = String.format("no permissions for %s and %s", pres.getMissingPrivileges(), user);
308303
log.debug(err);
309304

310305
request.queueForSending(new SecurityResponse(HttpStatus.SC_UNAUTHORIZED, err));

‎src/main/java/org/opensearch/security/privileges/PitPrivilegesEvaluator.java‎

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -40,12 +40,11 @@ public PrivilegesEvaluatorResponse evaluate(
4040
final PrivilegesEvaluationContext context,
4141
final ActionPrivileges actionPrivileges,
4242
final String action,
43-
final PrivilegesEvaluatorResponse presponse,
4443
final IndexResolverReplacer irr
4544
) {
4645

4746
if (!(request instanceof DeletePitRequest || request instanceof PitSegmentsRequest)) {
48-
return presponse;
47+
return null;
4948
}
5049
List<String> pitIds = new ArrayList<>();
5150

@@ -58,9 +57,9 @@ public PrivilegesEvaluatorResponse evaluate(
5857
}
5958
// if request is for all PIT IDs, skip custom pit ids evaluation
6059
if (pitIds.size() == 1 && "_all".equals(pitIds.get(0))) {
61-
return presponse;
60+
return null;
6261
} else {
63-
return handlePitsAccess(pitIds, context, actionPrivileges, action, presponse, irr);
62+
return handlePitsAccess(pitIds, context, actionPrivileges, action, irr);
6463
}
6564
}
6665

@@ -72,7 +71,6 @@ private PrivilegesEvaluatorResponse handlePitsAccess(
7271
PrivilegesEvaluationContext context,
7372
ActionPrivileges actionPrivileges,
7473
final String action,
75-
PrivilegesEvaluatorResponse presponse,
7674
final IndexResolverReplacer irr
7775
) {
7876
Map<String, String[]> pitToIndicesMap = OpenSearchSecurityPlugin.GuiceHolder.getPitService().getIndicesForPits(pitIds);
@@ -87,10 +85,9 @@ private PrivilegesEvaluatorResponse handlePitsAccess(
8785
PrivilegesEvaluatorResponse subResponse = actionPrivileges.hasIndexPrivilege(context, ImmutableSet.of(action), pitResolved);
8886
// Only if user has access to all PIT's indices, allow operation, otherwise continue evaluation in PrivilegesEvaluator.
8987
if (subResponse.isAllowed()) {
90-
presponse.allowed = true;
91-
presponse.markComplete();
88+
return PrivilegesEvaluatorResponse.ok();
9289
}
9390

94-
return presponse;
91+
return null;
9592
}
9693
}

‎src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorImpl.java‎

Lines changed: 19 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -307,7 +307,7 @@ public PrivilegesEvaluatorResponse evaluate(PrivilegesEvaluationContext context)
307307
action0 = PutMappingAction.NAME;
308308
}
309309

310-
PrivilegesEvaluatorResponse presponse = new PrivilegesEvaluatorResponse();
310+
PrivilegesEvaluatorResponse presponse;
311311

312312
final boolean isDebugEnabled = log.isDebugEnabled();
313313
if (isDebugEnabled) {
@@ -328,7 +328,7 @@ public PrivilegesEvaluatorResponse evaluate(PrivilegesEvaluationContext context)
328328

329329
presponse = actionPrivileges.hasClusterPrivilege(context, action0);
330330

331-
if (!presponse.allowed) {
331+
if (!presponse.isAllowed()) {
332332
log.info(
333333
"No cluster-level perm match for {} [Action [{}]] [RolesChecked {}]. No permissions for {}",
334334
user,
@@ -347,23 +347,26 @@ public PrivilegesEvaluatorResponse evaluate(PrivilegesEvaluationContext context)
347347
// check snapshot/restore requests
348348
// NOTE: Has to go first as restore request could be for protected and/or system indices and the request may
349349
// fail with 403 if system index or protected index evaluators are triggered first
350-
if (snapshotRestoreEvaluator.evaluate(request, task, action0, presponse).isComplete()) {
350+
presponse = snapshotRestoreEvaluator.evaluate(request, task, action0);
351+
if (presponse != null) {
351352
return presponse;
352353
}
353354

354355
// System index access
355-
if (systemIndexAccessEvaluator.evaluate(request, task, action0, requestedResolved, presponse, context, actionPrivileges, user)
356-
.isComplete()) {
356+
presponse = systemIndexAccessEvaluator.evaluate(request, task, action0, requestedResolved, context, actionPrivileges, user);
357+
if (presponse != null) {
357358
return presponse;
358359
}
359360

360361
// Protected index access
361-
if (protectedIndexAccessEvaluator.evaluate(request, task, action0, requestedResolved, presponse, mappedRoles).isComplete()) {
362+
presponse = protectedIndexAccessEvaluator.evaluate(request, task, action0, requestedResolved, mappedRoles);
363+
if (presponse != null) {
362364
return presponse;
363365
}
364366

365367
// check access for point in time requests
366-
if (pitPrivilegesEvaluator.evaluate(request, context, actionPrivileges, action0, presponse, irr).isComplete()) {
368+
presponse = pitPrivilegesEvaluator.evaluate(request, context, actionPrivileges, action0, irr);
369+
if (presponse != null) {
367370
return presponse;
368371
}
369372

@@ -380,7 +383,7 @@ public PrivilegesEvaluatorResponse evaluate(PrivilegesEvaluationContext context)
380383

381384
presponse = actionPrivileges.hasClusterPrivilege(context, action0);
382385

383-
if (!presponse.allowed) {
386+
if (!presponse.isAllowed()) {
384387
log.info(
385388
"No cluster-level perm match for {} {} [Action [{}]] [RolesChecked {}]. No permissions for {}",
386389
user,
@@ -411,29 +414,27 @@ public PrivilegesEvaluatorResponse evaluate(PrivilegesEvaluationContext context)
411414
if (!replaceResult.continueEvaluation) {
412415
if (replaceResult.accessDenied) {
413416
auditLog.logMissingPrivileges(action0, request, task);
417+
return PrivilegesEvaluatorResponse.insufficient(action0);
414418
} else {
415-
presponse.allowed = true;
416-
presponse.createIndexRequestBuilder = replaceResult.createIndexRequestBuilder;
419+
return PrivilegesEvaluatorResponse.ok().with(replaceResult.createIndexRequestBuilder);
417420
}
418-
return presponse;
419421
}
420422
}
421423

422424
log.debug("Allowed because we have cluster permissions for {}", action0);
423425

424-
presponse.allowed = true;
425-
return presponse;
426+
return PrivilegesEvaluatorResponse.ok();
426427
}
427428
}
428429
}
429430

430431
if (checkDocAllowListHeader(user, action0, request)) {
431-
presponse.allowed = true;
432-
return presponse;
432+
return PrivilegesEvaluatorResponse.ok();
433433
}
434434

435435
// term aggregations
436-
if (termsAggregationEvaluator.evaluate(requestedResolved, request, context, actionPrivileges, presponse).isComplete()) {
436+
presponse = termsAggregationEvaluator.evaluate(requestedResolved, request, context, actionPrivileges);
437+
if (presponse != null) {
437438
return presponse;
438439
}
439440

@@ -462,9 +463,7 @@ public PrivilegesEvaluatorResponse evaluate(PrivilegesEvaluationContext context)
462463
auditLog.logMissingPrivileges(action0, request, task);
463464
return PrivilegesEvaluatorResponse.insufficient(action0);
464465
} else {
465-
presponse.allowed = true;
466-
presponse.createIndexRequestBuilder = replaceResult.createIndexRequestBuilder;
467-
return presponse;
466+
return PrivilegesEvaluatorResponse.ok().with(replaceResult.createIndexRequestBuilder);
468467
}
469468
}
470469
}
@@ -497,8 +496,7 @@ public PrivilegesEvaluatorResponse evaluate(PrivilegesEvaluationContext context)
497496

498497
if (presponse.isAllowed()) {
499498
if (checkFilteredAliases(requestedResolved, action0, isDebugEnabled)) {
500-
presponse.allowed = false;
501-
return presponse;
499+
return PrivilegesEvaluatorResponse.insufficient(action0);
502500
}
503501

504502
log.debug("Allowed because we have all indices permissions for {}", action0);

0 commit comments

Comments
 (0)