Skip to content

Conversation

@fly602
Copy link
Contributor

@fly602 fly602 commented Jan 20, 2026

lightdm的state_user文件安全整改后,改为lightdm用户组,修改该文件所属组为可读写权限

但是dde-system-daemon设置快速登录也会修改该文件,通过g_key_file_save_to_file会创建新文件并覆盖旧文件,会将其权限改成root,导致切换用户时修改文件无权限.

Log: 解决切换用户失败的问题

PMS: BUG-339935

Influence: quicklogin

Summary by Sourcery

Bug Fixes:

  • Fix quick login configuration updates resetting the LightDM state file permissions so that subsequent user switching no longer fails.

lightdm的state_user文件安全整改后,改为lightdm用户组,修改该文件所属组为可读写权限

但是dde-system-daemon设置快速登录也会修改该文件,通过g_key_file_save_to_file会创建新文件并覆盖旧文件,会将其权限改成root,导致切换用户时修改文件无权限.

Log: 解决切换用户失败的问题

PMS: BUG-339935

Influence: quicklogin
@sourcery-ai
Copy link

sourcery-ai bot commented Jan 20, 2026

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Ensures the LightDM greeter state file retains correct ownership and now also enforces group-writable permissions after quick-login updates, preventing permission issues when switching users.

Sequence diagram for SetQuickLogin updating lightdm greeter state file permissions

sequenceDiagram
    participant Daemon as dde_system_daemon
    participant DisplayManager as display_manager_SetQuickLogin
    participant LightdmUser as lightdm_user
    participant OS as os
    participant GreeterStateFile as greeter_state_file

    Daemon->>DisplayManager: SetQuickLogin(username, enabled)
    activate DisplayManager

    DisplayManager->>GreeterStateFile: g_key_file_save_to_file
    note right of GreeterStateFile: File recreated with owner root and default permissions

    DisplayManager->>LightdmUser: Lookup("lightdm")
    LightdmUser-->>DisplayManager: Gid

    DisplayManager->>OS: Chown(GreeterStateFile, uid 0, gid lightdmGID)
    OS-->>DisplayManager: success

    DisplayManager->>OS: Chmod(GreeterStateFile, 0664)
    OS-->>DisplayManager: success

    note right of GreeterStateFile: Ownership root:lightdm and mode 664 (rw-rw-r--)

    DisplayManager-->>Daemon: error or nil
    deactivate DisplayManager
Loading

Flow diagram for SetQuickLogin permission correction logic

flowchart TD
    A[SetQuickLogin called] --> B[Perform quick login configuration
including g_key_file_save_to_file]
    B --> C[Defer permission fix for greeter state file]

    subgraph Deferred_cleanup
        C --> D[Lookup lightdm user]
        D -->|success| E[Parse lightdm Gid to integer]
        D -->|failure| I[Return from deferred function]

        E -->|success| F[Chown GreeterStateFile to uid 0, gid lightdmGID]
        E -->|failure| I

        F --> G[Chmod GreeterStateFile to 0664
rw-rw-r--]
        G --> H[End deferred cleanup
file owned by root:lightdm and group writable]
    end
Loading

File-Level Changes

Change Details Files
Ensure greeter state file keeps root:lightdm ownership and group-writable permissions after quick-login updates to avoid permission errors.
  • Expanded the cleanup defer comment to document that permissions should be 664 (rw-rw-r--) in addition to ownership root:lightdm.
  • After updating the greeter state file, retained logic to reset its group ownership to the lightdm group using os.Chown.
  • Added a call to os.Chmod to explicitly set the greeter state file permissions to 0664 after it is rewritten.
accounts1/users/display_manager.go

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@deepin-ci-robot
Copy link

deepin pr auto review

这段代码的修改主要是在 SetQuickLogin 函数的 defer 块中增加了对文件权限的设置。以下是对这段代码的审查意见,包括语法逻辑、代码质量、代码性能和代码安全方面:

1. 语法逻辑

  • 基本正确:代码在语法上没有问题,os.Chmod 的调用方式是正确的。
  • 错误处理缺失os.Chmod 的返回值被忽略了(使用了 _),这意味着如果权限设置失败,程序不会收到任何通知。虽然 defer 中的错误处理通常比较困难,但至少应该记录日志。

2. 代码质量

  • 注释清晰:注释很好地说明了修改的目的,这是好的实践。
  • 错误处理不一致:前面的 os.Chown 也忽略了错误,这里延续了相同的模式。如果可能,应该统一错误处理策略。
  • 硬编码权限:权限 0664 是硬编码的,建议定义为常量以提高可维护性。

3. 代码性能

  • 无显著影响os.Chmodos.Chown 都是系统调用,性能开销很小,不会成为瓶颈。
  • defer 的使用defer 的使用是合理的,确保文件操作后总是尝试恢复权限和所有权。

4. 代码安全

  • 权限设置0664 (rw-rw-r--) 允许组用户和其他用户读取和写入文件。需要确认这是否符合安全要求:
    • 如果文件包含敏感信息,应该限制权限(例如 06400600)。
    • 如果文件需要被其他服务读取,0664 可能是合理的。
  • 忽略错误的风险:忽略 os.Chmod 的错误可能导致文件权限不符合预期,从而引发安全问题(如权限过宽或过窄)。

改进建议

  1. 定义常量:将权限值定义为常量,便于维护和修改。
    const (
        GreeterStateFileMode = 0664
    )
  2. 记录错误:至少记录 os.Chmod 的错误,便于排查问题。
    if err := os.Chmod(GreeterStateFile, GreeterStateFileMode); err != nil {
        log.Printf("Warning: failed to set permissions for %s: %v", GreeterStateFile, err)
    }
  3. 检查权限合理性:确认 0664 是否符合最小权限原则。如果不需要其他用户写入,可以改为 0644 (rw-r--r--) 或更严格。

改进后的代码示例

const (
    GreeterStateFileMode = 0664
)

func SetQuickLogin(username string, enabled bool) error {
    // ... 其他代码 ...
    defer func() {
        // 保持文件所有权为 root:lightdm,权限为 0664(rw-rw-r--)
        lightdmUser, err := user.Lookup("lightdm")
        if err == nil {
            lightdmGID, err := strconv.Atoi(lightdmUser.Gid)
            if err == nil {
                if err := os.Chown(GreeterStateFile, 0, lightdmGID); err != nil {
                    log.Printf("Warning: failed to set ownership for %s: %v", GreeterStateFile, err)
                }
                if err := os.Chmod(GreeterStateFile, GreeterStateFileMode); err != nil {
                    log.Printf("Warning: failed to set permissions for %s: %v", GreeterStateFile, err)
                }
            }
        }
    }()
    // ... 其他代码 ...
}

总结

这段代码的修改是合理的,但可以通过定义常量、记录错误和检查权限合理性来进一步改进。安全性方面,需要确认 0664 是否符合实际需求。

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've left some high level feedback:

  • Consider avoiding magic numbers by defining a named constant or using os.FileMode(0o664) so the intended permissions are clearer and less error‑prone to change later.
  • The errors from os.Chown and os.Chmod are currently ignored; if these operations are important for correct behavior, it may be safer to at least log failures so permission issues can be diagnosed in the field.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- Consider avoiding magic numbers by defining a named constant or using `os.FileMode(0o664)` so the intended permissions are clearer and less error‑prone to change later.
- The errors from `os.Chown` and `os.Chmod` are currently ignored; if these operations are important for correct behavior, it may be safer to at least log failures so permission issues can be diagnosed in the field.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: fly602, robertkill

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@fly602 fly602 merged commit eaa1a41 into linuxdeepin:master Jan 20, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants