Skip to content

Conversation

@thoraj
Copy link
Contributor

@thoraj thoraj commented Nov 6, 2025

Summary

Based on the documentation for EnableAutoSave(), which states it "controls whether to save a policy rule automatically to the adapter when it is added or removed," we believe AutoSave should apply uniformly to all policy operations, including both regular policies and grouping policies.

However, we discovered that grouping policy operations were not respecting the AutoSave flag. This PR addresses that inconsistency.

Note: This issue was discovered while working on adding support for multiple CasbinDbContext instances in the EFCore adapter (see casbin-net/EFCore-Adapter#125), where proper AutoSave behavior is essential for correct adapter operation.

Problem

The EnableAutoSave(false) setting was not being respected for RBAC/grouping policy operations:

  • Regular policy operations worked correctly: AddPolicy(), RemovePolicy(), UpdatePolicy() did not save to adapter when AutoSave was disabled
  • Grouping policy operations did not respect the flag: AddGroupingPolicy(), RemoveGroupingPolicy(), UpdateGroupingPolicy() still saved to adapter even when AutoSave was disabled

Root Cause

The SetAutoSave() method in Casbin/Extensions/Model/ModelExtension.cs only iterated through policy assertions (section "p") to set the AutoSave flag. It never set the flag for role assertions (section "g"), even though RoleAssertion has its own PolicyManager with an AutoSave property.

Solution

Modified SetAutoSave() to configure the AutoSave flag for both policy and role/grouping assertions:

public static void SetAutoSave(this IModel model, bool autoSave)
{
    // Set AutoSave for policy assertions (section "p")
    foreach (KeyValuePair<string, PolicyAssertion> pair in model.Sections.GetPolicyAssertions())
    {
        pair.Value.PolicyManager.AutoSave = autoSave;
    }

    // Set AutoSave for role/grouping assertions (section "g") if they exist
    if (model.Sections.ContainsSection(PermConstants.Section.RoleSection))
    {
        foreach (KeyValuePair<string, RoleAssertion> pair in model.Sections.GetRoleAssertions())
        {
            pair.Value.PolicyManager.AutoSave = autoSave;
        }
    }
}

The fix includes a safety check (ContainsSection) to handle models without role definitions.

Changes

Modified Files

  1. Casbin/Extensions/Model/ModelExtension.cs

    • Updated SetAutoSave() method to configure AutoSave for both policy and role assertions
    • Added safety check for models without role definitions
  2. Casbin.UnitTests/ModelTests/EnforcerTest.cs

    • Added TestAutoSaveGroupingPolicy() - Verifies synchronous grouping policy operations respect AutoSave
    • Added TestAutoSaveGroupingPolicyAsync() - Verifies asynchronous grouping policy operations respect AutoSave
  3. Casbin.UnitTests/Mock/MockSingleAdapter.cs (new file)

    • Created test helper that implements ISingleAdapter
    • Tracks save operations to verify AutoSave behavior
    • Compatible with .NET 4.5.2 through .NET 9.0

Testing

  • ✅ New tests fail before the fix (confirming the issue existed)
  • ✅ New tests pass after the fix (confirming the fix works)
  • All 363 existing unit tests pass with no regressions
  • ✅ Full solution builds cleanly across all target frameworks

Impact

Affected Methods (Now Fixed)

All grouping policy methods now properly respect the AutoSave flag:

  • AddGroupingPolicy() / AddGroupingPolicyAsync()
  • AddGroupingPolicies() / AddGroupingPoliciesAsync()
  • RemoveGroupingPolicy() / RemoveGroupingPolicyAsync()
  • RemoveGroupingPolicies() / RemoveGroupingPoliciesAsync()
  • UpdateGroupingPolicy() / UpdateGroupingPolicyAsync()
  • UpdateGroupingPolicies() / UpdateGroupingPoliciesAsync()
  • RemoveFilteredGroupingPolicy() / RemoveFilteredGroupingPolicyAsync()

Backward Compatibility

  • Fully backward compatible
  • Models without role definitions continue to work correctly (safety check added)
  • No API changes
  • No behavioral changes for code that already worked correctly

We believe this fix aligns with the documented behavior and design intent of the AutoSave feature. However, if there was an intentional reason for the previous behavior, we're happy to discuss alternative approaches.

The EnableAutoSave(false) setting was not being respected for RBAC/grouping
policy operations. When AutoSave was disabled, policy operations (AddPolicy,
RemovePolicy, UpdatePolicy) correctly skipped adapter saving, but grouping
policy operations (AddGroupingPolicy, RemoveGroupingPolicy, etc.) still
saved to the adapter.

Root Cause:
The SetAutoSave() method only iterated through policy assertions (section "p")
to set the AutoSave flag. It never set the flag for role assertions (section "g"),
even though RoleAssertion has its own PolicyManager with an AutoSave property.

Solution:
Modified SetAutoSave() to also configure the AutoSave flag for role/grouping
assertions, with a safety check for models without role definitions.

Changes:
- Casbin/Extensions/Model/ModelExtension.cs: Updated SetAutoSave() to handle
  both policy and role assertions
- Casbin.UnitTests/ModelTests/EnforcerTest.cs: Added comprehensive tests for
  sync and async grouping policy operations
- Casbin.UnitTests/Mock/MockSingleAdapter.cs: Created test helper to track
  adapter save operations

All 363 unit tests pass with no regressions.
@CLAassistant
Copy link

CLAassistant commented Nov 6, 2025

CLA assistant check
All committers have signed the CLA.

@hsluoyz hsluoyz requested a review from Copilot November 6, 2025 14:51
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a bug where the EnableAutoSave() method was not applying the AutoSave setting to role/grouping policy assertions (section "g"), causing grouping policy changes to still trigger adapter saves even when AutoSave was disabled.

  • Extended SetAutoSave() to include role/grouping assertions in addition to policy assertions
  • Added two new tests to verify AutoSave behavior for grouping policies (sync and async)
  • Created MockSingleAdapter to facilitate testing of AutoSave behavior

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
Casbin/Extensions/Model/ModelExtension.cs Added logic to set AutoSave on role assertion PolicyManagers when role section exists
Casbin.UnitTests/ModelTests/EnforcerTest.cs Added tests TestAutoSaveGroupingPolicy() and TestAutoSaveGroupingPolicyAsync() to verify the fix
Casbin.UnitTests/Mock/MockSingleAdapter.cs New mock adapter that tracks individual policy operations to test AutoSave behavior

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@thoraj thoraj marked this pull request as ready for review November 6, 2025 14:55
@hsluoyz hsluoyz changed the title fix: respect AutoSave flag for grouping policy operations feat: respect AutoSave flag for grouping policy operations Nov 7, 2025
@hsluoyz hsluoyz merged commit e8ba47c into casbin:master Nov 7, 2025
14 checks passed
@github-actions
Copy link

github-actions bot commented Nov 7, 2025

🎉 This PR is included in version 2.19.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants