Skip to content

feat: pass the requested access to access control strategies - #1160

Merged
vbreuss merged 4 commits into
mainfrom
feat/access-control-requested-access
Oct 30, 2026
Merged

vbreuss merged 4 commits into
mainfrom
feat/access-control-requested-access

Conversation

@vbreuss

@vbreuss vbreuss commented Oct 28, 2026 •

Copy link
Copy Markdown
Member

An IAccessControlStrategy only receives the path and the IFileSystemExtensibility, so it can only grant or deny all access to a file or directory. Tests cannot simulate the common cases in between: a file that can be read but not written, or a directory whose files can be written but not deleted.

This adds an AccessRequest that the mock passes to the strategy:

  • Access is the requested FileAccess. Enumerating a directory requests FileAccess.Read.
  • IsDelete is true when the file or directory is deleted, because a plain FileAccess cannot tell a delete from a write. A delete also requests FileAccess.Write.
fileSystem.WithAccessControlStrategy(
    new DefaultAccessControlStrategy((path, _, request)
        => !(path.EndsWith("readonly.txt") && request.Access.HasFlag(FileAccess.Write))));

API changes

  • New abstract base class AccessControlStrategy that implements IAccessControlStrategy and adds IsAccessGranted(string, IFileSystemExtensibility, AccessRequest). A base class, rather than a second interface, can gain virtual members later without breaking derived strategies.
  • New AccessRequest with an internal constructor: only the mock creates requests, so the type can gain properties later without breaking callers or piling up constructor overloads.
  • DefaultAccessControlStrategy and NullAccessControlStrategy now derive from AccessControlStrategy. DefaultAccessControlStrategy gets a constructor overload for a callback that receives the AccessRequest.
  • The API snapshots under Tests/Api/ are updated for every target framework.

Compatibility

  • Existing IAccessControlStrategy implementations keep working: WithAccessControlStrategy wraps them, and they still receive only path and extensibility.
  • The two-parameter IsAccessGranted on AccessControlStrategy requests FileAccess.ReadWrite, so calling it never grants access by accident.
  • Existing DefaultAccessControlStrategy((path, extensibility) => ...) calls still compile; the lambda's parameter count selects the overload.

@vbreuss vbreuss self-assigned this Oct 28, 2026
Copilot AI balanced review requested due to automatic review settings October 28, 2026 20:07

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

@vbreuss
vbreuss enabled auto-merge (squash) October 28, 2026 20:09
@github-actions

github-actions Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

Test Results

     62 files       62 suites   1h 18m 0s ⏱️
 66 189 tests  62 677 ✅  3 512 💤 0 ❌
151 377 runs  136 607 ✅ 14 770 💤 0 ❌

Results for commit 58dc265.

♻️ This comment has been updated with latest results.

@vbreuss
vbreuss marked this pull request as draft October 29, 2026 05:13
auto-merge was automatically disabled October 29, 2026 05:13

Pull request was converted to draft

vbreuss and others added 4 commits October 30, 2026 00:27
…ted `FileAccess`

The new abstract `AccessControlStrategy` implements `IAccessControlStrategy` and adds an `IsAccessGranted` overload with the requested `FileAccess`, so tests can simulate files and directories that can be read but not written. Enumerating a directory requests `FileAccess.Read`. As a base class, it can be extended with virtual members in the future without breaking derived strategies.

`DefaultAccessControlStrategy` and `NullAccessControlStrategy` now derive from it, and `DefaultAccessControlStrategy` gets a constructor overload for a callback with the requested access. Existing `IAccessControlStrategy` implementations keep working unchanged.
Users could not discover the new overload without docs, and the test now shows the recommended HasFlag check. Also restores the original BOM state of the test file.
A plain FileAccess cannot tell a delete from a write, so strategies could not simulate files that may be written but not deleted. A request object can carry more information later without breaking derived strategies, and the API is not released yet, so this is the cheapest moment to change it.

The test strategy now implements the interface explicitly to avoid a false-positive unused-parameter warning.
Only the mock creates access requests, so an internal constructor lets the type gain properties later without breaking callers or accumulating constructor overloads.
@vbreuss
vbreuss force-pushed the feat/access-control-requested-access branch from 269a05c to 58dc265 Compare October 29, 2026 23:28
@vbreuss
vbreuss marked this pull request as ready for review October 29, 2026 23:28
@sonarqubecloud

Copy link
Copy Markdown

@mergify

mergify Bot commented Oct 30, 2026 •

Copy link
Copy Markdown

This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again.

@vbreuss vbreuss changed the title feat: add AccessControlStrategy base class that receives the requested FileAccess feat: pass the requested access to access control strategies Oct 30, 2026
@vbreuss
vbreuss merged commit 99e8908 into main Oct 30, 2026
15 checks passed
@vbreuss
vbreuss deleted the feat/access-control-requested-access branch October 30, 2026 00:47
@vbreuss vbreuss linked an issue Oct 30, 2026 that may be closed by this pull request
@github-actions

Copy link
Copy Markdown

This is addressed in release v7.2.0.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

How to detect the context in DefaultAccessControlStrategy()

2 participants