Skip to content

Improve application access validation in client authentication #2942

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Improve application access validation in client authentication #2942

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
27 changes: 27 additions & 0 deletions ....carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/util/OAuth2Util.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -615,6 +615,9 @@ public static boolean authenticateClient(String clientId, String clientSecretPro
public static boolean authenticateClient(String clientId, String clientSecretProvided, String appTenant)
throws IdentityOAuthAdminException, IdentityOAuth2Exception, InvalidOAuthClientException {

Comment on lines 615 to 617
Copy link

Choose a reason for hiding this comment

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

Log Improvement Suggestion No: 1

Suggested change
public static boolean authenticateClient(String clientId, String clientSecretProvided, String appTenant)
throws IdentityOAuthAdminException, IdentityOAuth2Exception, InvalidOAuthClientException {
public static boolean authenticateClient(String clientId, String clientSecretProvided, String appTenant)
throws IdentityOAuthAdminException, IdentityOAuth2Exception, InvalidOAuthClientException {
log.info("Authenticating OAuth client: " + clientId);

if (!isApplicationAccessible(clientId, appTenant)) {
throw new InvalidOAuthClientException("Application is disabled for the client_id: " + clientId);
Comment on lines +618 to +619
Copy link

Choose a reason for hiding this comment

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

Log Improvement Suggestion No: 2

Suggested change
if (!isApplicationAccessible(clientId, appTenant)) {
throw new InvalidOAuthClientException("Application is disabled for the client_id: " + clientId);
if (!isApplicationAccessible(clientId, appTenant)) {
log.warn("Application is disabled for client_id: " + clientId);

}
OAuthAppDO appDO = OAuth2Util.getAppInformationByClientId(clientId, appTenant);
if (appDO == null) {
if (log.isDebugEnabled()) {
Expand Down Expand Up @@ -660,6 +663,30 @@ public static boolean authenticateClient(String clientId, String clientSecretPro
return true;
}

private static boolean isApplicationAccessible(String clientId, String appTenant)
throws IdentityOAuth2Exception {

Comment on lines +666 to +668
Copy link

Choose a reason for hiding this comment

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

Log Improvement Suggestion No: 3

Suggested change
private static boolean isApplicationAccessible(String clientId, String appTenant)
throws IdentityOAuth2Exception {
private static boolean isApplicationAccessible(String clientId, String appTenant)
throws IdentityOAuth2Exception {
if (log.isDebugEnabled()) {
log.debug("Checking application accessibility for client_id: " + clientId);
}

ServiceProvider serviceProvider = OAuth2Util.getServiceProvider(clientId, appTenant);
DiagnosticLog.DiagnosticLogBuilder diagnosticLogBuilder = null;
if (LoggerUtils.isDiagnosticLogsEnabled()) {
diagnosticLogBuilder = new DiagnosticLog.DiagnosticLogBuilder(
OAuthConstants.LogConstants.OAUTH_INBOUND_SERVICE,
OAuthConstants.LogConstants.ActionIDs.VALIDATE_APPLICATION_ENABLED_STATUS);
diagnosticLogBuilder.inputParam(LogConstants.InputKeys.CLIENT_ID, clientId)
.logDetailLevel(DiagnosticLog.LogDetailLevel.APPLICATION);
}
if (!serviceProvider.isApplicationEnabled()) {
if (diagnosticLogBuilder != null) {
Comment on lines +678 to +679
Copy link

Choose a reason for hiding this comment

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

Log Improvement Suggestion No: 4

Suggested change
if (!serviceProvider.isApplicationEnabled()) {
if (diagnosticLogBuilder != null) {
if (!serviceProvider.isApplicationEnabled()) {
log.warn("Application is disabled for client_id: " + clientId);

diagnosticLogBuilder
.resultMessage("Application is disabled.")
.resultStatus(DiagnosticLog.ResultStatus.FAILED);
LoggerUtils.triggerDiagnosticLogEvent(diagnosticLogBuilder);
}
return false;
}
Comment on lines +685 to +686
Copy link

Choose a reason for hiding this comment

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

Log Improvement Suggestion No: 5

Suggested change
return false;
}
return false;
}
if (log.isDebugEnabled()) {
log.debug("Application is enabled for client_id: " + clientId);
}

return true;
}

private static boolean isTenantActive(String tenantDomain) throws IdentityOAuth2Exception {
try {
TenantManager tenantManager = OAuthComponentServiceHolder.getInstance()
Expand Down
39 changes: 32 additions & 7 deletions ...bon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth2/util/OAuth2UtilTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -497,6 +497,20 @@ public Object[][] authenticateClient() {
};
}

@DataProvider(name = "ApplicationStatusProvider")
public Object[][] applicationStatusProvider() {

ServiceProvider serviceProvider = new ServiceProvider();
serviceProvider.setApplicationEnabled(true);

ServiceProvider disabledServiceProvider = new ServiceProvider();
disabledServiceProvider.setApplicationEnabled(false);
return new Object[][]{
{serviceProvider, true},
{disabledServiceProvider, false}
};
}

@Test(dataProvider = "AuthenticateClient")
public void testAuthenticateClient(Object cacheResult, String clientSecretInDB, boolean expectedResult)
throws Exception {
Expand Down Expand Up @@ -560,8 +574,9 @@ public void testAuthenticateClientWithHashPersistenceProcessor(Object cacheResul
}
}

@Test
public void testAuthenticateClientWithAppTenant() throws Exception {
@Test(dataProvider = "ApplicationStatusProvider")
public void testAuthenticateClientWithAppTenant(ServiceProvider serviceProvider, boolean isAppEnabled)
throws Exception {

try (MockedStatic<AppInfoCache> appInfoCache = mockStatic(AppInfoCache.class)) {
OAuthAppDO appDO = new OAuthAppDO();
Expand All @@ -581,16 +596,26 @@ public void testAuthenticateClientWithAppTenant() throws Exception {
(mock, context) -> {
when(mock.getAppInformation(eq(clientId), anyInt())).thenReturn(appDO);
})) {
ApplicationManagementService applicationManagementService = mock(ApplicationManagementService.class);
OAuth2ServiceComponentHolder.setApplicationMgtService(applicationManagementService);
when(applicationManagementService.getServiceProviderByClientId(anyString(), anyString(), anyString()))
.thenReturn(serviceProvider);
identityTenantUtil.when(() -> IdentityTenantUtil.getTenantId(clientTenantDomain))
.thenReturn(clientTenantId);

// Mock realm and tenant manager.
when(oAuthComponentServiceHolderMock.getRealmService()).thenReturn(realmServiceMock);
when(realmServiceMock.getTenantManager()).thenReturn(tenantManagerMock);
when(tenantManagerMock.getTenantId(clientTenantDomain)).thenReturn(clientTenantId);
when(tenantManagerMock.isTenantActive(clientTenantId)).thenReturn(true);
lenient().when(oAuthComponentServiceHolderMock.getRealmService()).thenReturn(realmServiceMock);
lenient().when(realmServiceMock.getTenantManager()).thenReturn(tenantManagerMock);
lenient().when(tenantManagerMock.getTenantId(clientTenantDomain)).thenReturn(clientTenantId);
lenient().when(tenantManagerMock.isTenantActive(clientTenantId)).thenReturn(true);

assertTrue(OAuth2Util.authenticateClient(clientId, clientSecret, clientTenantDomain));
if (isAppEnabled) {
assertTrue(OAuth2Util.authenticateClient(clientId, clientSecret, clientTenantDomain));
} else {
assertThrows(InvalidOAuthClientException.class, () -> {
OAuth2Util.authenticateClient(clientId, clientSecret, clientTenantDomain);
});
}
}
}
}
Expand Down