Skip to content

Account for reserved cluster state role mappings #113687

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.

Sign up for GitHub

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

Closed
wants to merge 19 commits into from
Closed

Account for reserved cluster state role mappings #113687

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
c6245bb
Account for reserved cluster state role mappings
n1v0lg Sep 27, 2024
843353e
WIP
n1v0lg Sep 27, 2024
d09f464
Update docs/changelog/113687.yaml
n1v0lg Sep 27, 2024
6e2d6a6
Comment
n1v0lg Sep 27, 2024
488327e
Merge branch 'fix-cluster-state-role-mapping' of github.com:n1v0lg/el…
n1v0lg Sep 27, 2024
e0adf0c
Clean
n1v0lg Sep 30, 2024
5062238
Less
n1v0lg Sep 30, 2024
f6fb4fc
Tweaks
n1v0lg Sep 30, 2024
c8842fe
Less copying
n1v0lg Sep 30, 2024
19fe01a
Fix tests
n1v0lg Sep 30, 2024
ee254b6
Merge branch 'main' into fix-cluster-state-role-mapping
n1v0lg Sep 30, 2024
924aab7
ITs
n1v0lg Sep 30, 2024
5a5c574
Fixes and tests
n1v0lg Sep 30, 2024
dcd9351
Fix setup
n1v0lg Sep 30, 2024
e76f50b
Fix caching
n1v0lg Sep 30, 2024
fbcad41
Leave note to self
n1v0lg Sep 30, 2024
8459cac
Fix
n1v0lg Oct 1, 2024
8e25c0b
Fix
n1v0lg Oct 1, 2024
8706cab
Bring back Javadoc
n1v0lg Oct 1, 2024
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: 5 additions & 0 deletions docs/changelog/113687.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 113687
summary: Account for reserved cluster state role mappings
area: Authentication
type: bug
issues: []
119 changes: 71 additions & 48 deletions ...src/internalClusterTest/java/org/elasticsearch/integration/RoleMappingFileSettingsIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import org.elasticsearch.xpack.core.security.authc.support.UserRoleMapper;
import org.elasticsearch.xpack.core.security.authc.support.mapper.ExpressionRoleMapping;
import org.elasticsearch.xpack.security.action.rolemapping.ReservedRoleMappingAction;
import org.hamcrest.Matchers;
import org.junit.After;

import java.io.ByteArrayInputStream;
Expand All @@ -59,11 +60,9 @@
import static org.elasticsearch.xcontent.XContentType.JSON;
import static org.elasticsearch.xpack.core.security.test.TestRestrictedIndices.INTERNAL_SECURITY_MAIN_INDEX_7;
import static org.hamcrest.Matchers.allOf;
import static org.hamcrest.Matchers.contains;
import static org.hamcrest.Matchers.containsInAnyOrder;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.empty;
import static org.hamcrest.Matchers.emptyArray;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.hasSize;
import static org.hamcrest.Matchers.notNullValue;
Expand Down Expand Up @@ -270,18 +269,29 @@ private void assertRoleMappingsSaveOK(CountDownLatch savedClusterState, AtomicLo
assertThat(resolveRolesFuture.get(), containsInAnyOrder("kibana_user", "fleet_user"));
}

// the role mappings are not retrievable by the role mapping action (which only accesses "native" i.e. index-based role mappings)
var request = new GetRoleMappingsRequest();
request.setNames("everyone_kibana", "everyone_fleet");
var response = client().execute(GetRoleMappingsAction.INSTANCE, request).get();
assertFalse(response.hasMappings());
assertThat(response.mappings(), emptyArray());

// role mappings (with the same names) can also be stored in the "native" store
var putRoleMappingResponse = client().execute(PutRoleMappingAction.INSTANCE, sampleRestRequest("everyone_kibana")).actionGet();
assertTrue(putRoleMappingResponse.isCreated());
putRoleMappingResponse = client().execute(PutRoleMappingAction.INSTANCE, sampleRestRequest("everyone_fleet")).actionGet();
assertTrue(putRoleMappingResponse.isCreated());
// cluster-state role mappings are retrievable by the role mapping action since they are treated as reserved role mappings and need
// to be surfaced via API for BWC
assertGetResponseHasMappings("everyone_kibana", "everyone_fleet");

// role mappings (with the same names) cannot be modified via API calls since they are reserved
expectThrows(
IllegalArgumentException.class,
() -> client().execute(PutRoleMappingAction.INSTANCE, sampleRestRequest("everyone_kibana")).actionGet()
);
expectThrows(
IllegalArgumentException.class,
() -> client().execute(PutRoleMappingAction.INSTANCE, sampleRestRequest("everyone_fleet")).actionGet()
);
expectThrows(IllegalArgumentException.class, () -> {
var deleteRoleMappingRequest = new DeleteRoleMappingRequest();
deleteRoleMappingRequest.setName("everyone_kibana");
client().execute(DeleteRoleMappingAction.INSTANCE, deleteRoleMappingRequest).actionGet();
});
expectThrows(IllegalArgumentException.class, () -> {
var deleteRoleMappingRequest = new DeleteRoleMappingRequest();
deleteRoleMappingRequest.setName("everyone_fleet");
client().execute(DeleteRoleMappingAction.INSTANCE, deleteRoleMappingRequest).actionGet();
});
}

public void testRoleMappingsApplied() throws Exception {
Expand All @@ -291,64 +301,72 @@ public void testRoleMappingsApplied() throws Exception {
writeJSONFile(internalCluster().getMasterName(), testJSON, logger, versionCounter);

assertRoleMappingsSaveOK(savedClusterState.v1(), savedClusterState.v2());
logger.info("---> cleanup cluster settings...");

savedClusterState = setupClusterStateListenerForCleanup(internalCluster().getMasterName());

writeJSONFile(internalCluster().getMasterName(), emptyJSON, logger, versionCounter);
boolean awaitSuccessful = savedClusterState.v1().await(20, TimeUnit.SECONDS);
assertTrue(awaitSuccessful);

final ClusterStateResponse clusterStateResponse = clusterAdmin().state(
new ClusterStateRequest(TEST_REQUEST_TIMEOUT).waitForMetadataVersion(savedClusterState.v2().get())
).get();

assertNull(
clusterStateResponse.getState().metadata().persistentSettings().get(INDICES_RECOVERY_MAX_BYTES_PER_SEC_SETTING.getKey())
);

// native role mappings are not affected by the removal of the cluster-state based ones
// empty cluster state, and no native privileges means empty response from the API
{
var request = new GetRoleMappingsRequest();
request.setNames("everyone_kibana", "everyone_fleet");
var response = client().execute(GetRoleMappingsAction.INSTANCE, request).get();
assertTrue(response.hasMappings());
assertThat(
Arrays.stream(response.mappings()).map(ExpressionRoleMapping::getName).toList(),
containsInAnyOrder("everyone_kibana", "everyone_fleet")
);
assertFalse(response.hasMappings());
}

// and roles are resolved based on the native role mappings
// and for role mapping
for (UserRoleMapper userRoleMapper : internalCluster().getInstances(UserRoleMapper.class)) {
PlainActionFuture<Set<String>> resolveRolesFuture = new PlainActionFuture<>();
userRoleMapper.resolveRoles(
new UserRoleMapper.UserData("anyUsername", null, List.of(), Map.of(), mock(RealmConfig.class)),
resolveRolesFuture
);
assertThat(resolveRolesFuture.get(), contains("kibana_user_native"));
assertThat(resolveRolesFuture.get(), empty());
}

// since there are no reserved role mappings, we can create native ones with names of the reserved mappings that existed previously
{
var request = new DeleteRoleMappingRequest();
request.setName("everyone_kibana");
var response = client().execute(DeleteRoleMappingAction.INSTANCE, request).get();
assertTrue(response.isFound());
request = new DeleteRoleMappingRequest();
request.setName("everyone_fleet");
response = client().execute(DeleteRoleMappingAction.INSTANCE, request).get();
assertTrue(response.isFound());
var putRoleMappingResponse = client().execute(PutRoleMappingAction.INSTANCE, sampleRestRequest("everyone_kibana")).actionGet();
assertTrue(putRoleMappingResponse.isCreated());
putRoleMappingResponse = client().execute(PutRoleMappingAction.INSTANCE, sampleRestRequest("everyone_fleet")).actionGet();
assertTrue(putRoleMappingResponse.isCreated());
}

// no roles are resolved now, because both native and cluster-state based stores have been cleared
// native mapping is used in role mapping
for (UserRoleMapper userRoleMapper : internalCluster().getInstances(UserRoleMapper.class)) {
PlainActionFuture<Set<String>> resolveRolesFuture = new PlainActionFuture<>();
userRoleMapper.resolveRoles(
new UserRoleMapper.UserData("anyUsername", null, List.of(), Map.of(), mock(RealmConfig.class)),
resolveRolesFuture
);
assertThat(resolveRolesFuture.get(), empty());
assertThat(resolveRolesFuture.get(), Matchers.contains("kibana_user_native"));
}

// put back reserved cluster state role mapping
savedClusterState = setupClusterStateListener(internalCluster().getMasterName(), "everyone_kibana");
writeJSONFile(internalCluster().getMasterName(), testJSON, logger, versionCounter);
awaitSuccessful = savedClusterState.v1().await(20, TimeUnit.SECONDS);
assertTrue(awaitSuccessful);

// re-assert everything about native role mappings etc
assertRoleMappingsSaveOK(savedClusterState.v1(), savedClusterState.v2());

logger.info("---> cleanup cluster settings...");
// clean up
writeJSONFile(internalCluster().getMasterName(), emptyJSON, logger, versionCounter);
savedClusterState = setupClusterStateListenerForCleanup(internalCluster().getMasterName());
awaitSuccessful = savedClusterState.v1().await(20, TimeUnit.SECONDS);
assertTrue(awaitSuccessful);

assertNull(
clusterAdmin().state(new ClusterStateRequest(TEST_REQUEST_TIMEOUT).waitForMetadataVersion(savedClusterState.v2().get()))
.get()
.getState()
.metadata()
.persistentSettings()
.get(INDICES_RECOVERY_MAX_BYTES_PER_SEC_SETTING.getKey())
);

}

public static Tuple<CountDownLatch, AtomicLong> setupClusterStateListenerForError(
Expand Down Expand Up @@ -433,11 +451,8 @@ public void testRoleMappingApplyWithSecurityIndexClosed() throws Exception {
boolean awaitSuccessful = savedClusterState.v1().await(20, TimeUnit.SECONDS);
assertTrue(awaitSuccessful);

// no native role mappings exist
var request = new GetRoleMappingsRequest();
request.setNames("everyone_kibana", "everyone_fleet");
var response = client().execute(GetRoleMappingsAction.INSTANCE, request).get();
assertFalse(response.hasMappings());
// even though the index is closed, reserved mappings can still be fetched
assertGetResponseHasMappings("everyone_kibana", "everyone_fleet");

// cluster state settings are also applied
var clusterStateResponse = clusterAdmin().state(
Expand Down Expand Up @@ -494,4 +509,12 @@ private PutRoleMappingRequest sampleRestRequest(String name) throws Exception {
return new PutRoleMappingRequestBuilder(null).source(name, parser).request();
}
}

private static void assertGetResponseHasMappings(String... mappings) throws InterruptedException, ExecutionException {
var request = new GetRoleMappingsRequest();
request.setNames(mappings);
var response = client().execute(GetRoleMappingsAction.INSTANCE, request).get();
assertTrue(response.hasMappings());
assertThat(Arrays.stream(response.mappings()).map(ExpressionRoleMapping::getName).toList(), containsInAnyOrder(mappings));
}
}
21 changes: 18 additions & 3 deletions x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,7 @@
import org.elasticsearch.xpack.security.authc.support.mapper.ClusterStateRoleMapper;
import org.elasticsearch.xpack.security.authc.support.mapper.CompositeRoleMapper;
import org.elasticsearch.xpack.security.authc.support.mapper.NativeRoleMappingStore;
import org.elasticsearch.xpack.security.authc.support.mapper.ReservedRoleMappings;
import org.elasticsearch.xpack.security.authz.AuthorizationDenialMessages;
import org.elasticsearch.xpack.security.authz.AuthorizationService;
import org.elasticsearch.xpack.security.authz.DlsFlsRequestCacheDifferentiator;
Expand Down Expand Up @@ -851,14 +852,16 @@ Collection<Object> createComponents(

// realms construction
final NativeUsersStore nativeUsersStore = new NativeUsersStore(settings, client, systemIndices.getMainIndexManager());
final ClusterStateRoleMapper clusterStateRoleMapper = new ClusterStateRoleMapper(settings, scriptService, clusterService);
final NativeRoleMappingStore nativeRoleMappingStore = new NativeRoleMappingStore(
settings,
client,
systemIndices.getMainIndexManager(),
scriptService
scriptService,
new ReservedRoleMappings(clusterStateRoleMapper)
);
final ClusterStateRoleMapper clusterStateRoleMapper = new ClusterStateRoleMapper(settings, scriptService, clusterService);
final UserRoleMapper userRoleMapper = new CompositeRoleMapper(nativeRoleMappingStore, clusterStateRoleMapper);
Copy link
Contributor Author

@n1v0lg n1v0lg Sep 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This change (getting rid of CompositeRoleMapper) is a little nuanced. The reason I'm making it is:

The PR changes the NativeRoleMappingStore to fetch "reserved" (i.e., cluster-state-based) role mappings and use these instead of any native role mappings with the same name (see mergeWithReserved). This means that we read cluster-state role mappings in the NativeRoleMappingStore, and substitute mappings based on what we read. If we stuck with the CompositeRoleMapper here, we'd "double-read" cluster-state-based role mappings.

An alternative I originally considered is to filter out native role mappings that match reserved ones and then simply rely on CompositeRoleMapper here to fill in cluster-state ones. However, this would introduce a race condition:

  1. When we get role mappings from the native store, we read cluster-state and filter out.
  2. We returning the result.
  3. Concurrently, cluster state changes (mappings added or removed).
  4. We read cluster state again, including the new mappings.

However, this means we have a "stale" view obtained in step 2.

By going with current approach in this PR instead (remove CompositeRoleMapper), we avoid this race condition since we only read cluster state once.

Copy link
Contributor

@jfreden jfreden Oct 1, 2024

Choose a reason for hiding this comment

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

Tricky! I think your change here looks good. I thought of an alternative explained below.

I think it would be clearer to keep the role mapping stores separate and then leave the merging of them to some external class. Priorities can change and the way we want to merge them could change too. With your change ClusterStateRoleMapper is injected into the NativeRoleMappingStore through the wrapping class ReservedRoleMappings and you're using ClusterStateRoleMapper "internals" (getMappings - not exactly an internal but I think it was only public for testing purposes). It does violate the UserRoleMapper interface a little.

I think adding a new MergingCompositeRoleMapper (or a better name) that handles both read and write (so both getRoleMappings and putRoleMapping) would be more general. For read it would merge the result of the provided UserRoleMappers where priority could be the order of arguments passed (so ClusterStateRoleMapper would be passed first to have priority). For writes it could check the ClusterStateRoleMapper for reserved names before writing (according to the configuration) to the native store.

Copy link
Contributor Author

@n1v0lg n1v0lg Oct 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I've considered this (see a stub of it here) however it comes out a much bigger change, and I'm not sure it's worth it because the abstraction itself is sort of neither here nor there. The problems are:

The new class is neither Merging, nor a role mapper. Instead it's a class that merges on reads, but prevents reserved writes. So it's more of a ReservedRoleMappingWrappedStore. And it's not just a role mapper (that only handles resolving roles) but a role mapping store. The latter requires a whole new interface or abstract class, and plugging that interface in everywhere NativeRoleMappingStore is currently used. Do-able, but NativeRoleMappingStore is used in several actions and tests.

I'm just not sure that it's worth it to introduce all this generic structure to handle something that's unlikely to generalize. The change is already quite nuanced so I worry that making the delta bigger makes it harder to review and reason about. WDYT?

Copy link
Contributor

@jfreden jfreden Oct 1, 2024

Choose a reason for hiding this comment

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

Yes, I see what you mean. The interfaces are note exactly built for that and the change is big.

I'm just not sure that it's worth it to introduce all this generic structure to handle something that's unlikely to generalize. WDYT?

I think the coupling this introduces is a little concerning, but I don't think this is the right time to be making a huge refactor, so I think leaving it is fine.

final UserRoleMapper userRoleMapper = getUserRoleMapper(nativeRoleMappingStore, clusterStateRoleMapper);

final AnonymousUser anonymousUser = new AnonymousUser(settings);
components.add(anonymousUser);
final ReservedRealm reservedRealm = new ReservedRealm(environment, settings, nativeUsersStore, anonymousUser, threadPool);
Expand Down Expand Up @@ -1223,6 +1226,18 @@ Collection<Object> createComponents(
return components;
}

private UserRoleMapper getUserRoleMapper(NativeRoleMappingStore nativeRoleMappingStore, ClusterStateRoleMapper clusterStateRoleMapper) {
if (nativeRoleMappingStore.isEnabled()) {
// native role mapper is set up to handle both cluster state role mapping AND native role mapping
return nativeRoleMappingStore;
} else if (clusterStateRoleMapper.isEnabled()) {
return clusterStateRoleMapper;
} else {
// shouldn't happen but might as well handle this
return new CompositeRoleMapper();
}
}

private void applyPendingSecurityMigrations(SecurityIndexManager.State newState) {
// If no migrations have been applied and the security index is on the latest version (new index), all migrations can be skipped
if (newState.migrationsVersion == 0 && newState.createdOnLatestVersion) {
Expand Down
3 changes: 2 additions & 1 deletion ...a/org/elasticsearch/xpack/security/action/rolemapping/TransportGetRoleMappingsAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,9 @@ protected void doExecute(Task task, final GetRoleMappingsRequest request, final
} else {
names = new HashSet<>(Arrays.asList(request.getNames()));
}
// TODO make sure result is deterministic
Copy link
Contributor Author

@n1v0lg n1v0lg Sep 30, 2024

Choose a reason for hiding this comment

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

What I mean here is ordered the same each time: the respond now contains the output of ClusterStateRoleMapper#getMappings which is a set and therefore not well-ordered.

this.roleMappingStore.getRoleMappings(names, ActionListener.wrap(mappings -> {
ExpressionRoleMapping[] array = mappings.toArray(new ExpressionRoleMapping[mappings.size()]);
ExpressionRoleMapping[] array = mappings.toArray(new ExpressionRoleMapping[0]);
listener.onResponse(new GetRoleMappingsResponse(array));
}, listener::onFailure));
}
Expand Down
12 changes: 8 additions & 4 deletions ...in/java/org/elasticsearch/xpack/security/authc/support/mapper/ClusterStateRoleMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
* A role mapper the reads the role mapping rules (i.e. {@link ExpressionRoleMapping}s) from the cluster state
* (i.e. {@link RoleMappingMetadata}). This is not enabled by default.
*/
public final class ClusterStateRoleMapper extends AbstractRoleMapperClearRealmCache implements ClusterStateListener {
public class ClusterStateRoleMapper extends AbstractRoleMapperClearRealmCache implements ClusterStateListener {

/**
* This setting is never registered by the xpack security plugin - in order to enable the
Expand All @@ -44,6 +44,7 @@ public final class ClusterStateRoleMapper extends AbstractRoleMapperClearRealmCa
* of the {@link NativeRoleMappingStore} and this mapper.</li>
* </ul>
*/
// TODO we need to register this setting as an escape hatch
Copy link
Contributor

@jakelandis jakelandis Sep 30, 2024

Choose a reason for hiding this comment

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

I don't think we should expose this as setting if it is always true and will break things if we set it false. I think we rely (and should double check) the no-op,no-error behavior when the settings.json is not present. We should update the comments to ensure that it is know this is the ECK and serverless path.

Copy link
Contributor

@jfreden jfreden Oct 1, 2024

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

@n1v0lg n1v0lg Oct 1, 2024

Choose a reason for hiding this comment

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

I don't think we should expose this as setting if it is always true and will break things if we set it false.

I'm thinking of stateful, non-ECK deployments. The cluster-state role mapper still has some overhead in that it checks cluster state. If there is an issue we aren't thinking off, I'd really like there to be an escape hatch for stateful, non-ECK customers that by definition don't use cluster-state role mappings. I don't feel very strongly, but I think having a filtered, undocumented setting is a nice-to-have.

public static final String CLUSTER_STATE_ROLE_MAPPINGS_ENABLED = "xpack.security.authc.cluster_state_role_mappings.enabled";
private static final Logger logger = LogManager.getLogger(ClusterStateRoleMapper.class);

Expand All @@ -54,8 +55,7 @@ public final class ClusterStateRoleMapper extends AbstractRoleMapperClearRealmCa
public ClusterStateRoleMapper(Settings settings, ScriptService scriptService, ClusterService clusterService) {
this.scriptService = scriptService;
this.clusterService = clusterService;
// this role mapper is disabled by default and only code in other plugins can enable it
this.enabled = settings.getAsBoolean(CLUSTER_STATE_ROLE_MAPPINGS_ENABLED, false);
this.enabled = settings.getAsBoolean(CLUSTER_STATE_ROLE_MAPPINGS_ENABLED, true);
if (this.enabled) {
clusterService.addListener(this);
}
Expand All @@ -81,7 +81,11 @@ public void clusterChanged(ClusterChangedEvent event) {
}
}

private Set<ExpressionRoleMapping> getMappings() {
public boolean isEnabled() {
return enabled;
}

public Set<ExpressionRoleMapping> getMappings() {
if (enabled == false) {
return Set.of();
} else {
Expand Down
31 changes: 28 additions & 3 deletions ...in/java/org/elasticsearch/xpack/security/authc/support/mapper/NativeRoleMappingStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ public class NativeRoleMappingStore extends AbstractRoleMapperClearRealmCache {
/**
* This setting is never registered by the security plugin - in order to disable the native role APIs
* another plugin must register it as a boolean setting and cause it to be set to `false`.
*
* <p>
* If this setting is set to <code>false</code> then
* <ul>
* <li>the Rest APIs for native role mappings management are disabled.</li>
Expand Down Expand Up @@ -106,14 +106,26 @@ public class NativeRoleMappingStore extends AbstractRoleMapperClearRealmCache {
private final boolean lastLoadCacheEnabled;
private final AtomicReference<List<ExpressionRoleMapping>> lastLoadRef = new AtomicReference<>(null);
private final boolean enabled;
private final ReservedRoleMappings reservedRoleMappings;

public NativeRoleMappingStore(Settings settings, Client client, SecurityIndexManager securityIndex, ScriptService scriptService) {
public NativeRoleMappingStore(
Settings settings,
Client client,
SecurityIndexManager securityIndex,
ScriptService scriptService,
ReservedRoleMappings reservedRoleMappings
) {
this.settings = settings;
this.client = client;
this.securityIndex = securityIndex;
this.scriptService = scriptService;
this.lastLoadCacheEnabled = LAST_LOAD_CACHE_ENABLED_SETTING.get(settings);
this.enabled = settings.getAsBoolean(NATIVE_ROLE_MAPPINGS_ENABLED, true);
this.reservedRoleMappings = reservedRoleMappings;
Copy link
Contributor

@jakelandis jakelandis Sep 30, 2024

Choose a reason for hiding this comment

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

I like this since it simplifies things and nicely makes this change a no-op for serverless since IIUC this is completely disabled in serverless (and likley always will be)

}

public boolean isEnabled() {
return enabled;
}

/**
Expand Down Expand Up @@ -199,6 +211,12 @@ private <Request, Result> void modifyMapping(
Request request,
ActionListener<Result> listener
) {
if (reservedRoleMappings.isReserved(name)) {
listener.onFailure(
new IllegalArgumentException("Role mapping [" + name + "] is reserved and cannot be modified via Native Role Mapping APIs")
);
return;
}
if (securityIndex.isIndexUpToDate() == false) {
listener.onFailure(
new IllegalStateException(
Expand Down Expand Up @@ -304,12 +322,19 @@ public void onFailure(Exception e) {
}
}

public void getRoleMappings(Set<String> names, ActionListener<List<ExpressionRoleMapping>> listener) {
innerGetRoleMappings(
names,
listener.delegateFailureAndWrap((l, mappings) -> l.onResponse(reservedRoleMappings.mergeWithReserved(mappings)))
Copy link
Contributor

@jakelandis jakelandis Sep 30, 2024

Choose a reason for hiding this comment

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

I think I am getting abit lost to where ClusterStateRoleMapper#resolveRoles for reservedRoleMappings is called so that is garunteed to have the mappings already?

Copy link
Contributor

@jfreden jfreden Oct 1, 2024

Choose a reason for hiding this comment

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

ClusterStateRoleMapper::resolveRoles is just wrapping ClusterStateRoleMapper::getMappings that looks at the current cluster state and resolves the roles from the current cluster state. ClusterStateRoleMapper::resolveRoles was previously called from the CompositeRoleMapper but with this change getMappings is instead called directly (so resolveRoles is skipped) as far as I can see.

);
}

/**
* Retrieves one or more mappings from the index.
* If <code>names</code> is <code>null</code> or {@link Set#isEmpty empty}, then this retrieves all mappings.
* Otherwise it retrieves the specified mappings by name.
*/
public void getRoleMappings(Set<String> names, ActionListener<List<ExpressionRoleMapping>> listener) {
private void innerGetRoleMappings(Set<String> names, ActionListener<List<ExpressionRoleMapping>> listener) {
if (enabled == false) {
listener.onResponse(List.of());
} else if (names == null || names.isEmpty()) {
Expand Down
Loading