fix: ensure upgrade delivery enabled only for professional

[zhaohuiw42]

Task: https://pms.uniontech.com/task-view-388365.html

[github-actions]

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

[deepin-ci-robot]

deepin pr auto review

代码审查报告

本次代码修改主要实现了"升级交付功能(Upgrade Delivery/P2P Update)"仅对专业版系统开放的功能。整体逻辑清晰，测试覆盖较全面，但在性能、安全性和代码健壮性方面存在一些改进空间。

1. 语法与逻辑

• 逻辑正确性：
  • upgradeDeliveryEnabledForCurrentEdition 函数逻辑正确：只有当入参 enable 为 true 且当前系统为专业版时，才返回 true。
  • SetUpgradeDeliveryEnabled 方法修改后，实际保存的值是经过 upgradeDeliveryEnabledForCurrentEdition 过滤后的值。这意味着如果用户在非专业版系统上尝试开启此功能，配置文件中实际保存的会是 false。这符合"仅对专业版开放"的业务需求。
• 测试覆盖：
  • TestUpgradeDeliveryEnabledOnlyForProfessional 测试用例设计良好，覆盖了专业版开启/关闭、社区版开启、企业版开启等场景。
  • 使用 t.TempDir() 和 defer 恢复全局变量 osVersionFile 是处理测试中文件依赖和全局变量污染的良好实践。

2. 代码质量

• 全局变量依赖：
  • 问题：isProfessionalEdition 函数直接依赖硬编码的全局变量 osVersionFile (/etc/os-version)。这导致代码难以测试（虽然测试中通过 hack 全局变量解决了），也降低了函数的灵活性。
  • 建议：建议将文件路径作为参数传递给该函数，例如 isProfessionalEdition(filePath string)。或者在 Config 结构体中保存该路径。如果是为了保持接口一致，可以保留一个无参的包装函数调用带参的内部实现。
• SetUpgradeDeliveryEnabled 行为一致性：
  • 问题：SetUpgradeDeliveryEnabled 接收一个 enable 参数，但实际保存到持久化存储的值可能不是传入的 enable（例如在非专业版上传入 true，保存 false）。这可能会导致调用者困惑，如果调用者期望"保存我传入的值"。
  • 建议：虽然这可能是业务需求（禁止非专业版写入 true），但建议在注释中明确说明此行为，或者在日志中记录"由于非专业版，已强制关闭该功能"。

3. 代码性能

• 文件 I/O 性能：
  • 问题：isProfessionalEdition 函数在每次调用时都会执行文件读取和解析 (LoadFromFile)。
  • 影响：该函数在 getConfigFromDSettings（初始化时调用）和 SetUpgradeDeliveryEnabled（用户设置时调用）中被调用。频率尚可接受，不是高频热点。
  • 优化建议：
    1. 缓存：由于系统版本信息在运行期间通常不会改变，可以考虑在 Config 初始化时读取一次，将结果缓存在 Config 结构体中（例如 c.isProfessional bool）。后续判断直接读取内存即可，避免重复的磁盘 I/O。
    2. 惰性加载：如果不想在初始化时增加开销，可以使用 sync.Once 确保文件只被读取和解析一次。

4. 代码安全

• 文件读取安全：
  • 问题：osVersionFile 路径是固定的。如果该文件被恶意替换或篡改，可能会影响逻辑判断（虽然影响面有限，仅涉及升级策略）。
  • 建议：虽然 /etc/os-version 是系统关键文件，通常有权限保护，但在解析时建议增加对文件权限的简单检查，或者确保 keyfile 库在解析时不会执行危险操作（如代码注入）。
• SetP2PUpdateEnable 中的逻辑修正：
  • 审查：在 updater_ifc.go 中，SetP2PUpdateEnable 修改为 u.setPropP2PUpdateEnable(u.config.UpgradeDeliveryEnabled)。
  • 分析：这是一个非常关键且正确的修改。之前的代码直接使用用户传入的 enable 参数设置属性，忽略了 Config 中的业务逻辑限制（即非专业版强制为 false）。修改后，属性值始终与 Config 中经过校验的 UpgradeDeliveryEnabled 保持一致，防止了状态不同步或绕过限制的安全/逻辑漏洞。

5. 改进代码示例

针对上述"性能"和"代码质量"的建议，以下是优化后的代码片段示例：

config.go 修改建议：

// 在 Config 结构体中增加缓存字段
type Config struct {
    // ... 其他字段
    osVersion     string // 缓存版本名
    isPro         bool   // 缓存是否为专业版
    versionLoaded bool   // 标记是否已加载
}

// 封装读取逻辑，增加缓存
func (c *Config) checkIsProfessionalEdition() bool {
    if c.versionLoaded {
        return c.isPro
    }

    kf := keyfile.NewKeyFile()
    // 使用常量或配置中的路径
    err := kf.LoadFromFile(osVersionFile)
    if err != nil {
        logger.Warning("Failed to load os-version file:", err)
        c.isPro = false
    } else {
        editionName, err := kf.GetString("Version", "EditionName")
        if err != nil {
            logger.Warning("Failed to get EditionName:", err)
            c.isPro = false
        } else {
            c.isPro = (editionName == "Professional")
            c.osVersion = editionName
        }
    }
    
    c.versionLoaded = true
    return c.isPro
}

// 修改辅助函数，依赖 Config 实例
func upgradeDeliveryEnabledForCurrentEdition(c *Config, enable bool) bool {
    if !enable {
        return false
    }
    return c.checkIsProfessionalEdition()
}

// 在 SetUpgradeDeliveryEnabled 中使用
func (c *Config) SetUpgradeDeliveryEnabled(enable bool) error {
    // 传入 c 实例
    actualEnabled := upgradeDeliveryEnabledForCurrentEdition(c, enable)
    c.UpgradeDeliveryEnabled = actualEnabled
    return c.save(dSettingsKeyUpgradeDeliveryEnabled, actualEnabled)
}

总结：
该 diff 修复了关于专业版功能限制的逻辑漏洞，特别是在 updater_ifc.go 中的修正非常重要。主要改进点在于消除重复的文件 I/O 开销（通过缓存）以及减少对全局变量的硬编码依赖，以提高代码的健壮性和可维护性。

[deepin-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: zhaohuiw42

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