Add conditional email access feature changes #294
Merged
tcdlpds
merged 2 commits from rajitha/device-mgt-core:conditional-access
into master
10 months ago
Loading…
Reference in new issue
There is no content yet.
Delete Branch 'rajitha/device-mgt-core:conditional-access'
Deleting a branch is permanent. It CANNOT be undone. Continue?
Purpose
WIP: Add conditional email access feature changesto Add conditional email access feature changes 12 months ago901186fb5d
to6010c8143d
11 months agopublic static void init(CEADatasourceConfiguration ceaDatasourceConfiguration) {
dataSource = resolveDatasource(ceaDatasourceConfiguration);
if (dataSource == null) throw new NullPointerException("Datasource is null");
Enclose if condition with curly braces
}
private static DataSource resolveDatasource(CEADatasourceConfiguration ceaDatasourceConfiguration) {
if (ceaDatasourceConfiguration == null)
Enclose if condition with curly braces
if (ceaDatasourceConfiguration == null)
throw new NullPointerException("Null is retrieved for Datasource configuration");
JNDILookupDefinition jndiLookupDefinition = ceaDatasourceConfiguration.getJndiLookupDefinition();
if (jndiLookupDefinition == null)
Enclose if condition with curly braces
String datasourceName = jndiLookupDefinition.getJndiName();
List<JNDILookupDefinition.JNDIProperty>
jndiProperties = ceaDatasourceConfiguration.getJndiLookupDefinition().getJndiProperties();
if (jndiProperties == null || jndiProperties.isEmpty()) return lookupDatasource(datasourceName);
Enclose if condition with curly braces
}
public static CEAPolicyDAO getCEAPolicyDAO() {
if (productName == null) throw new IllegalStateException("Database is not initialized properly");
Enclose if condition with curly braces
import org.wso2.carbon.context.PrivilegedCarbonContext;
import java.nio.charset.StandardCharsets;
import java.sql.*;
Remove wildcard imports
}
private CEAPolicyDTO toCEAPolicyDTO(CEAPolicy ceaPolicy) {
if (ceaPolicy == null) throw new NullPointerException("CEAPolicy can't be null");
Enclose if condition with curly braces
ceaPolicyDTO.setCreatedTimestamp(new Timestamp(ceaPolicy.getCreated().getTime()));
ceaPolicyDTO.setUpdatedTimestamp(new Timestamp(ceaPolicy.getLastUpdated().getTime()));
ceaPolicyDTO.setTenantId(ceaPolicy.getTenantId());
if (ceaPolicy.getLastSynced() != null)
Enclose if condition with curly braces
}
private CEAPolicy toCEAPolicy(CEAPolicyDTO ceaPolicyDTO) {
if (ceaPolicyDTO == null) throw new NullPointerException("CEAPolicyDTO can't be null");
Enclose if condition with curly braces
ceaPolicy.setLastUpdated(new Date(ceaPolicyDTO.getUpdatedTimestamp().getTime()));
ceaPolicy.setSynced(ceaPolicyDTO.isSynced());
ceaPolicy.setTenantId(ceaPolicyDTO.getTenantId());
if (ceaPolicyDTO.getLastSyncedTimestamp() != null)
Enclose if condition with curly braces
private EnforcementServiceManager enforcementServiceManager;
private TaskService taskService;
private CEAPolicyMonitoringTaskManager ceaPolicyMonitoringTaskManager;
CEAManagementDataHolder() {
Add new line after instance variables
getInstance().getCeaPolicyMonitoringTaskManager();
CEAConfigManager ceaConfigManager = CEAConfigManager.getInstance();
CEAConfiguration ceaConfiguration = ceaConfigManager.getCeaConfiguration();
if (ceaPolicyMonitoringTaskManager == null)
Enclose if condition with curly braces
CEAPolicyAlreadyExistsException {
try {
CEAPolicyManagementDAOFactory.openConnection();
if (ceaPolicyDAO.retrieveCEAPolicy() != null) {
Enclose if condition with curly braces
try {
CEAPolicyManagementDAOFactory.openConnection();
CEAPolicy existingCeaPolicy = ceaPolicyDAO.retrieveCEAPolicy();
if (existingCeaPolicy == null) throw new CEAPolicyNotFoundException("CEA policy not found");
Enclose if condition with curly braces
try {
CEAPolicyMonitoringTaskManager ceaPolicyMonitoringTaskManager = CEAManagementDataHolder.
getInstance().getCeaPolicyMonitoringTaskManager();
if (ceaPolicyMonitoringTaskManager == null)
Enclose if condition with curly braces
throw new IllegalStateException("CEA policy monitoring task manager not initialized properly");
CEAPolicyManagementDAOFactory.openConnection();
CEAPolicy existingCeaPolicy = ceaPolicyDAO.retrieveCEAPolicy();
if (existingCeaPolicy == null) throw new CEAPolicyNotFoundException("CEA policy not found");
Enclose if condition with curly braces
@Override
public void startTask(long monitoringFrequency) throws CEAPolicyMonitoringTaskManagerException {
if (monitoringFrequency <= 0) throw new CEAPolicyMonitoringTaskManagerException("Invalid monitoring frequency");
Enclose if condition with curly braces
if (monitoringFrequency <= 0) throw new CEAPolicyMonitoringTaskManagerException("Invalid monitoring frequency");
TaskService taskService = CEAManagementDataHolder.getInstance().getTaskService();
if (taskService == null) throw new IllegalStateException("Task service is not initialized");
Enclose if condition with curly braces
import com.google.gson.JsonArray;
import com.google.gson.JsonElement;
import com.google.gson.JsonObject;
import io.entgra.device.mgt.core.cea.mgt.common.bean.*;
Remove wildcard imports
import java.text.ParseException;
import java.text.SimpleDateFormat;
import java.util.*;
Remove wildcard imports
Set<EmailOutlookAccessPolicy> emailOutlookAccessPolicies = ceaPolicy.getAccessPolicy().getEmailOutlookAccessPolicy();
if (emailOutlookAccessPolicies.contains(EmailOutlookAccessPolicy.NOT_CONFIGURED)) {
if (log.isDebugEnabled()) {
log.info("CEA email outlook policy not configured, but the support is available in " +
Change to debug log
if (ceaPolicy.getAccessPolicy().getPOPIMAPAccessPolicy().
equalsName(EmailOutlookAccessPolicy.NOT_CONFIGURED.name())) {
if (log.isDebugEnabled()) {
log.info("CEA POP/IMAP policy not configured, but support is available in " +
Change to debug log
if (ceaPolicy.getAccessPolicy().getWebOutlookAccessPolicy().
equalsName(WebOutlookAccessPolicy.NOT_CONFIGURED.name())) {
if (log.isDebugEnabled()) {
log.info("CEA Outlook web access policy not configured, but support is available in " +
Change to debug log
throw new CEAEnforcementException(msg);
}
if (powershellResponse.getResponseBody() == null) return Collections.emptyList();
Enclose if condition with curly braces
package io.entgra.device.mgt.core.cea.mgt.enforce.Impl.gateway;
import com.microsoft.aad.msal4j.*;
Remove wildcard imports
import org.apache.commons.logging.LogFactory;
import java.net.MalformedURLException;
import java.util.*;
Remove wildcard imports
import org.wso2.carbon.user.api.UserStoreManager;
import org.wso2.carbon.user.core.service.RealmService;
import java.util.*;
Remove wildcard imports
throw new IllegalStateException(msg);
}
List<Device> devices = deviceManagementProviderService.getEnrolledDevicesSince(since);
if (devices == null) return new ArrayList<>();
Enclose if condition with curly braces
throw new IllegalStateException(msg);
}
List<Device> devices = deviceManagementProviderService.getEnrolledDevicesPriorTo(priorTo);
if (devices == null) return new ArrayList<>();
Enclose if condition with curly braces
import io.entgra.device.mgt.core.cea.mgt.enforce.exception.PowershellExecutionException;
import io.entgra.device.mgt.core.cea.mgt.enforce.util.shell.Powershell;
import java.io.*;
Remove wildcard imports
import io.entgra.device.mgt.core.device.mgt.api.jaxrs.beans.CEAPolicyWrapper;
import io.entgra.device.mgt.core.device.mgt.api.jaxrs.beans.ErrorResponse;
import io.entgra.device.mgt.core.device.mgt.api.jaxrs.util.Constants;
import io.swagger.annotations.*;
Remove wildcard imports
import io.entgra.device.mgt.core.device.mgt.api.jaxrs.util.Constants;
import io.swagger.annotations.*;
import javax.ws.rs.*;
Remove wildcard imports
package io.entgra.device.mgt.core.device.mgt.api.jaxrs.service.impl.admin;
import io.entgra.device.mgt.core.cea.mgt.common.bean.*;
Remove wildcard imports
package io.entgra.device.mgt.core.device.mgt.api.jaxrs.service.impl.admin;
import io.entgra.device.mgt.core.cea.mgt.common.bean.*;
import io.entgra.device.mgt.core.cea.mgt.common.bean.enums.*;
Remove wildcard imports
import org.apache.commons.logging.LogFactory;
import org.apache.http.HttpStatus;
import javax.ws.rs.*;
Remove wildcard imports
private List<ActiveSyncServerUIConfiguration> constructActiveSyncServerConfigurations
(List<ServerUIConfiguration> serverUIConfigurations) {
List<ActiveSyncServerUIConfiguration> activeSyncServerUIConfigurations = new ArrayList<>();
if (serverUIConfigurations == null) return activeSyncServerUIConfigurations;
Enclose if condition with curly braces
import java.util.Date;
import java.util.List;
public interface CEAPolicyDAO {
Add doc comments to the class and method definitions
}
public static void beginTransaction() throws CEAPolicyManagementDAOException {
Connection connection = currentConnection.get();
What is the reason for obtaining connection here? Need to add the null check
preparedStatement.setInt(1, tenantId);
try (ResultSet resultSet = preparedStatement.executeQuery()) {
while (resultSet.next()) {
CEAPolicyDTO ceaPolicyDTO = new CEAPolicyDTO();
Move the object reference declaration outside the loop
try (PreparedStatement preparedStatement = connection.prepareStatement(query)) {
try (ResultSet resultSet = preparedStatement.executeQuery()) {
while (resultSet.next()) {
CEAPolicyDTO ceaPolicyDTO = new CEAPolicyDTO();
Move the object reference declaration outside the loop
if (ceaPolicy == null) throw new NullPointerException("CEAPolicy can't be null");
CEAPolicyDTO ceaPolicyDTO = new CEAPolicyDTO();
CEAPolicyContent ceaPolicyContent = new CEAPolicyContent();
Gson gson = new Gson();
What if the Gson object declared as a static field of the class instead of instantiating it inside the method?
private CEAPolicy toCEAPolicy(CEAPolicyDTO ceaPolicyDTO) {
if (ceaPolicyDTO == null) throw new NullPointerException("CEAPolicyDTO can't be null");
CEAPolicy ceaPolicy = new CEAPolicy();
Gson gson = new Gson();
What if the Gson object declared as a static field of the class instead of instantiating it inside the method?
}
}
protected void setDataSourceService(DataSourceService dataSourceService) {
Why there's no method body for this? If this DataSourceService is not required to initiate the CEAManagementServiceComponent, better to remove the scr reference.
ceaPolicyDAO = CEAPolicyManagementDAOFactory.getCEAPolicyDAO();
}
public static CEAManagerImpl getInstance() {
Why there's public constructor to instantiate the class and using this static method to return the instance?
getInstance().getCeaPolicyMonitoringTaskManager();
CEAConfigManager ceaConfigManager = CEAConfigManager.getInstance();
CEAConfiguration ceaConfiguration = ceaConfigManager.getCeaConfiguration();
if (ceaPolicyMonitoringTaskManager == null)
Move this null check to line 71 to avoid unnecessary resource allocation
CEAPolicyMonitoringTaskManager ceaPolicyMonitoringTaskManager = CEAManagementDataHolder.
getInstance().getCeaPolicyMonitoringTaskManager();
if (ceaPolicyMonitoringTaskManager == null)
throw new IllegalStateException("CEA policy monitoring task manager not initialized properly");
This error should be properly handled with a proper message since if the IllegalStateException is thrown, the deletion of the CEAPolicy will not happen.
}
@Override
public void enforce() throws CEAPolicyOperationException {
Better to add doc comments for the future references
powershell = Powershell.getPowershell();
}
public static ExchangeOnlineCEAEnforcementServiceImpl getInstance() throws UnsupportedOsException {
Method should be synchronized or need to do the null check again inside the synchronized block to prevent creation of two instances of the ExchangeOnlineCEAEnforcementServiceImpl object.
List<MailboxProfile> mailboxProfiles = generateMailboxPolicies(validActiveSyncDevices,
notValidActiveSyncDevices);
for (MailboxProfile mailboxProfile : mailboxProfiles) {
PowershellCommand powershellCommand = getCommand(Parser.COMMAND_SetCASMailbox.COMMAND, activeSyncServer);
Declare the object reference out of the loop
List<ActiveSyncDevice> activeSyncBlockedDevices) {
List<MailboxProfile> mailboxProfiles = new ArrayList<>();
for (ActiveSyncDevice activeSyncDevice : activeSyncAllowedDevices) {
MailboxProfile mailboxProfile = new MailboxProfile();
Declare the object reference out of the loop
}
for (ActiveSyncDevice activeSyncDevice : activeSyncBlockedDevices) {
MailboxProfile mailboxProfile = new MailboxProfile();
Declare the object reference out of the loop
private List<ActiveSyncDevice> constructActiveSyncDeviceList(PowershellResponse powershellResponse)
throws CEAEnforcementException {
if (powershellResponse == null) {
String msg = "Powershell response can't be null";
Not required to use additional variable for the message
}
if (!powershellResponse.isSuccess()) {
String msg = "Powershell request failed while getting active sync devices";
Not required to use additional variable for the message
if (powershellResponse.getResponseBody() == null) return Collections.emptyList();
if (!powershellResponse.getResponseBody().isJsonArray()) {
String msg = "Unexpected result retrieve when getting active sync devices";
Not required to use additional variable for the message
for (JsonElement element : elements) {
try {
JsonObject deviceJsonObject = element.getAsJsonObject();
ActiveSyncDevice activeSyncDevice = new ActiveSyncDevice();
Declare the both object references out of the loop
public interface Powershell {
String OS = System.getProperty("os.name").toLowerCase();
boolean IS_UNIX = (OS.indexOf("nix") >= 0 || OS.indexOf("nux") >= 0 || OS.indexOf("aix") > 0);
Since these IS_UNIX and IS_WINDOWS variables are not used in any subclasses, can't we declare them inside the getPowershell method?
Yes we can, but through out the application run time OS remain same, so we can improve the performance by putting OS checking in static fields. Bcz static will ensure to run the OS checking once.
}
public static CEAManagementService getCEAManagementService() {
PrivilegedCarbonContext ctx = PrivilegedCarbonContext.getThreadLocalCarbonContext();
Use a data holder to store the CEAManagementService and return it when invoking getCEAManagementService to prevent resolving the service each time via the OSGi context.
5f7d3712cc
to95d3080a7a
11 months ago95d3080a7a
to8eb6d6365d
10 months ago3b7cbc88cc
to064d953288
10 months ago/*
* Copyright (c) 2018 - 2023, Entgra (Pvt) Ltd. (http://www.entgra.io) All Rights Reserved.
Correct the licence year.
int tenantId = PrivilegedCarbonContext.getThreadLocalCarbonContext().getTenantId();
ceaPolicy.setTenantId(tenantId);
CEAPolicyDTO ceaPolicyDTO = toCEAPolicyDTO(ceaPolicy);
String query = "INSERT INTO DM_CEA_POLICIES (POLICY_CONTENT, CREATED_TIMESTAMP, UPDATED_TIMESTAMP, TENANT_ID) " +
Try enhancing the readability as follows while defining the query format.
You will see the importance of following this format when you defining a complex query, Hence practice this from the begining.
private CEAPolicy toCEAPolicy(CEAPolicyDTO ceaPolicyDTO) {
if (ceaPolicyDTO == null) {
throw new NullPointerException("CEAPolicyDTO can't be null");
Since NullPointerExceptions in Java are typically thrown as programming errors rather than recoverable exceptions, it is widely seen as bad practice to throw them. Any attempt by an application to use a null object reference results in the NullPointerException being thrown. This typically implies that the object is not properly initialized since the code is attempting to access a null field or call a function on an object that is not initialized.
Checking for null values and managing them appropriately is generally a preferable way to design your code to prevent null pointer exceptions. To do this, you may need to throw a more focused exception that explains the mistake and its possible solutions, use the null object pattern, or return a default value.
"hence aborting CEA policy deleting procedure";
throw new IllegalStateException(msg);
}
CEAPolicyManagementDAOFactory.openConnection();
This should be the first statement of the try block. Because if error throws in upper code block then finally block will be executed without opening a connection. Hence Handle upper part of the code in separate try block and the connection opening statement should be the first statement of this try block.
}
} catch (CEAManagementException e) {
String msg = "Error occurred while executing dynamic partitioned task for the CEA policy monitoring";
It is not required to define msg variable here.
String msg = "Error occurred while executing dynamic partitioned task for the CEA policy monitoring";
log.error(msg, e);
} catch (CEAConfigManagerException e) {
String msg = "Error occurred while retrieving CEA configuration";
It is not required to define msg variable here.
ceaPolicyOperation.enforce();
status = true;
} catch (EnforcementServiceManagerException | CEAPolicyOperationException e) {
String msg = "Error occurred while enforcing the CEA access policy for the tenant id" + ceaPolicy.getTenantId();
It is not required to define msg variable here.
try {
ceaManager.updateSyncStatus(status, syncedStartTime);
} catch (CEAManagementException e) {
String msg = "Error occurred while recording sync status";
It is not required to define msg variable here.
a3f60cf3c0
to2ad8f54a46
10 months ago}
@Enforce
public void enforceConditionalAccessPolicy(CEAPolicy ceaPolicy) throws CEAEnforcementException {
It seems in this method have repeating logic that can be moved to private method and improve the readability of the code.
}
}
private PowershellCommand getCommand(String command, ActiveSyncServer activeSyncServer)
Add Java Doc comment
return commandBuilder.build();
}
private PowershellCommand toAllMailboxesCommand(PowershellCommand command,
Add Java Doc comment
return getEXOMailbox;
}
private PowershellRequest getPowershellRequest(PowershellCommand command) {
Add Java Doc comment
return powershellRequest;
}
private List<MailboxProfile> generateMailboxPolicies(List<ActiveSyncDevice> activeSyncAllowedDevices,
Add Java Doc comment
return mailboxProfiles;
}
private List<ActiveSyncDevice> constructActiveSyncDeviceList(PowershellResponse powershellResponse)
Add Java Doc comment
return activeSyncDevices;
}
private List<ActiveSyncDevice> getConnectedActiveSyncDevicesAfter(Date after, ActiveSyncServer activeSyncServer)
Add Java Doc comment
return constructActiveSyncDeviceList(powershellResponse);
}
private List<ActiveSyncDevice> getConnectedActiveSyncDevicesBefore(Date before, ActiveSyncServer activeSyncServer)
Add Java Doc comment
return constructActiveSyncDeviceList(powershellResponse);
}
private List<ActiveSyncDevice> getAllConnectedActiveSyncDevices(ActiveSyncServer activeSyncServer)
Add Java Doc comment
}
}
private IConfidentialClientApplication getOrCreateConfidentialClientApplication(String clientId, String secret, String authority)
Add Java Doc comment
public class DeviceMgtUtil {
private static final Log log = LogFactory.getLog(DeviceMgtUtil.class);
public static List<ActiveSyncDevice> getEnrolledActiveSyncDevicesSince(Date since)
You can add another parameter and have a single method to handle 'getEnrolledActiveSyncDevicesSince' and 'getEnrolledActiveSyncDevicesPriorTo' methods. With that approach we can reduce the repeating code lines.
return DeviceMgtUtil.constructActiveSyncDeviceList(devices);
}
private static DeviceManagementProviderService getDeviceManagementProviderService() {
It is not necessary to have a private method for this single line.
return realmService.getTenantUserRealm(tenantId).getUserStoreManager();
}
private static String getIdentity(String owner, UserStoreManager userStoreManager)
Is it required to have this method only for this line?
return userStoreManager.getUserClaimValue(owner, Constants.EMAIL_CLAIM_URI, null);
}
private static List<ActiveSyncDevice> constructActiveSyncDeviceList(List<Device> devices)
Add Java Doc comment.
return activeSyncDevices;
}
public static ActiveSyncDevice mapToActiveSyncDevice(Device device, UserStoreManager userStoreManager)
Add Java Doc comment.
return constructResponse(exitCode, getStringContent(standardOutputStringWriter),
getStringContent(errorStringWriter));
} catch (IOException e) {
String msg = "IOException occurred while executing powershell command : "
Log the msg and error.
+ powershellRequest.getCommand();
throw new PowershellExecutionException(msg, e);
} catch (InterruptedException e) {
String msg = "Thread got interrupted while executing powershell command : "
Log the msg and error.
) CEAPolicyWrapper ceaPolicyWrapper);
@GET
@Path("/syncNow")
Since this is API context path it is not needed to have camelcase definition. Instead of it, you can have 'sync-now'. Further we uses nouns in the context paths. Do the adjustment for the context path.
/*
* Copyright (c) 2018 - 2023, Entgra (Pvt) Ltd. (http://www.entgra.io) All Rights Reserved.
Change the Year
}
@GET
@Path("/syncNow")
Need to modify this place as well. Further do the required modification in the UI level as well.
}
}
private CEAPolicy constructCEAPolicy(CEAPolicyWrapper ceaPolicyWrapper) {
Add Java Doc comment.
return ceaPolicy;
}
private List<ActiveSyncServerUIConfiguration> constructActiveSyncServerConfigurations
Add Java Doc comment.
if (serverUIConfigurations == null) {
return activeSyncServerUIConfigurations;
}
for (ServerUIConfiguration serverUIConfiguration : serverUIConfigurations) {
Put following part into outside of the loop.
ActiveSyncServerUIConfiguration activeSyncServerUIConfiguration
validateCEAGracePeriod(ceaPolicyWrapper.getGracePeriodEntries());
}
public static void validateActiveSyncServer(ActiveSyncServer activeSyncServer) {
Add Java Doc comment
}
}
public static void validateCEAAccessPolicy(AccessPolicyWrapper accessPolicyWrapper) {
Add Java Doc comment
}
}
public static void validateCEAGracePeriod(GracePeriodWrapper gracePeriodWrapper) {
Add Java Doc comment
9b4c5e6adb
into master 10 months agoReviewers
9b4c5e6adb
.