Feature disable password login #378

Merged
LukeGus merged 6 commits from feature-disable-password-login into dev-1.8.0 2025-10-08 00:59:44 +00:00
5 changed files with 124 additions and 0 deletions

View File

@@ -848,6 +848,23 @@ router.post("/login", async (req, res) => {
return res.status(400).json({ error: "Invalid username or password" });
gemini-code-assist[bot] commented 2025-10-08 00:53:37 +00:00 (Migrated from github.com)
Review

high

The current implementation has a "fail-open" vulnerability. If the database query to check allow_password_login fails, the catch block only logs a warning and allows the login process to continue. This could unintentionally permit password logins even when they are supposed to be disabled. The login attempt should be aborted if this check fails.

  } catch (e) {
    authLogger.error("Failed to check password login status", {
      operation: "login_check",
      error: e,
    });
    return res.status(500).json({ error: "Failed to check login status" });
  }
![high](https://www.gstatic.com/codereviewagent/high-priority.svg) The current implementation has a "fail-open" vulnerability. If the database query to check `allow_password_login` fails, the `catch` block only logs a warning and allows the login process to continue. This could unintentionally permit password logins even when they are supposed to be disabled. The login attempt should be aborted if this check fails. ```suggestion } catch (e) { authLogger.error("Failed to check password login status", { operation: "login_check", error: e, }); return res.status(500).json({ error: "Failed to check login status" }); } ```
gemini-code-assist[bot] commented 2025-10-08 00:53:38 +00:00 (Migrated from github.com)
Review

medium

Using as any bypasses TypeScript's type checking and can lead to runtime errors if the data structure changes. Since you are querying the settings table, you can expect row to be an object with a value property of type string. Casting to a more specific type improves type safety.

    if (row && (row as { value: string }).value !== "true") {
![medium](https://www.gstatic.com/codereviewagent/medium-priority.svg) Using `as any` bypasses TypeScript's type checking and can lead to runtime errors if the data structure changes. Since you are querying the `settings` table, you can expect `row` to be an object with a `value` property of type `string`. Casting to a more specific type improves type safety. ```suggestion if (row && (row as { value: string }).value !== "true") { ```
}
try {
const row = db.$client
.prepare("SELECT value FROM settings WHERE key = 'allow_password_login'")
.get();
if (row && (row as { value: string }).value !== "true") {
return res
.status(403)
.json({ error: "Password authentication is currently disabled" });
}
} catch (e) {
authLogger.error("Failed to check password login status", {
operation: "login_check",
error: e,
});
return res.status(500).json({ error: "Failed to check login status" });
}
try {
const user = await db
.select()
@@ -1098,6 +1115,43 @@ router.patch("/registration-allowed", authenticateJWT, async (req, res) => {
}
});
// Route: Get password login allowed status (public - needed for login page)
// GET /users/password-login-allowed
router.get("/password-login-allowed", async (req, res) => {
try {
const row = db.$client
.prepare("SELECT value FROM settings WHERE key = 'allow_password_login'")
.get();
res.json({ allowed: row ? (row as { value: string }).value === "true" : true });
} catch (err) {
authLogger.error("Failed to get password login allowed", err);
res.status(500).json({ error: "Failed to get password login allowed" });
}
});
// Route: Set password login allowed status (admin only)
// PATCH /users/password-login-allowed
router.patch("/password-login-allowed", authenticateJWT, async (req, res) => {
const userId = (req as any).userId;
try {
const user = await db.select().from(users).where(eq(users.id, userId));
if (!user || user.length === 0 || !user[0].is_admin) {
return res.status(403).json({ error: "Not authorized" });
}
const { allowed } = req.body;
if (typeof allowed !== "boolean") {
return res.status(400).json({ error: "Invalid value for allowed" });
}
db.$client
.prepare("UPDATE settings SET value = ? WHERE key = 'allow_password_login'")
.run(allowed ? "true" : "false");
res.json({ allowed });
} catch (err) {
authLogger.error("Failed to set password login allowed", err);
res.status(500).json({ error: "Failed to set password login allowed" });
}
});
// Route: Delete user account
// DELETE /users/delete-account
router.delete("/delete-account", authenticateJWT, async (req, res) => {

View File

@@ -409,10 +409,12 @@
"general": "General",
"userRegistration": "User Registration",
"allowNewAccountRegistration": "Allow new account registration",
"allowPasswordLogin": "Allow username/password login",
"missingRequiredFields": "Missing required fields: {{fields}}",
"oidcConfigurationUpdated": "OIDC configuration updated successfully!",
"failedToFetchOidcConfig": "Failed to fetch OIDC configuration",
"failedToFetchRegistrationStatus": "Failed to fetch registration status",
"failedToFetchPasswordLoginStatus": "Failed to fetch password login status",
"failedToFetchUsers": "Failed to fetch users",
"oidcConfigurationDisabled": "OIDC configuration disabled successfully!",
"failedToUpdateOidcConfig": "Failed to update OIDC configuration",

View File

@@ -395,10 +395,12 @@
"general": "常规",
"userRegistration": "用户注册",
"allowNewAccountRegistration": "允许新账户注册",
"allowPasswordLogin": "允许用户名/密码登录",
"missingRequiredFields": "缺少必填字段:{{fields}}",
"oidcConfigurationUpdated": "OIDC 配置更新成功!",
"failedToFetchOidcConfig": "获取 OIDC 配置失败",
"failedToFetchRegistrationStatus": "获取注册状态失败",
"failedToFetchPasswordLoginStatus": "获取密码登录状态失败",
"failedToFetchUsers": "获取用户列表失败",
"oidcConfigurationDisabled": "OIDC 配置禁用成功!",
"failedToUpdateOidcConfig": "更新 OIDC 配置失败",

View File

@@ -37,8 +37,10 @@ import { useConfirmation } from "@/hooks/use-confirmation.ts";
import {
getOIDCConfig,
getRegistrationAllowed,
getPasswordLoginAllowed,
getUserList,
updateRegistrationAllowed,
updatePasswordLoginAllowed,
updateOIDCConfig,
disableOIDCConfig,
makeUserAdmin,
@@ -62,6 +64,9 @@ export function AdminSettings({
const [allowRegistration, setAllowRegistration] = React.useState(true);
const [regLoading, setRegLoading] = React.useState(false);
const [allowPasswordLogin, setAllowPasswordLogin] = React.useState(true);
const [passwordLoginLoading, setPasswordLoginLoading] = React.useState(false);
const [oidcConfig, setOidcConfig] = React.useState({
client_id: "",
client_secret: "",
@@ -141,6 +146,27 @@ export function AdminSettings({
});
}, []);
React.useEffect(() => {
if (isElectron()) {
const serverUrl = (window as any).configuredServerUrl;
if (!serverUrl) {
return;
}
}
getPasswordLoginAllowed()
.then((res) => {
if (typeof res?.allowed === "boolean") {
setAllowPasswordLogin(res.allowed);
}
})
.catch((err) => {
if (err.code !== "NO_SERVER_CONFIGURED") {
toast.error(t("admin.failedToFetchPasswordLoginStatus"));
}
});
}, []);
const fetchUsers = async () => {
if (isElectron()) {
const serverUrl = (window as any).configuredServerUrl;
@@ -172,6 +198,16 @@ export function AdminSettings({
}
};
const handleTogglePasswordLogin = async (checked: boolean) => {
setPasswordLoginLoading(true);
try {
await updatePasswordLoginAllowed(checked);
setAllowPasswordLogin(checked);
} finally {
setPasswordLoginLoading(false);
}
};
const handleOIDCConfigSubmit = async (e: React.FormEvent) => {
e.preventDefault();
setOidcLoading(true);
@@ -483,6 +519,14 @@ export function AdminSettings({
/>
{t("admin.allowNewAccountRegistration")}
</label>
<label className="flex items-center gap-2">
<Checkbox
checked={allowPasswordLogin}
onCheckedChange={handleTogglePasswordLogin}
disabled={passwordLoginLoading}
/>
{t("admin.allowPasswordLogin")}
</label>
</div>
</TabsContent>

View File

@@ -1605,6 +1605,15 @@ export async function getRegistrationAllowed(): Promise<{ allowed: boolean }> {
}
}
export async function getPasswordLoginAllowed(): Promise<{ allowed: boolean }> {
try {
const response = await authApi.get("/users/password-login-allowed");
return response.data;
} catch (error) {
handleApiError(error, "check password login status");
}
}
export async function getOIDCConfig(): Promise<any> {
try {
const response = await authApi.get("/users/oidc-config");
@@ -1752,6 +1761,19 @@ export async function updateRegistrationAllowed(
}
gemini-code-assist[bot] commented 2025-10-08 00:53:38 +00:00 (Migrated from github.com)
Review

medium

The function updatePasswordLoginAllowed is typed to return Promise<any>. Based on the backend implementation, it returns an object { allowed: boolean }. Specifying the correct return type improves type safety and makes the code easier to understand and maintain.

export async function updatePasswordLoginAllowed(
  allowed: boolean,
): Promise<{ allowed: boolean }> {
![medium](https://www.gstatic.com/codereviewagent/medium-priority.svg) The function `updatePasswordLoginAllowed` is typed to return `Promise<any>`. Based on the backend implementation, it returns an object `{ allowed: boolean }`. Specifying the correct return type improves type safety and makes the code easier to understand and maintain. ```suggestion export async function updatePasswordLoginAllowed( allowed: boolean, ): Promise<{ allowed: boolean }> { ```
}
export async function updatePasswordLoginAllowed(
allowed: boolean,
): Promise<{ allowed: boolean }> {
try {
const response = await authApi.patch("/users/password-login-allowed", {
allowed,
});
return response.data;
} catch (error) {
handleApiError(error, "update password login allowed");
}
}
export async function updateOIDCConfig(config: any): Promise<any> {
try {
const response = await authApi.post("/users/oidc-config", config);