Skip to content

Conversation

@fly602
Copy link
Contributor

@fly602 fly602 commented Aug 13, 2025

  1. /lib/systemd/system/deepin-locale-helper.service 应该默认仅deepin和uos打包;
  2. /lib/systemd/system/deepin-sound-theme-player.service User不应该再使用deepin-sound-player;
  3. /var/lib/polkit-1/localauthority/10-vendor.d/org.deepin.dde.device.pkla 应该改用rules实现;

Log: dde-api安全整改
PMS: TASK-369021

Summary by Sourcery

Implement security hardening for dde-api by restricting default service installations, migrating polkit policies to rules, switching service users, updating dependencies, and cleaning up obsolete packaging.

New Features:

  • Add INSTALL_LOCALE_HELPER build flag to control installation of deepin-locale-helper.service

Enhancements:

  • Migrate polkit .pkla files to JavaScript rules under rules.d for device permissions
  • Switch deepin-sound-theme-player service to run as deepin-daemon user without a home directory
  • Limit installation of deepin-locale-helper.service to explicit builds

Build:

  • Update go.mod to replace and bump dependencies to deepin-maintained modules and latest versions
  • Add INSTALL_LOCALE_HELPER variable to Makefile for conditional service installation

Deployment:

  • Update RPM spec to use deepin-daemon user/group and install polkit rules instead of localauthority .pkla

Chores:

  • Remove outdated ArchLinux and Debian packaging scripts and cleanup files

@sourcery-ai
Copy link

sourcery-ai bot commented Aug 13, 2025

Reviewer's Guide

This PR enforces security improvements by migrating polkit rules, tightening service installation defaults, replacing the deepin-sound-player user with deepin-daemon, removing GSettings-based config loading, and updating Go dependencies and packaging scripts accordingly.

Class diagram for config loading changes in sound-theme-player

classDiagram
    class config {
        +bool Enabled
        +bool DesktopLoginEnabled
        +bool SystemShutdownEnabled
        +string Theme
    }
    class ConfigLoader {
        +saveUserConfig(uid int, cfg config) error
        +loadConfig(filename string, cfg config) error
    }
    config <.. ConfigLoader
    %% Removed: GSettings-based loading logic
    %% Removed: _loadDefaultCfgFromGSettings
    %% Removed: soundutils dependency
    %% Removed: gio dependency
Loading

Flow diagram for conditional installation of deepin-locale-helper.service

flowchart TD
    A[Start install] --> B{INSTALL_LOCALE_HELPER == 1?}
    B -- Yes --> C[Install deepin-locale-helper.service and related files]
    B -- No --> D[Remove deepin-locale-helper.service and related files]
    C --> E[Continue install]
    D --> E[Continue install]
Loading

File-Level Changes

Change Details Files
Migrate polkit authority to rules.d and conditionally install locale helper service
  • Move device authorization from 10-vendor.d/.pkla to rules.d/.rules
  • Introduce INSTALL_LOCALE_HELPER flag in Makefile to skip locale helper service by default
  • Update RPM spec to reference rules.d path and remove pkla entries
  • Remove legacy polkit .pkla files in Arch and Debian packaging
misc/polkit-rules/org.deepin.dde.device.rules
misc/polkit-localauthority/org.deepin.dde.device.pkla
Makefile
rpm/dde-api.spec
archlinux/deepin-api.install
debian/dde-api.postinst
Replace deepin-sound-player user with deepin-daemon
  • Change user/group creation and shared state directory from deepin-sound-player to deepin-daemon
  • Update D-Bus busconfig policies to reference deepin-daemon
  • Adjust service file ownership and descriptions
rpm/dde-api.spec
misc/conf/org.deepin.dde.SoundThemePlayer1.conf
misc/system-services/org.deepin.dde.SoundThemePlayer1.service
Remove GSettings-based default loading and simplify homeDir logic
  • Eliminate _loadDefaultCfgFromGSettings flag and all GSettings import and calls
  • Always assign defaultHomeDir for homeDir in main.go
sound-theme-player/config.go
sound-theme-player/main.go
Update Go module dependencies to Linux Deepin internal modules
  • Replace jinzhu/gorm and mattn/go-sqlite3 with go-dbus-factory, go-gir, go-lib
  • Bump indirect dependencies: golang/x/image v0.10.0, golang/x/sys, google/go-cmp, freetype, zaf/g711
go.mod
go.sum

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

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 @fly602 - I've reviewed your changes - here's some feedback:

  • Verify that removing the GSettings fallback for loading defaults still provides sensible initial configuration for fresh installs and tests.
  • After changing the service user to ‘deepin-daemon’, audit all scripts and service files to ensure there are no leftover references to ‘deepin-sound-player’.
  • Confirm that migrating from the old .pkla file to the new rules.d approach preserves the intended polkit permissions across all target distributions.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Verify that removing the GSettings fallback for loading defaults still provides sensible initial configuration for fresh installs and tests.
- After changing the service user to ‘deepin-daemon’, audit all scripts and service files to ensure there are no leftover references to ‘deepin-sound-player’.
- Confirm that migrating from the old .pkla file to the new rules.d approach preserves the intended polkit permissions across all target distributions.

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.

@fly602 fly602 force-pushed the master branch 5 times, most recently from 68ed894 to 23b1697 Compare August 14, 2025 06:12
fly602 added 2 commits August 14, 2025 14:17
/lib/systemd/system/deepin-locale-helper.service 应该默认仅deepin和uos打包;

Log: dde-api安全整改
PMS: TASK-369021
 /lib/systemd/system/deepin-sound-theme-player.service User不应该再使用deepin-sound-player

Log: dde-api安全整改
PMS: TASK-369021
/var/lib/polkit-1/localauthority/10-vendor.d/org.deepin.dde.device.pkla 应该改用rules实现;

Log: dde-api安全整改
PMS: TASK-369021
@deepin-bot
Copy link
Contributor

deepin-bot bot commented Aug 15, 2025

TAG Bot

New tag: 6.0.23
DISTRIBUTION: unstable
Suggest: synchronizing this PR through rebase #147

makefile中入参改用ifneq判断,dde-api安装时创建deepin-daemon用户

Log: dde-api安全整改
PMS: TASK-369021
@deepin-ci-robot
Copy link

deepin pr auto review

根据提供的git diff,我来分析一下代码的改进点:

  1. 用户管理变更:
  • deepin-sound-player 用户改为 deepin-daemon 用户
  • 删除了专门的 sound-player 用户创建和管理逻辑
  • 优点:简化了用户管理,减少系统用户数量
  • 建议:需要确保 deepin-daemon 用户有足够的权限来执行声音相关操作
  1. 系统服务改进:
  • 增强了 systemd 服务的安全限制,如 ProtectSystem=strictNoNewPrivileges=yes
  • 添加了 InaccessiblePaths 来保护敏感目录
  • 建议:考虑添加 PrivateUsers=yes 以进一步提高安全性
  1. Polkit 权限管理:
  • 从 .pkla 文件改为使用 .rules 文件
  • 添加了所有者标注 org.freedesktop.policykit.owner
  • 优点:使用 polkit rules 更灵活且安全
  • 建议:确保规则权限设置合理,避免过度授权
  1. 安装逻辑优化:
  • 添加了 INSTALL_LOCALE_HELPER 可选变量
  • 默认不安装 locale helper 相关服务
  • 优点:提高了安装的灵活性
  • 建议:考虑在文档中说明这个可选变量的用途
  1. 声音主题播放器改进:
  • 移除了从 gsettings 加载默认配置的代码
  • 简化了用户目录获取逻辑
  • 优点:减少了依赖,提高了稳定性
  • 建议:考虑添加配置文件路径的验证
  1. 目录结构优化:
  • 使用 StateDirectory 替代手动创建目录
  • 统一了数据目录管理
  • 优点:遵循 systemd 最佳实践
  • 建议:确保目录权限设置正确
  1. 文件清理:
  • 删除了不再需要的 .pkla 文件
  • 删除了 archlinux 特定的用户管理文件
  • 优点:减少了维护负担
  • 建议:确保所有相关配置都已更新

潜在问题:

  1. 权限变更可能需要测试确保功能正常
  2. 移除 gsettings 集成可能影响某些功能
  3. 用户变更可能需要数据迁移脚本

建议:

  1. 添加详细的迁移文档
  2. 增加测试用例覆盖权限变更
  3. 考虑添加配置文件版本控制
  4. 完善错误处理机制
  5. 添加日志记录以便调试

总体来说,这些改进提高了系统的安全性和可维护性,但需要确保充分测试以避免引入问题。

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: fly602, KT-lcz, 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 0dfcc46 into linuxdeepin:master Aug 19, 2025
16 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.

4 participants