Skip to content

Commit

Permalink
Replace searchShadowOwner with equivalent search
Browse files Browse the repository at this point in the history
The original searchShadowOwner execution was not cacheable
in global nor in local repository caches, resulting in excessive
number of search operations e.g. when authorizations were present.

Now the method is replaced by equivalent searchObjects call
at the level of RepositoryService, so that caching should be employed.
(Note that the translation to regular search operation was there
internally in SqaleRepositoryService implementation perhaps from the
day one.)
  • Loading branch information
mederly committed Dec 7, 2022
1 parent f45a5bd commit b9bca18
Show file tree
Hide file tree
Showing 10 changed files with 191 additions and 114 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -393,11 +393,6 @@ public <T extends ObjectType> DeleteObjectResult deleteObject(Class<T> type, Str
return null;
}

@Override
public <F extends FocusType> PrismObject<F> searchShadowOwner(String shadowOid, Collection<SelectorOptions<GetOperationOptions>> options, OperationResult parentResult) {
return null;
}

@Override
public long advanceSequence(String oid, OperationResult parentResult) {
return 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -392,7 +392,7 @@ public void test103GetAccountRaw() throws Exception {

// Just to avoid result pruning.
CompiledTracingProfile tracingProfile = tracer.compileProfile(
new TracingProfileType(prismContext)
new TracingProfileType()
.createTraceFile(false), parentResult);
OperationResult result = parentResult.subresult("get")
.tracingProfile(tracingProfile)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
*/
package com.evolveum.midpoint.model.intest.security;

import static org.assertj.core.api.Assertions.assertThat;
import static org.testng.AssertJUnit.*;

import java.io.File;
Expand All @@ -15,8 +16,13 @@
import java.util.stream.Collectors;
import javax.xml.datatype.XMLGregorianCalendar;

import com.evolveum.midpoint.repo.api.perf.PerformanceInformation;
import com.evolveum.midpoint.repo.api.perf.PerformanceMonitor;
import com.evolveum.midpoint.schema.statistics.RepositoryPerformanceInformationUtil;
import com.evolveum.midpoint.test.TestResource;

import com.evolveum.midpoint.util.MiscUtil;

import org.springframework.test.annotation.DirtiesContext;
import org.springframework.test.annotation.DirtiesContext.ClassMode;
import org.springframework.test.context.ContextConfiguration;
Expand Down Expand Up @@ -123,6 +129,9 @@ public class TestSecurityAdvanced extends AbstractSecurityTest {
private static final TestResource<RoleType> ROLE_READ_TASK_STATUS = new TestResource<>(TEST_DIR, "role-read-task-status.xml", "bc2d0900-ac17-40c1-acf8-eb5466995aae");
private static final TestResource<TaskType> TASK_DUMMY = new TestResource<>(TEST_DIR, "task-dummy.xml", "89bf08ec-c5b8-4641-95ca-37559c1f3896");

private static final TestResource<RoleType> ROLE_MANY_SHADOW_OWNER_AUTZ = new TestResource<>(
TEST_DIR, "role-many-shadow-owner-autz.xml", "c8c99194-3e5c-439b-bf98-c71146d3e1b5");

@Override
public void initSystem(Task initTask, OperationResult initResult) throws Exception {
super.initSystem(initTask, initResult);
Expand Down Expand Up @@ -151,9 +160,10 @@ public void initSystem(Task initTask, OperationResult initResult) throws Excepti
repoAddObjectFromFile(ROLE_READ_RESOURCE_OPERATIONAL_STATE_FILE, initResult);
repoAdd(ROLE_READ_TASK_STATUS, initResult);
repoAdd(TASK_DUMMY, initResult);
repoAdd(ROLE_MANY_SHADOW_OWNER_AUTZ, initResult);
}

private static final int NUMBER_OF_IMPORTED_ROLES = 20;
private static final int NUMBER_OF_IMPORTED_ROLES = 21;
private static final int NUMBER_OF_IMPORTED_TASKS = 1;

protected int getNumberOfRoles() {
Expand Down Expand Up @@ -3219,6 +3229,41 @@ public void test360AutzJackTaskRead() throws Exception {
assertEquals("Wrong # of items in task read", 2, task.getValue().size());
}

/**
* Checks the caching of "owner search" operation during autz checking (MID-8363).
*/
@Test
public void test370AutzJackManyAutz() throws Exception {
cleanupAutzTest(USER_JACK_OID);

assignRole(USER_JACK_OID, ROLE_MANY_SHADOW_OWNER_AUTZ.oid);
assignAccountToUser(USER_JACK_OID, RESOURCE_DUMMY_OID, null);
String accountOid =
MiscUtil.extractSingletonRequired(
getUser(USER_JACK_OID).asObjectable().getLinkRef())
.getOid();

login(USER_JACK_USERNAME);

when("account is retrieved");

PerformanceMonitor performanceMonitor = repositoryService.getPerformanceMonitor();
performanceMonitor.clearGlobalPerformanceInformation();
PrismObject<ShadowType> account = getObject(ShadowType.class, accountOid);

then("only one search is executed");
PerformanceInformation performanceInformation = performanceMonitor.getGlobalPerformanceInformation();
displayValue("performance information",
RepositoryPerformanceInformationUtil.format(performanceInformation.toRepositoryPerformanceInformationType()));
String opName = isNativeRepository() ? "SqaleRepositoryService.searchObjects" : "searchObjects";
assertThat(performanceInformation.getInvocationCount(opName))
.as("searchObjects operation count")
.isEqualTo(1);

and("account is OK");
display("account", account);
assertThat(account.getValue().getItems()).as("items in account object").hasSize(8);
}

@SuppressWarnings("SameParameterValue")
private ObjectQuery createOrgSubtreeAndNameQuery(String orgOid, String name) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
<!--
~ Copyright (c) 2014 Evolveum and contributors
~
~ This work is dual-licensed under the Apache License 2.0
~ and European Union Public License. See LICENSE file for details.
-->
<role oid="c8c99194-3e5c-439b-bf98-c71146d3e1b5"
xmlns="http://midpoint.evolveum.com/xml/ns/public/common/common-3">
<name>many-shadow-owner-autz</name>
<documentation>Testing MID-8363: repeated resolution of shadow owner</documentation>
<authorization>
<action>http://midpoint.evolveum.com/xml/ns/public/security/authorization-model-3#read</action>
<object>
<type>ShadowType</type>
<owner>
<special>self</special>
</owner>
</object>
<item>name</item>
</authorization>
<authorization>
<action>http://midpoint.evolveum.com/xml/ns/public/security/authorization-model-3#read</action>
<object>
<type>ShadowType</type>
<owner>
<special>self</special>
</owner>
</object>
<item>resourceRef</item>
</authorization>
<authorization>
<action>http://midpoint.evolveum.com/xml/ns/public/security/authorization-model-3#read</action>
<object>
<type>ShadowType</type>
<owner>
<special>self</special>
</owner>
</object>
<item>objectClass</item>
</authorization>
<authorization>
<action>http://midpoint.evolveum.com/xml/ns/public/security/authorization-model-3#read</action>
<object>
<type>ShadowType</type>
<owner>
<special>self</special>
</owner>
</object>
<item>kind</item>
</authorization>
<authorization>
<action>http://midpoint.evolveum.com/xml/ns/public/security/authorization-model-3#read</action>
<object>
<type>ShadowType</type>
<owner>
<special>self</special>
</owner>
</object>
<item>intent</item>
</authorization>
<authorization>
<action>http://midpoint.evolveum.com/xml/ns/public/security/authorization-model-3#read</action>
<object>
<type>ShadowType</type>
<owner>
<special>self</special>
</owner>
</object>
<item>attributes</item>
</authorization>
<authorization>
<action>http://midpoint.evolveum.com/xml/ns/public/security/authorization-model-3#read</action>
<object>
<type>ShadowType</type>
<owner>
<special>self</special>
</owner>
</object>
<item>activation</item>
</authorization>
<authorization>
<action>http://midpoint.evolveum.com/xml/ns/public/security/authorization-model-3#read</action>
<object>
<type>ShadowType</type>
<owner>
<special>self</special>
</owner>
</object>
<item>credentials</item>
</authorization>
<authorization>
<action>http://midpoint.evolveum.com/xml/ns/public/security/authorization-model-3#read</action>
<object>
<type>ShadowType</type>
<owner>
<special>self</special>
</owner>
</object>
<item>description</item>
</authorization>
<authorization>
<action>http://midpoint.evolveum.com/xml/ns/public/security/authorization-model-3#read</action>
<object>
<type>ShadowType</type>
<owner>
<special>self</special>
</owner>
</object>
<item>documentation</item>
</authorization>
</role>
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,12 @@

import java.util.Collection;

import com.evolveum.midpoint.prism.PrismConstants;

import com.evolveum.midpoint.prism.PrismContext;

import com.evolveum.midpoint.util.logging.TraceManager;

import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;

Expand Down Expand Up @@ -157,6 +163,8 @@ public interface RepositoryService {
String KEY_DIAG_DATA = "repositoryDiagData"; // see GetOperationOptions.attachDiagData
String KEY_ORIGINAL_OBJECT = "repositoryOriginalObject";

Trace LOGGER = TraceManager.getTrace(RepositoryService.class);

/**
* Returns object for provided OID.
*
Expand Down Expand Up @@ -519,7 +527,27 @@ <O extends ObjectType> boolean isAncestor(PrismObject<O> object, String descenda
* @throws IllegalArgumentException wrong OID format
* @deprecated TODO: we want to remove this in midScale
*/
<F extends FocusType> PrismObject<F> searchShadowOwner(String shadowOid, Collection<SelectorOptions<GetOperationOptions>> options, OperationResult parentResult);
default <F extends FocusType> PrismObject<F> searchShadowOwner(
String shadowOid, Collection<SelectorOptions<GetOperationOptions>> options, OperationResult parentResult) {
ObjectQuery query = PrismContext.get()
.queryFor(FocusType.class)
.item(FocusType.F_LINK_REF).ref(shadowOid, null, PrismConstants.Q_ANY)
.build();
SearchResultList<PrismObject<FocusType>> searchResult;
try {
searchResult = searchObjects(FocusType.class, query, options, parentResult);
} catch (SchemaException e) {
throw SystemException.unexpected(e, "when searching for shadow owner");
}

if (searchResult.isEmpty()) {
return null; // account shadow owner was not found
} else if (searchResult.size() > 1) {
LOGGER.warn("Found {} owners for shadow oid {}, returning first owner.", searchResult.size(), shadowOid);
}
//noinspection unchecked
return (PrismObject<F>) searchResult.get(0);
}

/**
* This operation is guaranteed to be atomic. If two threads or even two nodes request a value from
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,12 +126,6 @@ public <T extends Containerable> SearchResultList<T> searchContainers(Class<T> t
return searchOpHandler.searchContainers(type, query, options, parentResult);
}

@Override
public <F extends FocusType> PrismObject<F> searchShadowOwner(
String shadowOid, Collection<SelectorOptions<GetOperationOptions>> options, OperationResult parentResult) {
return searchOpHandler.searchShadowOwner(shadowOid, options, parentResult);
}

@Override
public <T extends ObjectType> int countObjects(Class<T> type, ObjectQuery query,
Collection<SelectorOptions<GetOperationOptions>> options, OperationResult parentResult)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -676,18 +676,16 @@ private void assertOperations(String operation, int expectedCount) {
private int getOperationCount(String operation) {
PerformanceInformation performanceInformation =
repositoryCache.getPerformanceMonitor().getGlobalPerformanceInformation();
OperationPerformanceInformation opData =
// performanceInformation.getAllData().get(operation);
performanceInformation.getAllData().get(opNamePrefix + operation);
OperationPerformanceInformation opData = performanceInformation.getAllData().get(opNamePrefix + operation);
return opData != null ? opData.getInvocationCount() : 0;
}

private void dumpStatistics() {
PerformanceInformation performanceInformation = repositoryCache.getPerformanceMonitor().getGlobalPerformanceInformation();
displayValue("Repository statistics", RepositoryPerformanceInformationUtil.format(performanceInformation.toRepositoryPerformanceInformationType()));
displayValue("Repository statistics",
RepositoryPerformanceInformationUtil.format(performanceInformation.toRepositoryPerformanceInformationType()));

Map<String, CachePerformanceCollector.CacheData> cache = CachePerformanceCollector.INSTANCE
.getGlobalPerformanceMap();
Map<String, CachePerformanceCollector.CacheData> cache = CachePerformanceCollector.INSTANCE.getGlobalPerformanceMap();
displayValue("Cache performance information (standard)",
CachePerformanceInformationUtil.format(CachePerformanceInformationUtil.toCachesPerformanceInformationType(cache)));
displayValue("Cache performance information (extra)",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1330,44 +1330,6 @@ public <O extends ObjectType> boolean isAncestor(
}
}

@Override
public <F extends FocusType> PrismObject<F> searchShadowOwner(String shadowOid,
Collection<SelectorOptions<GetOperationOptions>> options, OperationResult parentResult) {
Objects.requireNonNull(parentResult, "Operation result must not be null.");

OperationResult operationResult =
parentResult.subresult(opNamePrefix + OP_SEARCH_SHADOW_OWNER)
.addParam("shadowOid", shadowOid)
.addParam("options", String.valueOf(options))
.build();

try {
ObjectQuery query = prismContext()
.queryFor(FocusType.class)
.item(FocusType.F_LINK_REF).ref(shadowOid, null, PrismConstants.Q_ANY)
.build();
SearchResultList<PrismObject<FocusType>> result =
executeSearchObjects(FocusType.class, query, options, OP_SEARCH_SHADOW_OWNER);

if (result == null || result.isEmpty()) {
// account shadow owner was not found
return null;
} else if (result.size() > 1) {
logger.warn("Found {} owners for shadow oid {}, returning first owner.",
result.size(), shadowOid);
}
//noinspection unchecked
return (PrismObject<F>) result.get(0);
} catch (RepositoryException | RuntimeException | SchemaException e) {
throw handledGeneralException(e, operationResult);
} catch (Throwable t) {
recordFatalError(operationResult, t);
throw t;
} finally {
operationResult.close();
}
}

@Override
public long advanceSequence(String oid, OperationResult parentResult)
throws ObjectNotFoundException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -244,26 +244,6 @@ private <RV> RV executeQueryAttempts(ObjectQuery query, String operationName, Cl
}
}

@Override
public <F extends FocusType> PrismObject<F> searchShadowOwner(String shadowOid, Collection<SelectorOptions<GetOperationOptions>> options, OperationResult result) {
Validate.notEmpty(shadowOid, "Oid must not be null or empty.");
Validate.notNull(result, "Operation result must not be null.");

LOGGER.debug("Searching shadow owner for {}", shadowOid);

OperationResult subResult = result.subresult(SEARCH_SHADOW_OWNER)
.addParam("shadowOid", shadowOid)
.build();

try {
return executeAttempts(shadowOid, OP_SEARCH_SHADOW_OWNER, FocusType.class, "searching shadow owner",
subResult, () -> objectRetriever.searchShadowOwnerAttempt(shadowOid, options, subResult)
);
} catch (ObjectNotFoundException | SchemaException e) {
throw new AssertionError("Should not occur; exception should have been treated in searchShadowOwnerAttempt.", e);
}
}

@NotNull
@Override
public <T extends ObjectType> SearchResultList<PrismObject<T>> searchObjects(
Expand Down

0 comments on commit b9bca18

Please sign in to comment.