Skip to content

Dynamically Enable/Disable multi-tenancy, private tenant and set Defa… #2444

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
abhivka7 wants to merge 2 commits into from
Closed

Dynamically Enable/Disable multi-tenancy, private tenant and set Defa… #2444

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b24fa91
Dynamically Enable/Disable multi-tenancy, private tenant and set Defa…
Feb 17, 2023
8de3b7c
Merge branch 'opensearch-project:main' into multitenancy-abhivka-7-PR-1
abhivka7 Feb 23, 2023
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
8 changes: 8 additions & 0 deletions config/tenancy_config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
_meta:
type: "tenancyconfig"
config_version: 2

tenancy_config:
multitenancy_enabled: true
private_tenant_enabled: true
default_tenant: ""
Copy link
Contributor

@stephen-crawford stephen-crawford Feb 17, 2023

Choose a reason for hiding this comment

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

You can use ./gradlew spotlessApply to automatically correct style

Copy link
Contributor Author

@abhivka7 abhivka7 Feb 20, 2023

Choose a reason for hiding this comment

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

Thanks for the sugestion @scrawfor99 . I used ./gradlew spotlessApply as per your suggestion. It corrected style for some files but not tenancy_config.yml. Do you need any specific change in this file that you want me to do manually?

1 change: 1 addition & 0 deletions src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -913,6 +913,7 @@ public List<Setting<?>> getSettings() {

settings.add(Setting.simpleString(ConfigConstants.SECURITY_CERT_INTERCLUSTER_REQUEST_EVALUATOR_CLASS, Property.NodeScope, Property.Filtered));
settings.add(Setting.listSetting(ConfigConstants.SECURITY_NODES_DN, Collections.emptyList(), Function.identity(), Property.NodeScope));//not filtered here
settings.add(Setting.listSetting(ConfigConstants.SECURITY_TENANCY_CONFIG, Collections.emptyList(), Function.identity(), Property.NodeScope));

settings.add(Setting.boolSetting(ConfigConstants.SECURITY_NODES_DN_DYNAMIC_CONFIG_ENABLED, false, Property.NodeScope));//not filtered here

Expand Down
15 changes: 13 additions & 2 deletions src/main/java/org/opensearch/security/configuration/ConfigurationLoaderSecurity7.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,19 @@ public void noData(String id) {
}
}

// Since TenancyConfig is newly introduced data-type applying for existing clusters as well, we make it backward compatible by returning valid empty
// SecurityDynamicConfiguration.
if (cType == CType.TENANCYCONFIG) {
try {
SecurityDynamicConfiguration<?> empty = ConfigHelper.createEmptySdc(cType, ConfigurationRepository.getDefaultConfigVersion());
rs.put(cType, empty);
latch.countDown();
return;
} catch (Exception e) {
throw new RuntimeException(e);
}
}

if(cType == CType.AUDIT) {
// Audit configuration doc is not available in the index.
// Configuration cannot be hot-reloaded.
Expand Down Expand Up @@ -233,8 +246,6 @@ private SecurityDynamicConfiguration<?> toConfig(GetResponse singleGetResponse,
final long seqNo = singleGetResponse.getSeqNo();
final long primaryTerm = singleGetResponse.getPrimaryTerm();



if (ref == null || ref.length() == 0) {
log.error("Empty or null byte reference for {}", id);
return null;
Expand Down
3 changes: 2 additions & 1 deletion src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,10 +136,11 @@ public void run() {
ConfigHelper.uploadFile(client, cd+"roles_mapping.yml", securityIndex, CType.ROLESMAPPING, DEFAULT_CONFIG_VERSION);
ConfigHelper.uploadFile(client, cd+"internal_users.yml", securityIndex, CType.INTERNALUSERS, DEFAULT_CONFIG_VERSION);
ConfigHelper.uploadFile(client, cd+"action_groups.yml", securityIndex, CType.ACTIONGROUPS, DEFAULT_CONFIG_VERSION);
final boolean populateEmptyIfFileMissing = true;
Copy link
Contributor

@stephen-crawford stephen-crawford Feb 17, 2023

Choose a reason for hiding this comment

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

Should this be moved to a config constants file?

Copy link
Contributor Author

@abhivka7 abhivka7 Feb 20, 2023

Choose a reason for hiding this comment

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

Sure will do in next commit. Thanks for the suggestion.

Copy link
Member

@cliu123 cliu123 Feb 21, 2023

Choose a reason for hiding this comment

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

With this, would populateEmptyIfFileMissing always be true? When would it be false?

Copy link
Contributor Author

@abhivka7 abhivka7 Feb 21, 2023

Choose a reason for hiding this comment

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

It was already always true. We just moved the line of declaring this variable. From next commit onwards, we can change this from config constants file.

if(DEFAULT_CONFIG_VERSION == 2) {
ConfigHelper.uploadFile(client, cd+"tenants.yml", securityIndex, CType.TENANTS, DEFAULT_CONFIG_VERSION);
ConfigHelper.uploadFile(client, cd+"tenancy_config.yml", securityIndex, CType.TENANCYCONFIG, DEFAULT_CONFIG_VERSION, populateEmptyIfFileMissing);
Copy link
Member

@cwperks cwperks Feb 17, 2023

Choose a reason for hiding this comment

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

Should this only be populated if the file exists? I see tenants.yml does not create this config entry if no tenants.yml file is supplied.

}
final boolean populateEmptyIfFileMissing = true;
ConfigHelper.uploadFile(client, cd+"nodes_dn.yml", securityIndex, CType.NODESDN, DEFAULT_CONFIG_VERSION, populateEmptyIfFileMissing);
ConfigHelper.uploadFile(client, cd + "whitelist.yml", securityIndex, CType.WHITELIST, DEFAULT_CONFIG_VERSION, populateEmptyIfFileMissing);
ConfigHelper.uploadFile(client, cd + "allowlist.yml", securityIndex, CType.ALLOWLIST, DEFAULT_CONFIG_VERSION, populateEmptyIfFileMissing);
Expand Down
7 changes: 4 additions & 3 deletions src/main/java/org/opensearch/security/configuration/PrivilegesInterceptorImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
import org.opensearch.security.privileges.PrivilegesInterceptor;
import org.opensearch.security.resolver.IndexResolverReplacer.Resolved;
import org.opensearch.security.securityconf.DynamicConfigModel;
import org.opensearch.security.securityconf.TenancyConfigModel;
import org.opensearch.security.user.User;
import org.opensearch.threadpool.ThreadPool;

Expand Down Expand Up @@ -95,9 +96,10 @@ private boolean isTenantAllowed(final ActionRequest request, final String action
*/
@Override
public ReplaceResult replaceDashboardsIndex(final ActionRequest request, final String action, final User user, final DynamicConfigModel config,
final TenancyConfigModel tenancyconfig,
final Resolved requestedResolved, final Map<String, Boolean> tenants) {

final boolean enabled = config.isDashboardsMultitenancyEnabled();//config.dynamic.kibana.multitenancy_enabled;
final boolean enabled = tenancyconfig.isDashboardsMultitenancyEnabled();
Copy link

@expani expani Feb 17, 2023

Choose a reason for hiding this comment

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

nit: Camel Case

Copy link
Contributor Author

@abhivka7 abhivka7 Feb 20, 2023

Choose a reason for hiding this comment

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

Thanks @anijain-Amazon for finding this miss. Will update in next commit.

Copy link
Member

@cliu123 cliu123 Feb 21, 2023

Choose a reason for hiding this comment

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

With this change, would config.dynamic.kibana.multitenancy_enabled be ignored? Would this work in mixed clusters? Based on my understanding, security index is node-scoped. If there're 2 settings(the old and the new), could there be 2 different values(false and true) of multitenancy_enabled setting in a mixed cluster?

Copy link
Contributor Author

@abhivka7 abhivka7 Feb 21, 2023

Choose a reason for hiding this comment

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

@cliu123 going forward we will only use config.dynamic.kibana.multitenancy_enabled and this will get priority over config.dynamic.kibana.multitenancy_enabled.

If there're 2 settings(the old and the new), could there be 2 different values(false and true) of multitenancy_enabled setting in a mixed cluster?

In this case we will give priority to new settings.

Copy link
Collaborator

@RyanL1997 RyanL1997 Feb 21, 2023

Choose a reason for hiding this comment

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

Hi @abhivka7, thanks for this contribution. I see the point that you are trying to extend the original multi-tenancy feature with this change. So do we need a feature flag for this dynamically enable/disable multi-tenancy feature? Or it will be applied directly by enable multi tenancy?

Copy link
Contributor Author

@abhivka7 abhivka7 Mar 17, 2023

Choose a reason for hiding this comment

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

We don't meed a flag for these changes. We will keep multi-tenancy enabled for new domains and changes can be applied directly.


if (!enabled) {
return CONTINUE_EVALUATION_REPLACE_RESULT;
Expand All @@ -116,6 +118,7 @@ public ReplaceResult replaceDashboardsIndex(final ActionRequest request, final S
//intercept when requests are not made by the kibana server and if the kibana index/alias (.kibana) is the only index/alias involved
final boolean dashboardsIndexOnly = !user.getName().equals(dashboardsServerUsername) && resolveToDashboardsIndexOrAlias(requestedResolved, dashboardsIndexName);
final boolean isTraceEnabled = log.isTraceEnabled();

Copy link
Member

@cliu123 cliu123 Feb 21, 2023

Choose a reason for hiding this comment

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

Please remove the empty line and avoid format-only changes.

if (requestedTenant == null || requestedTenant.length() == 0) {
if (isTraceEnabled) {
log.trace("No tenant, will resolve to " + dashboardsIndexName);
Expand All @@ -124,7 +127,6 @@ public ReplaceResult replaceDashboardsIndex(final ActionRequest request, final S
if (dashboardsIndexOnly && !isTenantAllowed(request, action, user, tenants, "global_tenant")) {
return ACCESS_DENIED_REPLACE_RESULT;
}

return CONTINUE_EVALUATION_REPLACE_RESULT;
}

Expand Down Expand Up @@ -175,7 +177,6 @@ public ReplaceResult replaceDashboardsIndex(final ActionRequest request, final S
}

}

return CONTINUE_EVALUATION_REPLACE_RESULT;
}

Expand Down
12 changes: 7 additions & 5 deletions src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,20 +109,21 @@ protected void handleApiRequest(final RestChannel channel, final RestRequest req
try {
// validate additional settings, if any
AbstractConfigurationValidator validator = getValidator(request, request.content());
if (!validator.validate()) {
if (validator!= null && !validator.validate()) {
Copy link

@expani expani Feb 17, 2023

Choose a reason for hiding this comment

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

If we change the condition to validator == null || !validator.validate() then the use of checks in the lines below is not required.

Copy link
Contributor Author

@abhivka7 abhivka7 Feb 20, 2023

Choose a reason for hiding this comment

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

This was added while testing and I forgot to remove it. Thanks @anijain-Amazon for pointing this out. Will remove this check condition in next commit.

Copy link
Member

@cwperks cwperks Feb 17, 2023

Choose a reason for hiding this comment

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

I see that this PR introduces a validator. When will the validator ever be null? Are the changes in this file needed?

Copy link
Contributor Author

@abhivka7 abhivka7 Feb 20, 2023

Choose a reason for hiding this comment

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

This was added while testing and I forgot to remove it. Thanks @cwperks for pointing this out. Will remove this check condition in next commit.

request.params().clear();
badRequestResponse(channel, validator);
return;
}
JsonNode jsonNode = validator == null ? null : validator.getContentAsNode();
switch (request.method()) {
case DELETE:
handleDelete(channel,request, client, validator.getContentAsNode()); break;
handleDelete(channel,request, client, jsonNode); break;
case POST:
handlePost(channel,request, client, validator.getContentAsNode());break;
handlePost(channel,request, client, jsonNode);break;
case PUT:
handlePut(channel,request, client, validator.getContentAsNode());break;
handlePut(channel,request, client, jsonNode);break;
case GET:
handleGet(channel,request, client, validator.getContentAsNode());break;
handleGet(channel,request, client, jsonNode);break;
default:
throw new IllegalArgumentException(request.method() + " not supported");
}
Expand Down Expand Up @@ -481,6 +482,7 @@ protected void successResponse(RestChannel channel) {
try {
final XContentBuilder builder = channel.newBuilder();
builder.startObject();
builder.endObject();
Copy link

@expani expani Feb 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Were we not calling builder.endObject(); before ?

Copy link
Contributor Author

@abhivka7 abhivka7 Feb 20, 2023

Choose a reason for hiding this comment

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

No we were not doing it. That was a miss I guess.

channel.sendResponse(
new BytesRestResponse(RestStatus.OK, builder));
} catch (IOException e) {
Expand Down
3 changes: 2 additions & 1 deletion src/main/java/org/opensearch/security/dlic/rest/api/Endpoint.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,5 +29,6 @@ public enum Endpoint {
WHITELIST,
ALLOWLIST,
NODESDN,
SSL;
SSL,
TENANCYCONFIG;
}
1 change: 1 addition & 0 deletions src/main/java/org/opensearch/security/dlic/rest/api/RestApiAdminPrivilegesEvaluator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ default String build() {
.put(Endpoint.ROLES, action -> buildEndpointPermission(Endpoint.ROLES))
.put(Endpoint.ROLESMAPPING, action -> buildEndpointPermission(Endpoint.ROLESMAPPING))
.put(Endpoint.TENANTS, action -> buildEndpointPermission(Endpoint.TENANTS))
.put(Endpoint.TENANCYCONFIG, action -> buildEndpointPermission(Endpoint.TENANCYCONFIG))
.put(Endpoint.SSL, action -> {
switch (action) {
case CERTS_INFO_ACTION:
Expand Down
1 change: 1 addition & 0 deletions src/main/java/org/opensearch/security/dlic/rest/api/SecurityRestApiActions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ public static Collection<RestHandler> getHandler(final Settings settings,
handlers.add(new ValidateApiAction(settings, configPath, controller, client, adminDns, cr, cs, principalExtractor, evaluator, threadPool, auditLog));
handlers.add(new AccountApiAction(settings, configPath, controller, client, adminDns, cr, cs, principalExtractor, evaluator, threadPool, auditLog));
handlers.add(new NodesDnApiAction(settings, configPath, controller, client, adminDns, cr, cs, principalExtractor, evaluator, threadPool, auditLog));
handlers.add(new TenancyConfigAction(settings, configPath, controller, client, adminDns, cr, cs, principalExtractor, evaluator, threadPool, auditLog));
handlers.add(new WhitelistApiAction(settings, configPath, controller, client, adminDns, cr, cs, principalExtractor, evaluator, threadPool, auditLog));
handlers.add(new AllowlistApiAction(settings, configPath, controller, client, adminDns, cr, cs, principalExtractor, evaluator, threadPool, auditLog));
handlers.add(new AuditApiAction(settings, configPath, controller, client, adminDns, cr, cs, principalExtractor, evaluator, threadPool, auditLog));
Expand Down
104 changes: 104 additions & 0 deletions src/main/java/org/opensearch/security/dlic/rest/api/TenancyConfigAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
package org.opensearch.security.dlic.rest.api;

import java.io.IOException;
import java.nio.file.Path;
import java.util.Collections;
import java.util.List;

import com.fasterxml.jackson.databind.JsonNode;
import com.google.common.collect.ImmutableList;

import org.opensearch.client.Client;
import org.opensearch.cluster.service.ClusterService;
import org.opensearch.common.bytes.BytesReference;
import org.opensearch.common.inject.Inject;
import org.opensearch.common.settings.Settings;
import org.opensearch.rest.RestChannel;
import org.opensearch.rest.RestController;
import org.opensearch.rest.RestRequest;
import org.opensearch.rest.RestRequest.Method;
import org.opensearch.security.auditlog.AuditLog;
import org.opensearch.security.configuration.AdminDNs;
import org.opensearch.security.configuration.ConfigurationRepository;
import org.opensearch.security.dlic.rest.validation.AbstractConfigurationValidator;
import org.opensearch.security.dlic.rest.validation.TenancyConfigValidator;
import org.opensearch.security.dlic.rest.validation.SecurityConfigValidator;
import org.opensearch.security.privileges.PrivilegesEvaluator;
import org.opensearch.security.securityconf.impl.CType;
import org.opensearch.security.securityconf.impl.SecurityDynamicConfiguration;
import org.opensearch.security.ssl.transport.PrincipalExtractor;
import org.opensearch.security.support.ConfigConstants;
import org.opensearch.threadpool.ThreadPool;

import static org.opensearch.security.dlic.rest.support.Utils.addRoutesPrefix;

public class TenancyConfigAction extends PatchableResourceApiAction{

private static final List<Route> allRoutes = new ImmutableList.Builder<Route>()
.addAll(addRoutesPrefix(
ImmutableList.of(
new Route(Method.GET, "/tenancyconfig/"),
new Route(Method.PUT, "/tenancyconfig/"),
new Route(Method.PATCH, "/tenancyconfig/")
)
Copy link
Collaborator

@shikharj05 shikharj05 Feb 20, 2023

Choose a reason for hiding this comment

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

Is DELETE not required?

Copy link
Contributor Author

@abhivka7 abhivka7 Feb 20, 2023

Choose a reason for hiding this comment

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

I don't think there will be a use case where we require DELETE. We should not delete tenancy_config and we can replace existing config using PATCH.
Correct me if I am wrong @devardee

))
.build();

@Inject
public TenancyConfigAction(Settings settings, Path configPath, RestController controller, Client client,
AdminDNs adminDNs, ConfigurationRepository cl, ClusterService cs,
PrincipalExtractor principalExtractor, PrivilegesEvaluator evaluator,
ThreadPool threadPool, AuditLog auditLog) {
super(settings, configPath, controller, client, adminDNs, cl, cs, principalExtractor, evaluator, threadPool, auditLog);
}

@Override
public List<Route> routes() {
return allRoutes;
}

@Override
protected void handleGet(RestChannel channel, RestRequest request, Client client, final JsonNode content) throws IOException{

final SecurityDynamicConfiguration<?> configuration = load(getConfigName(), true);
filter(configuration);
successResponse(channel,configuration);
Copy link
Contributor

@stephen-crawford stephen-crawford Feb 17, 2023

Choose a reason for hiding this comment

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

If you want to do anything special for responses you will need to add onResponse logic here.

Copy link
Contributor Author

@abhivka7 abhivka7 Feb 20, 2023

Choose a reason for hiding this comment

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

@scrawfor99 We do not want to do anything special with our responses. I don't think we need that logic here.

}


@Override
protected AbstractConfigurationValidator getValidator(RestRequest request, BytesReference ref, Object... params) {
return new TenancyConfigValidator(request, ref, this.settings, params);
}

@Override
protected void handleDelete(RestChannel channel, final RestRequest request, final Client client, final JsonNode content) throws IOException{
notImplemented(channel, Method.DELETE);
}

@Override
protected void handlePut(RestChannel channel, final RestRequest request, final Client client, final JsonNode content) throws IOException{
successResponse(channel);
Copy link

@expani expani Feb 17, 2023

Choose a reason for hiding this comment

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

If this is not doing anything, shouldn't it be notImplemented as well ?

Copy link
Contributor Author

@abhivka7 abhivka7 Feb 20, 2023

Choose a reason for hiding this comment

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

We want to enable PUT request for tenancyConfig. That's why we are not using notImplemented .

Copy link
Member

@DarshitChanpura DarshitChanpura Feb 24, 2023

Choose a reason for hiding this comment

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

For my understanding, why is it returning successResponse directly?

}

@Override
protected String getResourceName() {
return null;
}

@Override
protected CType getConfigName() {
return CType.TENANCYCONFIG;
}

@Override
protected boolean hasPermissionsToCreate(final SecurityDynamicConfiguration<?> dynamicConfigFactory,
final Object content, final String resourceName) {
return true;
}

@Override
protected Endpoint getEndpoint() {
return Endpoint.TENANCYCONFIG;
}
}
28 changes: 28 additions & 0 deletions src/main/java/org/opensearch/security/dlic/rest/validation/TenancyConfigValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package org.opensearch.security.dlic.rest.validation;

import org.opensearch.common.bytes.BytesReference;
import org.opensearch.common.settings.Settings;
import org.opensearch.rest.RestRequest;

/*
* SPDX-License-Identifier: Apache-2.0
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*
* Modifications Copyright OpenSearch Contributors. See
* GitHub history for details.
*/

public class TenancyConfigValidator extends AbstractConfigurationValidator {

public TenancyConfigValidator(final RestRequest request, final BytesReference ref, final Settings opensearchSettings, Object... param) {
super(request, ref, opensearchSettings, param);
this.payloadMandatory = true;

allowedKeys.put("multitenancy_enabled", DataType.BOOLEAN);
allowedKeys.put("private_tenant_enabled", DataType.BOOLEAN);
allowedKeys.put("default_tenant", DataType.STRING);
}
}
17 changes: 14 additions & 3 deletions src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@
import org.opensearch.security.resolver.IndexResolverReplacer.Resolved;
import org.opensearch.security.securityconf.ConfigModel;
import org.opensearch.security.securityconf.DynamicConfigModel;
import org.opensearch.security.securityconf.TenancyConfigModel;
import org.opensearch.security.securityconf.SecurityRoles;
import org.opensearch.security.support.ConfigConstants;
import org.opensearch.security.support.WildcardMatcher;
Expand Down Expand Up @@ -134,6 +135,7 @@ public class PrivilegesEvaluator {
private final boolean dlsFlsEnabled;
private final boolean dfmEmptyOverwritesAll;
private DynamicConfigModel dcm;
private TenancyConfigModel tcm;
private final NamedXContentRegistry namedXContentRegistry;

public PrivilegesEvaluator(final ClusterService clusterService, final ThreadPool threadPool,
Expand Down Expand Up @@ -175,6 +177,11 @@ public void onDynamicConfigModelChanged(DynamicConfigModel dcm) {
this.dcm = dcm;
}

@Subscribe
public void onTenancyConfigModelChanged(TenancyConfigModel tcm) {
this.tcm = tcm;
}

private SecurityRoles getSecurityRoles(Set<String> roles) {
return configModel.getSecurityRoles().filter(roles);
}
Expand Down Expand Up @@ -328,7 +335,7 @@ public PrivilegesEvaluatorResponse evaluate(final User user, String action0, fin
} else {
if(privilegesInterceptor.getClass() != PrivilegesInterceptor.class) {

final PrivilegesInterceptor.ReplaceResult replaceResult = privilegesInterceptor.replaceDashboardsIndex(request, action0, user, dcm, requestedResolved,
final PrivilegesInterceptor.ReplaceResult replaceResult = privilegesInterceptor.replaceDashboardsIndex(request, action0, user, dcm, tcm, requestedResolved,
mapTenants(user, mappedRoles));

if (isDebugEnabled) {
Expand Down Expand Up @@ -412,7 +419,7 @@ public PrivilegesEvaluatorResponse evaluate(final User user, String action0, fin

if(privilegesInterceptor.getClass() != PrivilegesInterceptor.class) {

final PrivilegesInterceptor.ReplaceResult replaceResult = privilegesInterceptor.replaceDashboardsIndex(request, action0, user, dcm, requestedResolved, mapTenants(user, mappedRoles));
final PrivilegesInterceptor.ReplaceResult replaceResult = privilegesInterceptor.replaceDashboardsIndex(request, action0, user, dcm, tcm, requestedResolved, mapTenants(user, mappedRoles));

if (isDebugEnabled) {
log.debug("Result from privileges interceptor: {}", replaceResult);
Expand Down Expand Up @@ -522,9 +529,13 @@ public Set<String> getAllConfiguredTenantNames() {

public boolean multitenancyEnabled() {
return privilegesInterceptor.getClass() != PrivilegesInterceptor.class
&& dcm.isDashboardsMultitenancyEnabled();
&& tcm.isDashboardsMultitenancyEnabled();
Copy link
Member

@DarshitChanpura DarshitChanpura Feb 24, 2023

Choose a reason for hiding this comment

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

Can we remove this method from DynamicConfigModel and its inheritor DynamicConfigModelV7 since it is not used anymore?

}

public boolean privateTenantEnabled(){ return tcm.isDashboardsPrivateTenantEnabled(); }

public String dashboardsDefaultTenant(){ return tcm.dashboardsDefaultTenant(); }

public boolean notFailOnForbiddenEnabled() {
return privilegesInterceptor.getClass() != PrivilegesInterceptor.class
&& dcm.isDnfofEnabled();
Expand Down
4 changes: 3 additions & 1 deletion src/main/java/org/opensearch/security/privileges/PrivilegesInterceptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import org.opensearch.common.util.concurrent.ThreadContext;
import org.opensearch.security.resolver.IndexResolverReplacer.Resolved;
import org.opensearch.security.securityconf.DynamicConfigModel;
import org.opensearch.security.securityconf.TenancyConfigModel;
import org.opensearch.security.user.User;
import org.opensearch.threadpool.ThreadPool;

Expand Down Expand Up @@ -74,10 +75,11 @@ public PrivilegesInterceptor(final IndexNameExpressionResolver resolver, final C
}

public ReplaceResult replaceDashboardsIndex(final ActionRequest request, final String action, final User user, final DynamicConfigModel config,
final TenancyConfigModel tenancyconfig,
final Resolved requestedResolved, final Map<String, Boolean> tenants) {
throw new RuntimeException("not implemented");
}

protected final ThreadContext getThreadContext() {
return threadPool.getThreadContext();
}
Expand Down
3 changes: 3 additions & 0 deletions src/main/java/org/opensearch/security/rest/SecurityInfoAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,9 @@ public void accept(RestChannel channel) throws Exception {
builder.field("principal", (String)threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_SSL_PRINCIPAL));
builder.field("peer_certificates", certs != null && certs.length > 0 ? certs.length + "" : "0");
builder.field("sso_logout_url", (String)threadContext.getTransient(ConfigConstants.SSO_LOGOUT_URL));
builder.field("tenancy_enabled", evaluator.multitenancyEnabled());
Copy link
Member

@DarshitChanpura DarshitChanpura Feb 24, 2023

Choose a reason for hiding this comment

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

the name tenancy_enabled is slightly misleading. Can we have multi_tenancy_enabled instead?

Copy link
Contributor Author

@abhivka7 abhivka7 Mar 17, 2023

Choose a reason for hiding this comment

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

Ok will make this change.

builder.field("private_tenant_enabled", evaluator.privateTenantEnabled());
builder.field("default_tenant", evaluator.dashboardsDefaultTenant());

if(user != null && verbose) {
try {
Expand Down
Loading