fix: Obtain password only during user activity #426
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
Reviewer's GuideIntegrate DBus-based login session monitoring in NetHotspotController to only retrieve the hotspot password when the user is active, and update QML security settings to use PSK flag 0 for WPA and SAE networks. Sequence diagram for DBus-based user activity monitoring in NetHotspotControllersequenceDiagram
participant NetHotspotController
participant DBus
participant LoginSession
NetHotspotController->>DBus: createMethodCall("Get", "Display")
DBus-->>NetHotspotController: updateDisplay(QDBusVariant)
NetHotspotController->>LoginSession: createMethodCall("Get", "Remote")
LoginSession-->>NetHotspotController: QDBusPendingReply<QDBusVariant>
NetHotspotController->>DBus: connect("PropertiesChanged", onLoginSessionPropertiesChanged)
DBus-->>NetHotspotController: onLoginSessionPropertiesChanged(properties)
NetHotspotController->>NetHotspotController: updateConfig() (only if user active)
Updated class diagram for NetHotspotControllerclassDiagram
class NetHotspotController {
-HotspotController *m_hotspotController
-bool m_isEnabled
-bool m_enabledable
-bool m_deviceEnabled
-bool m_userActive
-QVariantMap m_config
-QStringList m_shareDevice
-QStringList m_optionalDevice
+void updateEnabledable()
+void updateData()
+void updateConfig()
+void onLoginSessionPropertiesChanged(const QString &, const QVariantMap &properties, const QStringList &)
+void updateDisplay(const QDBusVariant &display)
}
NetHotspotController --> HotspotController
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:
- Consider replacing QDBusPendingReply.waitForFinished with an asynchronous callback to avoid blocking the main event loop when fetching session properties.
- Instead of defaulting m_userActive to true, perform an initial D-Bus query of the Active property on startup (and handle errors) so you don't trigger updateConfig prematurely.
- The QML change setting psk-flags to 0 makes password storage less restrictive—please add an inline comment or doc explaining this security decision for future maintainers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider replacing QDBusPendingReply.waitForFinished with an asynchronous callback to avoid blocking the main event loop when fetching session properties.
- Instead of defaulting m_userActive to true, perform an initial D-Bus query of the Active property on startup (and handle errors) so you don't trigger updateConfig prematurely.
- The QML change setting psk-flags to 0 makes password storage less restrictive—please add an inline comment or doc explaining this security decision for future maintainers.
## Individual Comments
### Comment 1
<location> `net-view/operation/private/nethotspotcontroller.cpp:38-40` </location>
<code_context>
connect(m_hotspotController, &HotspotController::itemAdded, this, &NetHotspotController::updateConfig);
connect(m_hotspotController, &HotspotController::itemRemoved, this, &NetHotspotController::updateConfig);
connect(m_hotspotController, &HotspotController::itemChanged, this, &NetHotspotController::updateConfig, Qt::QueuedConnection);
+ QDBusMessage msg = QDBusMessage::createMethodCall("org.freedesktop.login1", "/org/freedesktop/login1/user/self", "org.freedesktop.DBus.Properties", "Get");
+ msg << "org.freedesktop.login1.User" << "Display";
+ QDBusConnection::systemBus().callWithCallback(msg, this, SLOT(updateDisplay(QDBusVariant)));
}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider error handling for DBus call failures.
If the DBus service is unavailable or the call fails, the code may behave unexpectedly. Please add error handling or a fallback to ensure robustness.
Suggested implementation:
```cpp
QDBusMessage msg = QDBusMessage::createMethodCall("org.freedesktop.login1", "/org/freedesktop/login1/user/self", "org.freedesktop.login1.User", "Display");
msg << "org.freedesktop.login1.User" << "Display";
QDBusConnection::systemBus().callWithCallback(
msg,
this,
SLOT(updateDisplay(QDBusVariant)),
SLOT(handleDBusError(QDBusError))
);
```
```cpp
}
// Error handling slot for DBus call failures
void NetHotspotController::handleDBusError(const QDBusError &error)
{
qWarning() << "DBus call failed:" << error.name() << error.message();
// Optionally, set a fallback value or notify the user/UI
}
```
You must declare the new slot `handleDBusError(const QDBusError &error)` in the header file (`nethotspotcontroller.h`) under the `public slots:` or `private slots:` section, depending on your class design.
Example:
```cpp
private slots:
void handleDBusError(const QDBusError &error);
```
</issue_to_address>
### Comment 2
<location> `net-view/operation/private/nethotspotcontroller.cpp:218-222` </location>
<code_context>
+ }
+ QDBusMessage msg = QDBusMessage::createMethodCall("org.freedesktop.login1", path.path(), "org.freedesktop.DBus.Properties", "Get");
+ msg << "org.freedesktop.login1.Session" << "Remote";
+ QDBusPendingReply<QDBusVariant> remoteReply = QDBusConnection::systemBus().asyncCall(msg);
+ remoteReply.waitForFinished();
+ if (!remoteReply.value().variant().toBool()) {
+ QDBusConnection::systemBus().connect("org.freedesktop.login1", path.path(), "org.freedesktop.DBus.Properties", "PropertiesChanged", this, SLOT(onLoginSessionPropertiesChanged(QString, QVariantMap, QStringList)));
</code_context>
<issue_to_address>
**suggestion (performance):** Blocking wait on async DBus call may impact responsiveness.
Consider replacing waitForFinished() with a non-blocking mechanism, such as connecting the reply to a slot or lambda, to prevent blocking the main thread.
```suggestion
QDBusPendingReply<QDBusVariant> *remoteReply = new QDBusPendingReply<QDBusVariant>(QDBusConnection::systemBus().asyncCall(msg));
QObject *self = this;
QObject::connect(remoteReply, &QDBusPendingReply<QDBusVariant>::finished, [remoteReply, path, self]() {
if (!remoteReply->value().variant().toBool()) {
QDBusConnection::systemBus().connect("org.freedesktop.login1", path.path(), "org.freedesktop.DBus.Properties", "PropertiesChanged", self, SLOT(onLoginSessionPropertiesChanged(QString, QVariantMap, QStringList)));
}
remoteReply->deleteLater();
});
```
</issue_to_address>
### Comment 3
<location> `net-view/operation/private/nethotspotcontroller.cpp:198-201` </location>
<code_context>
+void NetHotspotController::onLoginSessionPropertiesChanged(const QString &, const QVariantMap &properties, const QStringList &)
+{
+ if (properties.contains("Active")) {
+ m_userActive = properties.value("Active").value<bool>();
+ updateConfig();
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Type conversion from QVariant to bool may be fragile.
Using value<bool>() may not handle type mismatches or missing values safely. Prefer QVariant::toBool() or check the type before conversion to avoid unexpected behavior.
```suggestion
if (properties.contains("Active")) {
const QVariant activeValue = properties.value("Active");
m_userActive = activeValue.toBool();
updateConfig();
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Obtain password only during user activity pms: BUG-338367
deepin pr auto review我来对这个代码变更进行详细审查：
// 在nethotspotcontroller.h中添加注释
/**
* @brief m_userActive 用户当前是否处于活动状态
* 用于控制是否需要认证才能访问热点配置
*/
bool m_userActive;
// 在nethotspotcontroller.cpp中优化DBUS调用
void NetHotspotController::initializeDBusConnection()
{
// 初始化DBUS连接
QDBusMessage msg = QDBusMessage::createMethodCall("org.freedesktop.login1",
"/org/freedesktop/login1/user/self",
"org.freedesktop.DBus.Properties",
"Get");
msg << "org.freedesktop.login1.User" << "Display";
QDBusPendingCallWatcher *watcher = new QDBusPendingCallWatcher(
QDBusConnection::systemBus().asyncCall(msg), this);
connect(watcher, &QDBusPendingCallWatcher::finished,
this, &NetHotspotController::handleDisplayResponse);
}
// 添加错误处理
void NetHotspotController::handleDisplayResponse(QDBusPendingCallWatcher *watcher)
{
QDBusPendingReply<QDBusVariant> reply = *watcher;
if (reply.isError()) {
qWarning() << "Failed to get display property:" << reply.error().message();
watcher->deleteLater();
return;
}
// 处理正常响应
updateDisplay(reply.value());
watcher->deleteLater();
}
总的来说，这个变更主要是增加了用户活动状态的检查机制，但在代码结构、错误处理和安全性方面还有改进空间。建议在合并前进行更全面的测试。 |
mhduiy
approved these changes
Nov 3, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: caixr23, mhduiy 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.
Obtain password only during user activity
pms: BUG-338367
Summary by Sourcery
Defer hotspot password retrieval until the user session is active by integrating systemd-logind DBus signals to monitor display and session ‘Active’ properties, and update the configuration only when the session is active. Also adjust the default PSK flags in the Hotspot QML to zero.
New Features:
Bug Fixes:
Enhancements: