refactor: cut off helper #56
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hi @calsys456. Thanks for your PR. I'm waiting for a linuxdeepin member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Reviewer's GuideRefactors authentication and session startup by inlining the former helper process into the daemon: introduces a new in-process Auth implementation using PAM and UserSession, removes the separate auth/helper submodules, converts Auth from a request-driven QObject API to a simple state/field-based class, and rewires Display and Greeter to use the new interface and exit-status enum while simplifying session handling and greeter plumbing. Sequence diagram for authentication and user session startup with new AuthsequenceDiagram
autonumber
actor User
participant Greeter
participant Display
participant Auth
participant Pam
participant UserSession
User ->> Greeter: enter credentials
Greeter ->> Auth: new Auth(parent)
Greeter ->> Auth: set fields (user, displayServerCmd, sessionPath, singleMode, skipAuth)
Note over Greeter,Auth: For normal login Display creates Auth and sets session info
par Identify_only_or_full
Display ->> Auth: start(secret)
and
Greeter ->> Auth: start(secret_or_empty)
end
Auth ->> Pam: user = user
Auth ->> Pam: start()
Pam -->> Auth: bool started
alt PAM start failed
Auth -->> Display: authentication(user, false, identifyOnly)
Auth -->> Greeter: authentication(user, false, identifyOnly)
Auth ->> Auth: utmpLogin(..., authSuccessful=false)
Auth -->> Display: finished(AUTH_ERROR)
Auth -->> Greeter: finished(AUTH_ERROR)
else PAM started
alt skipAuth is false
Auth ->> Pam: authenticate(secret)
Pam -->> Auth: bool success
alt auth failed
Auth -->> Display: authentication(user, false, identifyOnly)
Auth -->> Greeter: authentication(user, false, identifyOnly)
Auth ->> Auth: utmpLogin(..., authSuccessful=false)
Auth -->> Display: finished(AUTH_ERROR)
Auth -->> Greeter: finished(AUTH_ERROR)
else auth ok
Auth -->> Display: authentication(user, true, identifyOnly)
Auth -->> Greeter: authentication(user, true, identifyOnly)
end
else skipAuth is true
Auth -->> Display: authentication(user, true, identifyOnly)
Auth -->> Greeter: authentication(user, true, identifyOnly)
end
opt sessionPath not empty
Auth ->> Pam: openSession(environment)
Pam -->> Auth: env or failure
alt openSession failed
Auth -->> Display: sessionStarted(false, 0)
Auth -->> Greeter: sessionStarted(false, 0)
Auth -->> Display: finished(SESSION_ERROR)
Auth -->> Greeter: finished(SESSION_ERROR)
else openSession ok
Auth ->> Auth: fill HOME,PWD,SHELL,USER,LOGNAME
Auth ->> UserSession: setProcessEnvironment(env)
Auth ->> UserSession: start()
Auth ->> Auth: utmpLogin(..., authSuccessful=true)
Auth ->> Auth: xdgSessionId = env[XDG_SESSION_ID]
Auth -->> Display: sessionStarted(true, xdgSessionId)
Auth -->> Greeter: sessionStarted(true, xdgSessionId)
end
end
end
rect rgb(235,235,235)
Note over UserSession,Auth: Later, when UserSession exits, Auth emits finished status via UserSession finished connection
end
Class diagram for new in-daemon Auth and UserSession modelclassDiagram
class Auth {
<<QObject>>
+bool active
+QString displayServerCmd
+QString sessionPath
+Session_Type sessionType
+QString sessionFileName
+QString user
+QByteArray cookie
+bool autologin
+bool greeter
+bool singleMode
+bool identifyOnly
+bool skipAuth
+QProcessEnvironment environment
+int id
+static int lastId
+QString sessionId
+int tty
+int xdgSessionId
+Auth(QObject *parent)
+~Auth()
+void start(QByteArray secret)
+void stop()
+void authentication(QString user, bool success, bool identifyOnly)
+void sessionStarted(bool success, int xdgSessionId)
+void finished(Auth_ExitStatus status)
-Pam *m_pam
-UserSession *m_session
-void utmpLogin(QString vt, QString displayName, QString user, qint64 pid, bool authSuccessful)
-void utmpLogout(QString vt, QString displayName, qint64 pid)
}
class Auth_ExitStatus {
<<enumeration>>
SUCCESS
AUTH_ERROR
SESSION_ERROR
OTHER_ERROR
DISPLAYSERVER_ERROR
TTY_ERROR
}
class UserSession {
<<QProcess>>
+qint64 cachedProcessId
+UserSession(Auth *parent)
+void start()
+void stop()
-void childModifier()
-QTemporaryFile m_xauthFile
-Auth *m_auth
}
class Pam {
+QString user
+bool sessionOpened
+Pam(QObject *parent)
+bool start()
+bool authenticate(QByteArray secret)
+std_optional_QProcessEnvironment openSession(QProcessEnvironment baseEnv)
+void closeSession()
}
class Display {
+QVector~Auth*~ m_auths
+Auth *m_currentAuth
+int m_terminalId
+int m_sessionTerminalId
+void identifyUser(QString user, QString password)
+void startAuth(QString user, QString password, Session session)
+void slotAuthenticationFinished(QString user, bool success, bool identifyOnly)
+void slotSessionStarted(bool success, int xdgSessionId)
+void slotHelperFinished(Auth_ExitStatus status)
}
class Greeter {
+Auth *m_auth
+bool m_singleMode
+bool m_userActivated
+QString m_user
+QString m_displayServerCmd
+void start()
+void onSessionStarted(bool success, int xdgSessionId)
+void onHelperFinished(Auth_ExitStatus status)
+bool isRunning() const
}
Auth "1" o-- "1" Pam : owns
Auth "1" o-- "1" UserSession : owns
UserSession --> Auth : parent
Display --> Auth : manages
Greeter --> Auth : manages
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The new
UserSession::cachedProcessIdmember is never updated (previouslym_cachedProcessIdwas set afterwaitForStarted()), soAuth::stop()may log out with an invalid PID and skip utmp/wtmp logout; consider updatingcachedProcessIdon process start/finish and using it inutmpLogout. - In
Auth::start(), theLOGIN_FAILEDmacro usesenvironmentto deriveDISPLAY/XDG_VTNR, butenvironmentmay still be empty at that point, so failed logins will be recorded with missing TTY/display; consider passing in or constructing a minimal environment (or TTY/display) before callingutmpLoginon failure.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `UserSession::cachedProcessId` member is never updated (previously `m_cachedProcessId` was set after `waitForStarted()`), so `Auth::stop()` may log out with an invalid PID and skip utmp/wtmp logout; consider updating `cachedProcessId` on process start/finish and using it in `utmpLogout`.
- In `Auth::start()`, the `LOGIN_FAILED` macro uses `environment` to derive `DISPLAY`/`XDG_VTNR`, but `environment` may still be empty at that point, so failed logins will be recorded with missing TTY/display; consider passing in or constructing a minimal environment (or TTY/display) before calling `utmpLogin` on failure.
## Individual Comments
### Comment 1
<location> `src/daemon/Auth.cpp:108-117` </location>
<code_context>
+ void Auth::stop() {
+ Q_ASSERT(getuid() == 0);
+
+ qint64 pid = m_session->processId();
+ m_session->stop();
+ if (!identifyOnly && m_pam->sessionOpened) {
+ m_pam->closeSession();
+ }
+
+ // write logout to utmp/wtmp
+ if (pid < 0)
+ return;
+ QProcessEnvironment env = m_session->processEnvironment();
+ QString vt = env.value(QStringLiteral("XDG_VTNR"));
+ QString displayId = env.value(QStringLiteral("DISPLAY"));
+ utmpLogout(vt, displayId, pid);
+ }
+
</code_context>
<issue_to_address>
**issue:** `pid < 0` check likely won’t filter out the "no process" case, leading to bogus utmp logout records.
In `Auth::stop()` you fetch the PID before stopping the session:
```cpp
qint64 pid = m_session->processId();
...
// write logout to utmp/wtmp
if (pid < 0)
return;
...
utmpLogout(vt, displayId, pid);
```
On Qt, `QProcess::processId()` usually returns `0` when there is no running process, not a negative value, so this guard won’t stop a logout with PID 0 for sessions that never actually started. Please use a check that matches the documented “no process” value, e.g. `pid <= 0`, before calling `utmpLogout`.
</issue_to_address>
### Comment 2
<location> `src/daemon/Auth.cpp:72-42` </location>
<code_context>
+
+ Q_EMIT authentication(user, true, identifyOnly);
+
+ if (!sessionPath.isEmpty()) {
+ auto sessionEnv = m_pam->openSession(environment);
+ if (!sessionEnv.has_value()) {
+ Q_EMIT sessionStarted(false, 0);
+ Q_EMIT finished(SESSION_ERROR);
+ return;
+ }
+
+ QProcessEnvironment env = *sessionEnv;
</code_context>
<issue_to_address>
**issue (bug_risk):** `Auth::start()` never emits `finished()` when no session is started (e.g. identify-only), so listeners may leak state.
`finished(...)` is only emitted inside the `!sessionPath.isEmpty()` branch (either on `SESSION_ERROR` or via `UserSession` exit). When `sessionPath` is empty (identify-only), `Auth::start()` returns without ever emitting `finished`, but callers like `Display::identifyUser()` still rely on it (e.g. `slotHelperFinished` removing the `Auth` from `m_auths`). This can leave stale `Auth` instances and inconsistent state. Please emit `finished(SUCCESS)` (or another appropriate status) at the end of `Auth::start()` when `sessionPath` is empty to keep the lifecycle consistent.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
zccrs
previously approved these changes
Dec 18, 2025
calsys456
force-pushed
the
refactor
branch
3 times, most recently
from
db20bad to
e12bb91
Compare
- Merge HelperApp into Auth - Move Auth & UserSession into daemon - Simplify Auth, remove redundant logics This commit largely simplify the code. Note that the function of starting sessions other than treeland will temporarily not work after this commit, due to some wrongly designed obsolete code. This will be fixed soon in the next commit.
zccrs
approved these changes
Dec 22, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: calsys456, zccrs 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 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit largely simplify the code.
Note that the function of starting sessions other than treeland will temporarily not work after this commit, due to some wrongly designed obsolete code. This will be fixed soon in the next commit.
Summary by Sourcery
Merge authentication and session management into the daemon, removing the separate helper components and simplifying the login/session startup flow.
Enhancements:
Build: