Skip to content

Commit 6c4632b

Browse files
n1v0lgn1v0lg
committed
Revert "Change read-only suffix for file based role mappings (#114205)"
This reverts commit bc8f9dc.
1 parent f551cef commit 6c4632b

File tree

6 files changed

+15
-58
lines changed

6 files changed

+15
-58
lines changed

‎x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/RoleMappingFileSettingsIT.java‎

Lines changed: 5 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -359,9 +359,9 @@ public void testGetRoleMappings() throws Exception {
359359
Arrays.stream(response.mappings()).map(ExpressionRoleMapping::getName).toList(),
360360
containsInAnyOrder(
361361
"everyone_kibana",
362-
"everyone_kibana" + RESERVED_ROLE_MAPPING_SUFFIX,
362+
"everyone_kibana " + RESERVED_ROLE_MAPPING_SUFFIX,
363363
"_everyone_kibana",
364-
"everyone_fleet" + RESERVED_ROLE_MAPPING_SUFFIX,
364+
"everyone_fleet " + RESERVED_ROLE_MAPPING_SUFFIX,
365365
"zzz_mapping",
366366
"123_mapping"
367367
)
@@ -378,16 +378,6 @@ public void testGetRoleMappings() throws Exception {
378378
// it's possible to delete overlapping native role mapping
379379
assertTrue(client().execute(DeleteRoleMappingAction.INSTANCE, deleteRequest("everyone_kibana")).actionGet().isFound());
380380

381-
// Fetch a specific file based role
382-
request = new GetRoleMappingsRequest();
383-
request.setNames("everyone_kibana" + RESERVED_ROLE_MAPPING_SUFFIX);
384-
response = client().execute(GetRoleMappingsAction.INSTANCE, request).get();
385-
assertTrue(response.hasMappings());
386-
assertThat(
387-
Arrays.stream(response.mappings()).map(ExpressionRoleMapping::getName).toList(),
388-
containsInAnyOrder("everyone_kibana" + RESERVED_ROLE_MAPPING_SUFFIX)
389-
);
390-
391381
savedClusterState = setupClusterStateListenerForCleanup(internalCluster().getMasterName());
392382
writeJSONFile(internalCluster().getMasterName(), emptyJSON, logger, versionCounter);
393383
awaitSuccessful = savedClusterState.v1().await(20, TimeUnit.SECONDS);
@@ -566,7 +556,9 @@ private static void assertGetResponseHasMappings(boolean readOnly, String... map
566556
assertThat(
567557
Arrays.stream(response.mappings()).map(ExpressionRoleMapping::getName).toList(),
568558
containsInAnyOrder(
569-
Arrays.stream(mappings).map(mapping -> mapping + (readOnly ? RESERVED_ROLE_MAPPING_SUFFIX : "")).toArray(String[]::new)
559+
Arrays.stream(mappings)
560+
.map(mapping -> mapping + (readOnly ? " " + RESERVED_ROLE_MAPPING_SUFFIX : ""))
561+
.toArray(String[]::new)
570562
)
571563
);
572564
}

‎x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/role/TransportDeleteRoleAction.java‎

Lines changed: 2 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -10,15 +10,13 @@
1010
import org.elasticsearch.action.ActionListener;
1111
import org.elasticsearch.action.support.ActionFilters;
1212
import org.elasticsearch.action.support.TransportAction;
13-
import org.elasticsearch.common.logging.HeaderWarning;
1413
import org.elasticsearch.common.util.concurrent.EsExecutors;
1514
import org.elasticsearch.injection.guice.Inject;
1615
import org.elasticsearch.tasks.Task;
1716
import org.elasticsearch.transport.TransportService;
1817
import org.elasticsearch.xpack.core.security.action.role.DeleteRoleAction;
1918
import org.elasticsearch.xpack.core.security.action.role.DeleteRoleRequest;
2019
import org.elasticsearch.xpack.core.security.action.role.DeleteRoleResponse;
21-
import org.elasticsearch.xpack.security.authc.support.mapper.ClusterStateRoleMapper;
2220
import org.elasticsearch.xpack.security.authz.ReservedRoleNameChecker;
2321
import org.elasticsearch.xpack.security.authz.store.NativeRolesStore;
2422

@@ -27,20 +25,16 @@ public class TransportDeleteRoleAction extends TransportAction<DeleteRoleRequest
2725
private final NativeRolesStore rolesStore;
2826
private final ReservedRoleNameChecker reservedRoleNameChecker;
2927

30-
private final ClusterStateRoleMapper clusterStateRoleMapper;
31-
3228
@Inject
3329
public TransportDeleteRoleAction(
3430
ActionFilters actionFilters,
3531
NativeRolesStore rolesStore,
3632
TransportService transportService,
37-
ReservedRoleNameChecker reservedRoleNameChecker,
38-
ClusterStateRoleMapper clusterStateRoleMapper
33+
ReservedRoleNameChecker reservedRoleNameChecker
3934
) {
4035
super(DeleteRoleAction.NAME, actionFilters, transportService.getTaskManager(), EsExecutors.DIRECT_EXECUTOR_SERVICE);
4136
this.rolesStore = rolesStore;
4237
this.reservedRoleNameChecker = reservedRoleNameChecker;
43-
this.clusterStateRoleMapper = clusterStateRoleMapper;
4438
}
4539

4640
@Override
@@ -51,19 +45,7 @@ protected void doExecute(Task task, DeleteRoleRequest request, ActionListener<De
5145
}
5246

5347
try {
54-
rolesStore.deleteRole(request, listener.safeMap((found) -> {
55-
if (clusterStateRoleMapper.hasMapping(request.name())) {
56-
// Allow to delete a mapping with the same name in the native role mapping store as the file_settings namespace, but
57-
// add a warning header to signal to the caller that this could be a problem.
58-
HeaderWarning.addWarning(
59-
"A read only role mapping with the same name ["
60-
+ request.name()
61-
+ "] has been previously been defined in a configuration file. "
62-
+ "The read only role mapping will still be active."
63-
);
64-
}
65-
return new DeleteRoleResponse(found);
66-
}));
48+
rolesStore.deleteRole(request, listener.safeMap(DeleteRoleResponse::new));
6749
} catch (Exception e) {
6850
logger.error((Supplier<?>) () -> "failed to delete role [" + request.name() + "]", e);
6951
listener.onFailure(e);

‎x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/rolemapping/TransportGetRoleMappingsAction.java‎

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@
2525
import java.util.HashSet;
2626
import java.util.List;
2727
import java.util.Set;
28-
import java.util.stream.Collectors;
2928
import java.util.stream.Stream;
3029

3130
import static org.elasticsearch.xpack.security.authc.support.mapper.ClusterStateRoleMapper.RESERVED_ROLE_MAPPING_SUFFIX;
@@ -64,12 +63,7 @@ protected void doExecute(Task task, final GetRoleMappingsRequest request, final
6463
roleMappingStore.getRoleMappings(names, ActionListener.wrap(mappings -> {
6564
List<ExpressionRoleMapping> combinedRoleMappings = Stream.concat(
6665
mappings.stream(),
67-
clusterStateRoleMapper.getMappings(names == null ? null : names.stream().map(name -> {
68-
// If a read-only role is fetched by name including suffix, remove suffix
69-
return name.endsWith(RESERVED_ROLE_MAPPING_SUFFIX)
70-
? name.substring(0, name.length() - RESERVED_ROLE_MAPPING_SUFFIX.length())
71-
: name;
72-
}).collect(Collectors.toSet()))
66+
clusterStateRoleMapper.getMappings(names)
7367
.stream()
7468
.map(this::cloneAndMarkAsReadOnly)
7569
.sorted(Comparator.comparing(ExpressionRoleMapping::getName))
@@ -81,7 +75,7 @@ protected void doExecute(Task task, final GetRoleMappingsRequest request, final
8175
private ExpressionRoleMapping cloneAndMarkAsReadOnly(ExpressionRoleMapping mapping) {
8276
// Mark role mappings from cluster state as "read only" by adding a suffix to their name
8377
return new ExpressionRoleMapping(
84-
mapping.getName() + RESERVED_ROLE_MAPPING_SUFFIX,
78+
mapping.getName() + " " + RESERVED_ROLE_MAPPING_SUFFIX,
8579
mapping.getExpression(),
8680
mapping.getRoles(),
8781
mapping.getRoleTemplates(),

‎x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/mapper/ClusterStateRoleMapper.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ public class ClusterStateRoleMapper extends AbstractRoleMapperClearRealmCache im
4747
* </ul>
4848
*/
4949
public static final String CLUSTER_STATE_ROLE_MAPPINGS_ENABLED = "xpack.security.authc.cluster_state_role_mappings.enabled";
50-
public static final String RESERVED_ROLE_MAPPING_SUFFIX = "-read-only-operator-config";
50+
public static final String RESERVED_ROLE_MAPPING_SUFFIX = "(read only)";
5151
private static final Logger logger = LogManager.getLogger(ClusterStateRoleMapper.class);
5252

5353
private final ScriptService scriptService;

‎x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/role/TransportDeleteRoleActionTests.java‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@
1919
import org.elasticsearch.xpack.core.security.action.role.DeleteRoleResponse;
2020
import org.elasticsearch.xpack.core.security.authc.AuthenticationTestHelper;
2121
import org.elasticsearch.xpack.core.security.authz.store.ReservedRolesStore;
22-
import org.elasticsearch.xpack.security.authc.support.mapper.ClusterStateRoleMapper;
2322
import org.elasticsearch.xpack.security.authz.ReservedRoleNameChecker;
2423
import org.elasticsearch.xpack.security.authz.store.NativeRolesStore;
2524
import org.junit.BeforeClass;
@@ -67,8 +66,7 @@ public void testReservedRole() {
6766
mock(ActionFilters.class),
6867
rolesStore,
6968
transportService,
70-
new ReservedRoleNameChecker.Default(),
71-
mock(ClusterStateRoleMapper.class)
69+
new ReservedRoleNameChecker.Default()
7270
);
7371

7472
DeleteRoleRequest request = new DeleteRoleRequest();
@@ -117,8 +115,7 @@ private void testValidRole(String roleName) {
117115
mock(ActionFilters.class),
118116
rolesStore,
119117
transportService,
120-
new ReservedRoleNameChecker.Default(),
121-
mock(ClusterStateRoleMapper.class)
118+
new ReservedRoleNameChecker.Default()
122119
);
123120

124121
DeleteRoleRequest request = new DeleteRoleRequest();
@@ -171,8 +168,7 @@ public void testException() {
171168
mock(ActionFilters.class),
172169
rolesStore,
173170
transportService,
174-
new ReservedRoleNameChecker.Default(),
175-
mock(ClusterStateRoleMapper.class)
171+
new ReservedRoleNameChecker.Default()
176172
);
177173

178174
DeleteRoleRequest request = new DeleteRoleRequest();

‎x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/rolemapping/TransportPutRoleMappingActionTests.java‎

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@
2929
import java.util.Set;
3030
import java.util.concurrent.atomic.AtomicReference;
3131

32-
import static org.elasticsearch.xpack.security.authc.support.mapper.ClusterStateRoleMapper.RESERVED_ROLE_MAPPING_SUFFIX;
3332
import static org.hamcrest.Matchers.aMapWithSize;
3433
import static org.hamcrest.Matchers.contains;
3534
import static org.hamcrest.Matchers.equalTo;
@@ -98,18 +97,12 @@ public void testPutMappingWithInvalidName() {
9897
final FieldExpression expression = new FieldExpression("username", Collections.singletonList(new FieldExpression.FieldValue("*")));
9998
IllegalArgumentException illegalArgumentException = expectThrows(
10099
IllegalArgumentException.class,
101-
() -> put("anarchy" + RESERVED_ROLE_MAPPING_SUFFIX, expression, "superuser", Collections.singletonMap("dumb", true))
100+
() -> put("anarchy (read only)", expression, "superuser", Collections.singletonMap("dumb", true))
102101
);
103102

104103
assertThat(
105104
illegalArgumentException.getMessage(),
106-
equalTo(
107-
"Invalid mapping name [anarchy"
108-
+ RESERVED_ROLE_MAPPING_SUFFIX
109-
+ "]. ["
110-
+ RESERVED_ROLE_MAPPING_SUFFIX
111-
+ "] is not an allowed suffix"
112-
)
105+
equalTo("Invalid mapping name [anarchy (read only)]. [(read only)] is not an allowed suffix")
113106
);
114107
}
115108

0 commit comments

Comments
 (0)