Skip to content

Commit

Permalink
CB-5833 Fix e-mails recognition for SSO (#3019)
Browse files Browse the repository at this point in the history
  • Loading branch information
sergeyteleshev authored Nov 12, 2024
1 parent a779a0e commit 110546a
Show file tree
Hide file tree
Showing 15 changed files with 158 additions and 35 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ public class WebAuthProviderDescriptor extends AbstractDescriptor {
private final boolean trusted;
private final boolean isPrivate;
private final boolean isAuthHidden;
private final boolean isCaseInsensitive;
private final String[] requiredFeatures;
private final boolean isRequired;
private final String[] types;
Expand All @@ -69,6 +70,7 @@ public WebAuthProviderDescriptor(IConfigurationElement cfg) {
this.isPrivate = CommonUtils.toBoolean(cfg.getAttribute("private"));
this.isRequired = CommonUtils.toBoolean(cfg.getAttribute("required"));
this.isAuthHidden = CommonUtils.toBoolean(cfg.getAttribute("authHidden"));
this.isCaseInsensitive = CommonUtils.toBoolean(cfg.getAttribute("caseInsensitive"));

for (IConfigurationElement cfgElement : cfg.getChildren("configuration")) {
List<WebAuthProviderProperty> properties = WebAuthProviderRegistry.readProperties(cfgElement);
Expand Down Expand Up @@ -132,6 +134,10 @@ public boolean isAuthHidden() {
return isAuthHidden;
}

public boolean isCaseInsensitive() {
return isCaseInsensitive;
}

public List<WebAuthProviderProperty> getConfigurationParameters() {
return new ArrayList<>(configurationParameters.values());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -161,12 +161,13 @@ public AdminUserInfo createUser(
if (userName.isEmpty()) {
throw new DBWebException("Empty user name");
}
webSession.addInfoMessage("Create new user - " + userName);
String userId = userName.toLowerCase();
webSession.addInfoMessage("Create new user - " + userId);

try {
var securityController = webSession.getAdminSecurityController();
securityController.createUser(userName, Map.of(), enabled, authRole);
var smUser = securityController.getUserById(userName);
securityController.createUser(userId, Map.of(), enabled, authRole);
var smUser = securityController.getUserById(userId);
return new AdminUserInfo(webSession, new WebUser(smUser));
} catch (Exception e) {
throw new DBWebException("Error creating new user", e);
Expand Down
1 change: 1 addition & 0 deletions server/bundles/io.cloudbeaver.service.security/plugin.xml
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
<plugin>
<extension point="org.jkiss.dbeaver.auth.provider">
<authProvider id="local" label="Local"
caseInsensitive="true"
description="Local name/password based authentication"
class="io.cloudbeaver.auth.provider.local.LocalAuthProvider"
icon="platform:/plugin/org.jkiss.dbeaver.model/icons/tree/key.png">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,9 @@ public String validateLocalAuth(@NotNull DBRProgressMonitor monitor,
throw new DBException("No user password provided");
}
String clientPasswordHash = AuthPropertyEncryption.hash.encrypt(userName, clientPassword);
if (!storedPasswordHash.equals(clientPasswordHash)) {
// we also need to check a hash with lower case (CB-5833)
String clientPasswordHashLowerCase = AuthPropertyEncryption.hash.encrypt(userName.toLowerCase(), clientPassword);
if (!storedPasswordHash.equals(clientPasswordHash) && !clientPasswordHashLowerCase.equals(storedPasswordHash)) {
throw new DBException("Invalid user name or password");
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,9 @@ private boolean isSubjectExists(String subjectId) throws DBCException {
///////////////////////////////////////////
// Users

/**
* Creates user. Saves user id in database in lower-case.
*/
@Override
public void createUser(
@NotNull String userId,
Expand All @@ -126,6 +129,7 @@ public void createUser(
if (CommonUtils.isEmpty(userId)) {
throw new DBCException("Empty user name is not allowed");
}
userId = userId.toLowerCase(); // creating new users only with lowercase
if (isSubjectExists(userId)) {
throw new DBCException("User or team '" + userId + "' already exists");
}
Expand All @@ -140,6 +144,9 @@ public void createUser(
}
}

/**
* Creates user. Saves user id in database as it is.
*/
public void createUser(
@NotNull Connection dbCon,
@NotNull String userId,
Expand Down Expand Up @@ -910,20 +917,38 @@ private String findUserByCredentials(
@NotNull WebAuthProviderDescriptor authProvider,
@NotNull Map<String, Object> authParameters,
boolean onlyActive // throws exception if user is inactive
) throws DBException {
String userId = findUserByCredentials(authProvider, authParameters, onlyActive, false);
if (userId == null && authProvider.isCaseInsensitive()) {
// try to find user id with lower case is auth provider is case-insensitive
return findUserByCredentials(authProvider, authParameters, onlyActive, true);
}
return userId;
}

@Nullable
private String findUserByCredentials(
@NotNull WebAuthProviderDescriptor authProvider,
@NotNull Map<String, Object> authParameters,
boolean onlyActive,
boolean isCaseInsensitive
) throws DBCException {
Map<String, Object> identCredentials = new LinkedHashMap<>();
Map<String, String> identCredentials = new LinkedHashMap<>();
String[] propNames = authParameters.keySet().toArray(new String[0]);
for (AuthPropertyDescriptor prop : authProvider.getCredentialParameters(propNames)) {
if (prop.isIdentifying()) {
String propId = CommonUtils.toString(prop.getId());
Object paramValue = authParameters.get(propId);
if (paramValue == null) {
if (authParameters.get(propId) == null) {
throw new DBCException("Authentication parameter '" + prop.getId() + "' is missing");
}
if (prop.getEncryption() == AuthPropertyEncryption.hash) {
throw new DBCException("Hash encryption can't be used in identifying credentials");
}
identCredentials.put(propId, paramValue);
String paramValue = CommonUtils.toString(authParameters.get(propId));
identCredentials.put(
propId,
isCaseInsensitive ? paramValue.toLowerCase() : paramValue
);
}
}
if (identCredentials.isEmpty()) {
Expand All @@ -947,9 +972,9 @@ private String findUserByCredentials(
try (PreparedStatement dbStat = dbCon.prepareStatement(database.normalizeTableNames(sql.toString()))) {
dbStat.setString(1, authProvider.getId());
int param = 2;
for (Map.Entry<String, Object> credEntry : identCredentials.entrySet()) {
for (Map.Entry<String, String> credEntry : identCredentials.entrySet()) {
dbStat.setString(param++, credEntry.getKey());
dbStat.setString(param++, CommonUtils.toString(credEntry.getValue()));
dbStat.setString(param++, credEntry.getValue());
}

try (ResultSet dbResult = dbStat.executeQuery()) {
Expand Down Expand Up @@ -980,6 +1005,15 @@ private String findUserByCredentials(
@Override
public Map<String, Object> getUserCredentials(String userId, String authProviderId) throws DBCException {
WebAuthProviderDescriptor authProvider = getAuthProvider(authProviderId);
Map<String, Object> creds = getUserCredentials(authProvider, userId);
if (creds.isEmpty() && authProvider.isCaseInsensitive()) {
return getUserCredentials(authProvider, userId.toLowerCase());
}
return creds;
}

@NotNull
private Map<String, Object> getUserCredentials(WebAuthProviderDescriptor authProvider, String userId) throws DBCException {
try (Connection dbCon = database.openConnection()) {
try (PreparedStatement dbStat = dbCon.prepareStatement(
database.normalizeTableNames("SELECT CRED_ID,CRED_VALUE FROM {table_prefix}CB_USER_CREDENTIALS\n" +
Expand All @@ -990,7 +1024,6 @@ public Map<String, Object> getUserCredentials(String userId, String authProvider

try (ResultSet dbResult = dbStat.executeQuery()) {
Map<String, Object> credentials = new LinkedHashMap<>();

while (dbResult.next()) {
credentials.put(dbResult.getString(1), dbResult.getString(2));
}
Expand Down Expand Up @@ -1182,6 +1215,7 @@ public void createTeam(String teamId, String name, String description, String gr
if (CommonUtils.isEmpty(teamId)) {
throw new DBCException("Empty team name is not allowed");
}
teamId = teamId.toLowerCase();
if (isSubjectExists(teamId)) {
throw new DBCException("User or team '" + teamId + "' already exists");
}
Expand Down Expand Up @@ -2427,13 +2461,20 @@ private String findOrCreateExternalUserByCredentials(
return null;
}

userId = userIdFromCredentials;
userId = authProvider.isCaseInsensitive() ? userIdFromCredentials.toLowerCase() : userIdFromCredentials;
if (!isSubjectExists(userId)) {
createUser(userId,
Map.of(),
true,
resolveUserAuthRole(null, authRole)
);
log.debug("Create user: " + userId);
try (Connection dbCon = database.openConnection()) {
createUser(
dbCon,
userId,
Map.of(),
true,
resolveUserAuthRole(null, authRole)
);
} catch (SQLException e) {
throw new DBException("Error saving user in database", e);
}
}
setUserCredentials(userId, authProvider.getId(), userCredentials);
} else if (userId == null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,19 @@
*/
package io.cloudbeaver.test.platform;

import io.cloudbeaver.auth.provider.local.LocalAuthProvider;
import io.cloudbeaver.auth.provider.rp.RPAuthProvider;
import io.cloudbeaver.test.WebGQLClient;
import org.jkiss.code.NotNull;
import org.jkiss.dbeaver.model.auth.SMAuthStatus;
import org.jkiss.dbeaver.model.data.json.JSONUtils;
import org.jkiss.utils.SecurityUtils;
import org.junit.Assert;
import org.junit.Test;

import java.util.List;
import java.util.Map;
import java.util.Set;

public class AuthenticationTest {
private static final String GQL_OPEN_SESSION = """
Expand All @@ -39,6 +43,12 @@ mutation openSession($defaultLocale: String) {
userId
}
}""";
private static final String GQL_AUTH_LOGOUT = """
query authLogoutExtended($provider: ID, $configuration: ID) {
result: authLogoutExtended(provider: $provider, configuration: $configuration) {
redirectLinks
}
}""";

@Test
public void testLoginUser() throws Exception {
Expand All @@ -47,6 +57,34 @@ public void testLoginUser() throws Exception {
Assert.assertEquals(SMAuthStatus.SUCCESS.name(), JSONUtils.getString(authInfo, "authStatus"));
}


@Test
public void testLoginUserWithCamelCase() throws Exception {
WebGQLClient client = CEServerTestSuite.createClient();
for (String userId : Set.of("Test", "tESt", "tesT", "TEST")) {
Map<String, Object> credsWithCamelCase = getUserCredentials(userId);
// authenticating with user
Map<String, Object> authInfo = CEServerTestSuite.authenticateTestUser(client, credsWithCamelCase);
Assert.assertEquals(SMAuthStatus.SUCCESS.name(), JSONUtils.getString(authInfo, "authStatus"));
Map<String, Object> activeUser = client.sendQuery(GQL_ACTIVE_USER, null);
Assert.assertEquals(userId.toLowerCase(), JSONUtils.getString(activeUser, "userId"));
// making logout
client.sendQuery(GQL_AUTH_LOGOUT, Map.of("provider", "local"));

activeUser = client.sendQuery(GQL_ACTIVE_USER, null);
Assert.assertNotEquals(userId.toLowerCase(), JSONUtils.getString(activeUser, "userId"));
}
}

@NotNull
private Map<String, Object> getUserCredentials(@NotNull String userId) throws Exception {
return Map.of(
LocalAuthProvider.CRED_USER, userId,
LocalAuthProvider.CRED_PASSWORD, SecurityUtils.makeDigest("test")
);
}


@Test
public void testReverseProxyAnonymousModeLogin() throws Exception {
WebGQLClient client = CEServerTestSuite.createClient();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,11 +101,18 @@ public static WebGQLClient createClient(@NotNull HttpClient httpClient) {
}

public static Map<String, Object> authenticateTestUser(@NotNull WebGQLClient client) throws Exception {
return authenticateTestUser(client, TEST_CREDENTIALS);
}

public static Map<String, Object> authenticateTestUser(
@NotNull WebGQLClient client,
@NotNull Map<String, Object> credentials
) throws Exception {
return client.sendQuery(
WebGQLClient.GQL_AUTHENTICATE,
Map.of(
"provider", LocalAuthProvider.PROVIDER_ID,
"credentials", TEST_CREDENTIALS
"credentials", credentials
)
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,16 @@ export const UserFormInfoCredentials = observer<Props>(function UserFormInfoCred
return (
<Container gap vertical>
<GroupTitle keepSize>{translate('authentication_user_credentials')}</GroupTitle>
<InputField type="text" name="userId" state={tabState.state} readOnly={editing || disabled} keepSize tiny required>
<InputField
description={!editing ? translate('authentication_user_name_description') : undefined}
type="text"
name="userId"
state={tabState.state}
readOnly={editing || disabled}
keepSize
tiny
required
>
{translate('authentication_user_name')}
</InputField>
{local && (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,8 @@ export class UserFormInfoPart extends FormPart<IUserFormInfoState, IUserFormStat
authRole: getTransformedAuthRole(this.state.authRole),
enabled: this.state.enabled,
});
// userId is modified by backend and may not match value we sent, so we need to update the state
this.state.userId = user.userId;
this.initialState.userId = user.userId;
this.formState.setMode(FormMode.Edit);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,10 @@ interface State {
export function useUsersTable(filters: IUserFilters) {
const translate = useTranslate();
const usersResource = useService(UsersResource);
const searchFilter = filters.search.trim().toLowerCase();
const enabledStateFilter = filters.status === 'true' ? true : filters.status === 'false' ? false : undefined;
const pagination = useOffsetPagination(UsersResource, {
key: UsersResourceFilterKey(filters.search.toLowerCase(), filters.status === 'true' ? true : filters.status === 'false' ? false : undefined),
key: UsersResourceFilterKey(searchFilter, enabledStateFilter),
});
const usersLoader = useResource(useUsersTable, usersResource, pagination.currentPage);
const notificationService = useService(NotificationService);
Expand All @@ -48,7 +50,7 @@ export function useUsersTable(filters: IUserFilters) {
get users() {
const users = Array.from(
new Set([
...this.usersLoader.resource.get(UsersResourceNewUsers),
...this.usersLoader.resource.get(UsersResourceFilterKey(searchFilter, enabledStateFilter)),
...usersResource.get(pagination.allPages).filter(isDefined).sort(compareUsers),
]),
);
Expand Down
1 change: 1 addition & 0 deletions webapp/packages/plugin-authentication/src/locales/en.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ export default [
['authentication_identity_provider_dialog_subtitle', 'Choose configuration you want to sign in with'],

['authentication_user_name', 'Username'],
['authentication_user_name_description', "User's identifier is stored in lowercase"],
['authentication_user_role', 'Role'],
['authentication_user_credentials', 'Credentials'],
['authentication_user_meta_parameters', 'Parameters'],
Expand Down
Loading

0 comments on commit 110546a

Please sign in to comment.