mirror of
https://github.com/iflytek/skillhub.git
synced 2026-10-11 03:37:57 +00:00
fix(auth): preserve login on existing installations
Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
This commit is contained in:
XiaoSeS
parent
f297fdfd09
commit
ad2412c075
9 changed files with 117 additions and 15 deletions
|
|
@ -6,7 +6,7 @@
|
|||
|
||||
## 空库初始化
|
||||
|
||||
仅在数据库没有对应记录时，启动参数提供初始值：
|
||||
全新安装且尚无用户时，启动参数提供初始值。已有用户的实例升级时，若尚无 `auth.local` 记录，首次启动会把两个开关都设为开启，以保持原有登录行为：
|
||||
|
||||
| 环境变量 | Helm 值 | 默认值 |
|
||||
| --- | --- | --- |
|
||||
|
|
@ -26,6 +26,6 @@
|
|||
|
||||
规则按身份来源代码和**已验证邮箱**匹配。只有外部身份通过现有准入策略、并且新账号首次创建为可用状态时，才给新账号授予选定平台角色；规则随后标为“已授权”，不会再次使用。拒绝或待审批的登录不会触发授权，规则不会绕过准入策略。已有账号请在“用户管理”中直接授权。相同邮箱的本地账号不会因此合并或获得角色。
|
||||
|
||||
当前飞书和钉钉适配器把邮箱标记为未验证：它们的用户资料没有提供可证明用户控制该邮箱的信号。因此以 `feishu` 或 `dingtalk` 配置的邮箱规则不会触发自动授权。GitHub 的已验证主邮箱、提供 `email_verified=true` 的 OIDC 身份等可匹配。飞书或钉钉账号首次进入后，可由已有超级管理员在用户管理中授权；如果部署方只允许飞书或钉钉登录，必须先安排其他可用的管理员入口。不能为了触发规则而把未验证邮箱改成已验证。
|
||||
当前飞书和钉钉适配器把邮箱标记为未验证：它们的用户资料没有提供可证明用户控制该邮箱的信号。因此后台和初始化均拒绝为 `feishu` 或 `dingtalk` 配置邮箱规则。GitHub 的已验证主邮箱、提供 `email_verified=true` 的 OIDC 身份等可匹配。飞书或钉钉账号首次进入后，可由已有超级管理员在用户管理中授权；如果部署方只允许飞书或钉钉登录，必须先安排其他可用的管理员入口。不能为了触发规则而把未验证邮箱改成已验证。
|
||||
|
||||
后台可以新增规则、调整待匹配规则的角色、停用规则，并查看已使用规则的匹配身份和授权账号。只允许超级管理员操作；修改带版本号，避免两个管理员同时改动时后写覆盖前写。紧急恢复可通过受控数据库操作处理，并保留审计记录。
|
||||
|
|
|
|||
|
|
@ -47,10 +47,16 @@
|
|||
|
||||
新安装首次初始化时 SHALL 使用部署参数或代码默认值填充缺失设置，并持久化初始化状态。已经持久化的设置 SHALL 不被后续部署参数或重启覆盖。外部服务秘密不得存入设置表。
|
||||
|
||||
已有用户的实例升级且首次缺失 `auth.local` 时 SHALL 初始化为两个开关均开启，保持原有认证行为；部署参数不应在该升级场景关闭本地认证。
|
||||
|
||||
#### Scenario: 后台关闭登录后重启
|
||||
- **WHEN** 后台已持久化 `passwordLoginEnabled=false`，部署参数仍写着开启
|
||||
- **THEN** 重启后数据库设置仍为关闭
|
||||
|
||||
#### Scenario: 已有用户的实例首次升级
|
||||
- **WHEN** 数据库已有用户，但首次升级时还没有 `auth.local`，且部署参数设为关闭本地密码登录
|
||||
- **THEN** 系统把两个开关初始化为开启，不因部署参数切断原有登录入口
|
||||
|
||||
#### Scenario: 新版本增加设置键
|
||||
- **WHEN** 升级版本新增注册过的设置键
|
||||
- **THEN** 系统只为新键补入默认值，不修改既有键
|
||||
|
|
|
|||
|
|
@ -67,15 +67,21 @@ public class InitialAuthSettingsInitializer implements ApplicationRunner {
|
|||
}
|
||||
return null;
|
||||
});
|
||||
Long existingUsers = null;
|
||||
if (settings.findBySettingKey(LocalAuthSettingsService.SETTING_KEY).isEmpty()) {
|
||||
existingUsers = jdbcTemplate.queryForObject("SELECT COUNT(*) FROM user_account", Long.class);
|
||||
boolean newInstallation = existingUsers == 0L;
|
||||
settings.save(new SystemSetting(LocalAuthSettingsService.SETTING_KEY, Map.of(
|
||||
LocalAuthSettingsService.PASSWORD_LOGIN_KEY, properties.isPasswordLoginEnabled(),
|
||||
LocalAuthSettingsService.SELF_REGISTRATION_KEY, properties.isSelfRegistrationEnabled())));
|
||||
LocalAuthSettingsService.PASSWORD_LOGIN_KEY,
|
||||
newInstallation ? properties.isPasswordLoginEnabled() : true,
|
||||
LocalAuthSettingsService.SELF_REGISTRATION_KEY,
|
||||
newInstallation ? properties.isSelfRegistrationEnabled() : true)));
|
||||
}
|
||||
if (settings.findBySettingKey(GRANT_SEED_MARKER).isPresent()) {
|
||||
return;
|
||||
}
|
||||
if (jdbcTemplate.queryForObject("SELECT COUNT(*) FROM user_account", Long.class) == 0L) {
|
||||
if ((existingUsers != null ? existingUsers
|
||||
: jdbcTemplate.queryForObject("SELECT COUNT(*) FROM user_account", Long.class)) == 0L) {
|
||||
seedInitialRules();
|
||||
}
|
||||
settings.save(new SystemSetting(GRANT_SEED_MARKER, Map.of("initialized", true)));
|
||||
|
|
|
|||
|
|
@ -48,6 +48,41 @@ class InitialAuthSettingsInitializerTest {
|
|||
verify(rules, never()).save(any());
|
||||
}
|
||||
|
||||
@Test
|
||||
void emptyDatabaseUsesConfiguredLoginFlags() {
|
||||
when(settings.findBySettingKey(LocalAuthSettingsService.SETTING_KEY)).thenReturn(Optional.empty());
|
||||
when(settings.findBySettingKey("auth.initial-role-grants.initialized")).thenReturn(Optional.empty());
|
||||
when(jdbc.queryForObject("SELECT COUNT(*) FROM user_account", Long.class)).thenReturn(0L);
|
||||
InitialAuthSettingsProperties properties = new InitialAuthSettingsProperties();
|
||||
properties.setPasswordLoginEnabled(false);
|
||||
properties.setSelfRegistrationEnabled(false);
|
||||
|
||||
new InitialAuthSettingsInitializer(properties, settings, rules, roles, new ObjectMapper(), jdbc).run(args);
|
||||
|
||||
verify(settings).save(org.mockito.ArgumentMatchers.argThat(setting ->
|
||||
setting.getSettingKey().equals(LocalAuthSettingsService.SETTING_KEY)
|
||||
&& setting.getValue().equals(Map.of("passwordLoginEnabled", false,
|
||||
"selfRegistrationEnabled", false))));
|
||||
}
|
||||
|
||||
@Test
|
||||
void existingUsersUpgradeWithSafeDefaultsEvenWhenDeploymentFlagsAreDisabled() {
|
||||
when(settings.findBySettingKey(LocalAuthSettingsService.SETTING_KEY)).thenReturn(Optional.empty());
|
||||
when(settings.findBySettingKey("auth.initial-role-grants.initialized")).thenReturn(Optional.empty());
|
||||
when(jdbc.queryForObject("SELECT COUNT(*) FROM user_account", Long.class)).thenReturn(2L);
|
||||
InitialAuthSettingsProperties properties = new InitialAuthSettingsProperties();
|
||||
properties.setPasswordLoginEnabled(false);
|
||||
properties.setSelfRegistrationEnabled(false);
|
||||
|
||||
new InitialAuthSettingsInitializer(properties, settings, rules, roles, new ObjectMapper(), jdbc).run(args);
|
||||
|
||||
verify(settings).save(org.mockito.ArgumentMatchers.argThat(setting ->
|
||||
setting.getSettingKey().equals(LocalAuthSettingsService.SETTING_KEY)
|
||||
&& setting.getValue().equals(Map.of("passwordLoginEnabled", true,
|
||||
"selfRegistrationEnabled", true))));
|
||||
verify(rules, never()).save(any());
|
||||
}
|
||||
|
||||
@Test
|
||||
void existingMarkerPreventsDeletedRulesFromBeingSeededAgain() {
|
||||
when(settings.findBySettingKey(LocalAuthSettingsService.SETTING_KEY))
|
||||
|
|
|
|||
|
|
@ -178,11 +178,11 @@ class IdentityBindingServiceTest {
|
|||
|
||||
@Test
|
||||
void bindOrCreate_grantIsVisibleInFirstPrincipalBeforeSessionCreation() {
|
||||
OAuthClaims claims = new OAuthClaims("feishu", "external-1", "admin@example.com", true, "admin", Map.of());
|
||||
OAuthClaims claims = new OAuthClaims("github", "external-1", "admin@example.com", true, "admin", Map.of());
|
||||
Role role = new Role();
|
||||
ReflectionTestUtils.setField(role, "code", "SUPER_ADMIN");
|
||||
AtomicBoolean granted = new AtomicBoolean();
|
||||
when(bindingRepo.findByProviderCodeAndSubject("feishu", "external-1")).thenReturn(Optional.empty());
|
||||
when(bindingRepo.findByProviderCodeAndSubject("github", "external-1")).thenReturn(Optional.empty());
|
||||
when(userRepo.save(any(UserAccount.class))).thenAnswer(invocation -> invocation.getArgument(0));
|
||||
doAnswer(invocation -> {
|
||||
granted.set(true);
|
||||
|
|
@ -201,9 +201,9 @@ class IdentityBindingServiceTest {
|
|||
|
||||
@Test
|
||||
void bindOrCreate_doesNotMergeWithLocalAccountSharingEmail() {
|
||||
OAuthClaims claims = new OAuthClaims("feishu", "external-1", "shared@example.com", true,
|
||||
OAuthClaims claims = new OAuthClaims("github", "external-1", "shared@example.com", true,
|
||||
"external-user", Map.of());
|
||||
when(bindingRepo.findByProviderCodeAndSubject("feishu", "external-1")).thenReturn(Optional.empty());
|
||||
when(bindingRepo.findByProviderCodeAndSubject("github", "external-1")).thenReturn(Optional.empty());
|
||||
when(userRepo.save(any(UserAccount.class))).thenAnswer(invocation -> invocation.getArgument(0));
|
||||
when(roleBindingRepo.findByUserId(any())).thenReturn(List.of());
|
||||
|
||||
|
|
|
|||
|
|
@ -102,6 +102,17 @@ class LocalAuthServiceTest {
|
|||
verifyNoInteractions(userAccountRepository, credentialRepository);
|
||||
}
|
||||
|
||||
@Test
|
||||
void disabledPasswordLoginStopsChangePasswordBeforeCredentialLookup() {
|
||||
doThrow(new AuthFlowException(HttpStatus.FORBIDDEN, "error.auth.local.login.disabled"))
|
||||
.when(authSettings).requirePasswordLogin();
|
||||
|
||||
assertThatThrownBy(() -> service.changePassword("usr_1", "old", "Newpass123!"))
|
||||
.isInstanceOf(AuthFlowException.class)
|
||||
.extracting("status").isEqualTo(HttpStatus.FORBIDDEN);
|
||||
verifyNoInteractions(credentialRepository, passwordEncoder);
|
||||
}
|
||||
|
||||
@Test
|
||||
void register_createsUserAndCredential() {
|
||||
given(credentialRepository.existsByUsernameIgnoreCase("alice")).willReturn(false);
|
||||
|
|
|
|||
|
|
@ -87,6 +87,28 @@ class PasswordResetServiceTest {
|
|||
verifyNoInteractions(userAccountRepository, resetRequestRepository, mailSender);
|
||||
}
|
||||
|
||||
@Test
|
||||
void disabledPasswordLoginStopsResetConfirmationBeforeCodeLookup() {
|
||||
doThrow(new AuthFlowException(HttpStatus.FORBIDDEN, "error.auth.local.login.disabled"))
|
||||
.when(authSettings).requirePasswordLogin();
|
||||
|
||||
assertThatThrownBy(() -> service.confirmPasswordReset("alice@example.com", "123456", "Abcd123!"))
|
||||
.isInstanceOf(AuthFlowException.class)
|
||||
.extracting("status").isEqualTo(HttpStatus.FORBIDDEN);
|
||||
verifyNoInteractions(userAccountRepository, resetRequestRepository, credentialRepository);
|
||||
}
|
||||
|
||||
@Test
|
||||
void disabledPasswordLoginStopsAdminResetBeforeAccountLookup() {
|
||||
doThrow(new AuthFlowException(HttpStatus.FORBIDDEN, "error.auth.local.login.disabled"))
|
||||
.when(authSettings).requirePasswordLogin();
|
||||
|
||||
assertThatThrownBy(() -> service.adminTriggerPasswordReset("usr_1", "admin_1"))
|
||||
.isInstanceOf(AuthFlowException.class)
|
||||
.extracting("status").isEqualTo(HttpStatus.FORBIDDEN);
|
||||
verifyNoInteractions(userAccountRepository, resetRequestRepository, credentialRepository);
|
||||
}
|
||||
|
||||
@Test
|
||||
void requestPasswordReset_withEligibleEmail_savesRequestAndSendsEmail() {
|
||||
UserAccount user = new UserAccount("usr_1", "alice", "alice@example.com", null);
|
||||
|
|
|
|||
|
|
@ -36,12 +36,12 @@ class InitialExternalRoleGrantServiceTest {
|
|||
|
||||
@Test
|
||||
void verifiedEmailConsumesMatchingRuleAndGrantsNewUser() {
|
||||
ExternalRoleGrantRule rule = new ExternalRoleGrantRule("feishu", "admin@example.com", role, null);
|
||||
when(rules.lockByIdentityAndStatus("feishu", "admin@example.com", ExternalRoleGrantRule.Status.ACTIVE))
|
||||
ExternalRoleGrantRule rule = new ExternalRoleGrantRule("github", "admin@example.com", role, null);
|
||||
when(rules.lockByIdentityAndStatus("github", "admin@example.com", ExternalRoleGrantRule.Status.ACTIVE))
|
||||
.thenReturn(Optional.of(rule));
|
||||
when(role.getCode()).thenReturn("SUPER_ADMIN");
|
||||
|
||||
service.grantForNewUser(new OAuthClaims("FEISHU", "external-42", " Admin@Example.COM ", true,
|
||||
service.grantForNewUser(new OAuthClaims("GITHUB", "external-42", " Admin@Example.COM ", true,
|
||||
"admin", Map.of()), "usr_new");
|
||||
|
||||
ArgumentCaptor<UserRoleBinding> grant = ArgumentCaptor.forClass(UserRoleBinding.class);
|
||||
|
|
@ -58,7 +58,7 @@ class InitialExternalRoleGrantServiceTest {
|
|||
|
||||
@Test
|
||||
void unverifiedEmailCannotConsumeRule() {
|
||||
service.grantForNewUser(new OAuthClaims("feishu", "external-42", "admin@example.com", false,
|
||||
service.grantForNewUser(new OAuthClaims("github", "external-42", "admin@example.com", false,
|
||||
"admin", Map.of()), "usr_new");
|
||||
|
||||
verify(rules, never()).lockByIdentityAndStatus(any(), any(), eq(ExternalRoleGrantRule.Status.ACTIVE));
|
||||
|
|
@ -68,10 +68,10 @@ class InitialExternalRoleGrantServiceTest {
|
|||
|
||||
@Test
|
||||
void absentActiveRuleDoesNotGrantOrAudit() {
|
||||
when(rules.lockByIdentityAndStatus("feishu", "admin@example.com", ExternalRoleGrantRule.Status.ACTIVE))
|
||||
when(rules.lockByIdentityAndStatus("github", "admin@example.com", ExternalRoleGrantRule.Status.ACTIVE))
|
||||
.thenReturn(Optional.empty());
|
||||
|
||||
service.grantForNewUser(new OAuthClaims("feishu", "external-42", "admin@example.com", true,
|
||||
service.grantForNewUser(new OAuthClaims("github", "external-42", "admin@example.com", true,
|
||||
"admin", Map.of()), "usr_new");
|
||||
|
||||
verify(bindings, never()).save(any());
|
||||
|
|
|
|||
|
|
@ -2,11 +2,13 @@ package com.iflytek.skillhub.auth.settings;
|
|||
|
||||
import static org.assertj.core.api.Assertions.assertThatThrownBy;
|
||||
import static org.assertj.core.api.Assertions.assertThatCode;
|
||||
import static org.assertj.core.api.Assertions.assertThat;
|
||||
import static org.mockito.Mockito.when;
|
||||
|
||||
import com.iflytek.skillhub.auth.exception.AuthFlowException;
|
||||
import java.util.Map;
|
||||
import java.util.Optional;
|
||||
import java.util.concurrent.atomic.AtomicReference;
|
||||
import org.junit.jupiter.api.Test;
|
||||
import org.junit.jupiter.api.extension.ExtendWith;
|
||||
import org.mockito.Mock;
|
||||
|
|
@ -19,6 +21,26 @@ import org.springframework.test.util.ReflectionTestUtils;
|
|||
class LocalAuthSettingsServiceTest {
|
||||
@Mock private SystemSettingRepository repository;
|
||||
|
||||
@Test
|
||||
void separateServiceInstancesObserveCurrentStoredValue() {
|
||||
SystemSetting initial = new SystemSetting("auth.local",
|
||||
Map.of("passwordLoginEnabled", true, "selfRegistrationEnabled", true));
|
||||
ReflectionTestUtils.setField(initial, "id", 1L);
|
||||
AtomicReference<SystemSetting> stored = new AtomicReference<>(initial);
|
||||
when(repository.findBySettingKey("auth.local")).thenAnswer(invocation -> Optional.of(stored.get()));
|
||||
LocalAuthSettingsService firstInstance = new LocalAuthSettingsService(repository);
|
||||
LocalAuthSettingsService secondInstance = new LocalAuthSettingsService(repository);
|
||||
|
||||
assertThat(firstInstance.current().passwordLoginEnabled()).isTrue();
|
||||
SystemSetting updated = new SystemSetting("auth.local",
|
||||
Map.of("passwordLoginEnabled", false, "selfRegistrationEnabled", true));
|
||||
ReflectionTestUtils.setField(updated, "id", 1L);
|
||||
stored.set(updated);
|
||||
|
||||
assertThat(secondInstance.current().passwordLoginEnabled()).isFalse();
|
||||
assertThat(firstInstance.current().passwordLoginEnabled()).isFalse();
|
||||
}
|
||||
|
||||
@Test
|
||||
void closedPasswordLoginBlocksPasswordAndRegistration() {
|
||||
SystemSetting setting = new SystemSetting("auth.local",
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue