fix: remove glibc-based password expiry check and fix domain user#69
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRemoves the custom glibc-based password expiry helper and relies solely on the model’s updatePasswordExpiredInfo(), while ensuring newly discovered domain users are added to the local model and fixing a crash when currentUser() is null during auth completion. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
createAuthentication, instead of just logging that password expiry info will not be synced when the user is missing from the model, consider explicitly documenting or handling how/when that expiry info will be synchronized later to avoid silent divergence from backend state. - In
onAuthFinished, you can simplify and slightly optimize the repeatedm_model->currentUser()lookup by storing it in a local variable before the null checks and reuse that pointer for both updating expiry info and checkingexpiredState().
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `createAuthentication`, instead of just logging that password expiry info will not be synced when the user is missing from the model, consider explicitly documenting or handling how/when that expiry info will be synchronized later to avoid silent divergence from backend state.
- In `onAuthFinished`, you can simplify and slightly optimize the repeated `m_model->currentUser()` lookup by storing it in a local variable before the null checks and reuse that pointer for both updating expiry info and checking `expiredState()`.
## Individual Comments
### Comment 1
<location path="src/lightdm-deepin-greeter/greeterworker.cpp" line_range="890-893" />
<code_context>
m_greeter->respond(m_authFramework->AuthSessionPath(m_account) + QString(";") + m_password);
- updatePasswordExpiredStateBySPName(m_account);
- if (m_model->currentUser()->expiredState() == User::ExpiredAlready) {
+ if (m_model->currentUser()) {
+ m_model->currentUser()->updatePasswordExpiredInfo();
+ }
+ if (m_model->currentUser() && m_model->currentUser()->expiredState() == User::ExpiredAlready) {
changePasswd();
}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider avoiding repeated `currentUser()` lookups and ensure it refers to the authenticating account.
In `onAuthFinished`, `m_model->currentUser()` is invoked three times and implicitly assumed to match `m_account`. Please capture it once (e.g. `auto user = m_model->currentUser();`) and use that pointer. This reduces redundant lookups, minimizes race-condition risk, and makes it explicit that you’re operating on the user associated with the completed auth session.
Suggested implementation:
```cpp
m_greeter->respond(m_authFramework->AuthSessionPath(m_account) + QString(";") + m_password);
auto user = m_model->currentUser();
if (user) {
user->updatePasswordExpiredInfo();
if (user->expiredState() == User::ExpiredAlready) {
changePasswd();
}
}
```
If the model provides an API to fetch a user explicitly by account (e.g. `findUserByName(m_account)` or similar), consider replacing `m_model->currentUser()` with that call when initializing `user` to make the association with the authenticating account explicit.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
deepin pr auto review★ 总体评分:100分■ 【总体评价】
■ 【详细分析】
■ 【改进建议代码示例】 // 当前代码已经非常完善,此处仅展示保留的防御性检查最佳实践
void GreeterWorker::onAuthFinished()
{
qCInfo(DDE_SHELL) << "Auth finished";
if (m_greeter->inAuthentication()) {
m_greeter->respond(m_authFramework->AuthSessionPath(m_account) + QString(";") + m_password);
// 确保用户对象存在后再进行状态更新和判断
if (m_model->currentUser()) {
m_model->currentUser()->updatePasswordExpiredInfo();
if (m_model->currentUser()->expiredState() == User::ExpiredAlready) {
changePasswd();
}
}
} else {
// 其他逻辑
}
} |
Synchronize source files from linuxdeepin/dde-session-shell. Source-pull-request: linuxdeepin/dde-session-shell#69
|
|
||
| // 新发现的用户(如域管用户首次登录),加入本地 model, | ||
| // 确保后续 createAuthentication 中 findUserByName 能找到 | ||
| if (!originalUsePtr) { |
There was a problem hiding this comment.
合并后,在snipe仓库提一笔大于dde-daemon修改的版本依赖。
Synchronize source files from linuxdeepin/dde-session-shell. Source-pull-request: linuxdeepin/dde-session-shell#69
handling 1. Remove `updatePasswordExpiredStateBySPName()` function that used glibc `getspnam` to check password expiry, as it was only needed for first-time domain user login 2. Ensure new domain users discovered in `checkAccount()` are added to the local model immediately, so subsequent operations like `createAuthentication()` can find them via `findUserByName()` 3. Fall back to using `updatePasswordExpiredInfo()` through existing model interfaces for all users, including newly discovered ones 4. Fix a crash in `onAuthFinished()` where `m_model->currentUser()` could be null when checking `expiredState()` Log: Fix domain user first login; remove redundant glibc password expiry logic Influence: 1. Test domain user first login scenario to verify password expiry info is correctly synced 2. Test normal user login to ensure no regression with password expiry checks 3. Verify no crashes occur during authentication finish for any user type 4. Test multi-user switching, including between local and domain users fix: 移除基于 glibc 的密码过期检查并修正域管用户处理 1. 移除 `updatePasswordExpiredStateBySPName()` 函数,该函数使用 glibc 的 `getspnam` 检查密码过期,只在域管用户首次登录时需要 2. 确保在 `checkAccount()` 中新发现的域管用户立即加入本地 model,使得后 续 `createAuthentication()` 等操作能通过 `findUserByName()` 找到 3. 所有用户(包括新发现用户)都回退到通过现有 model 接口使用 `updatePasswordExpiredInfo()` 4. 修复 `onAuthFinished()` 中 `m_model->currentUser()` 可能为空时检查 `expiredState()` 导致的崩溃 Log: 修复域管用户首次登录;移除冗余的 glibc 密码过期逻辑 Influence: 1. 测试域管用户首次登录场景,验证密码过期信息是否正确同步 2. 测试普通用户登录,确保密码过期检查无回归 3. 验证任意用户在认证结束时不会发生崩溃 4. 测试多用户切换,包括本地用户和域管用户之间的切换 PMS: BUG-367143 Change-Id: Icbbe23ae6246d4e60c1dbc34d35a4f414d86e8df
Synchronize source files from linuxdeepin/dde-session-shell. Source-pull-request: linuxdeepin/dde-session-shell#69
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: xionglinlin, yixinshark The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Synchronize source files from linuxdeepin/dde-session-shell. Source-pull-request: linuxdeepin/dde-session-shell#69
handling
updatePasswordExpiredStateBySPName()function that used glibcgetspnamto check password expiry, as it was only needed for first-time domain user logincheckAccount()are added to the local model immediately, so subsequent operations likecreateAuthentication()can find them viafindUserByName()updatePasswordExpiredInfo()through existing model interfaces for all users, including newly discovered onesonAuthFinished()wherem_model->currentUser()could be null when checkingexpiredState()Log: Fix domain user first login; remove redundant glibc password expiry logic
Influence:
fix: 移除基于 glibc 的密码过期检查并修正域管用户处理
updatePasswordExpiredStateBySPName()函数,该函数使用 glibc 的getspnam检查密码过期,只在域管用户首次登录时需要checkAccount()中新发现的域管用户立即加入本地 model,使得后 续createAuthentication()等操作能通过findUserByName()找到updatePasswordExpiredInfo()onAuthFinished()中m_model->currentUser()可能为空时检查expiredState()导致的崩溃Log: 修复域管用户首次登录;移除冗余的 glibc 密码过期逻辑
Influence:
PMS: BUG-367143
Change-Id: Icbbe23ae6246d4e60c1dbc34d35a4f414d86e8df
Summary by Sourcery
Align password expiry handling with the user model and remove legacy glibc-based logic.
Bug Fixes:
Enhancements:
Chores: