Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AccessControlProvider does not receive correct entity #171

Closed
ryxxxx opened this issue Dec 22, 2024 · 1 comment · Fixed by #179
Closed

AccessControlProvider does not receive correct entity #171

ryxxxx opened this issue Dec 22, 2024 · 1 comment · Fixed by #179
Labels
Server Improvements or additions to the server code
Milestone

Comments

@ryxxxx
Copy link

ryxxxx commented Dec 22, 2024

Describe the bug

The entity that is passed to the AccessControlProvider does not have any properties that are only stored server-side (such as a UserId), when it is passed from a PUT request. Accordingly, all PUT-requests comparing the UserId of the entity to the user making the request will return "Unauthorized". DELETE requests function as expected. The difference can be explained by examining the source (I only included the relevant lines):

TableController.Update.cs:

public virtual async Task<IActionResult> ReplaceAsync([FromRoute] string id, CancellationToken cancellationToken = default)
{
    TEntity entity = await DeserializeJsonContent(cancellationToken).ConfigureAwait(false);

    TEntity existing = await Repository.ReadAsync(id, cancellationToken).ConfigureAwait(false);

    if (!AccessControlProvider.EntityIsInView(existing))
    {
        Logger.LogWarning("ReplaceAsync: {id} statusCode=404 not in view", id);
        throw new HttpException(StatusCodes.Status404NotFound);
    }

    await AuthorizeRequestAsync(TableOperation.Update, entity, cancellationToken).ConfigureAwait(false);
}

TableController.Delete.cs:

public virtual async Task<IActionResult> DeleteAsync([FromRoute] string id, CancellationToken cancellationToken = default)
{
    TEntity entity = await Repository.ReadAsync(id, cancellationToken).ConfigureAwait(false);

    if (!AccessControlProvider.EntityIsInView(entity))
    {
        Logger.LogWarning("DeleteAsync: {id} statusCode=404 not in view", id);
        throw new HttpException(StatusCodes.Status404NotFound);
    }

    await AuthorizeRequestAsync(TableOperation.Delete, entity, cancellationToken).ConfigureAwait(false);
}

In Delete.cs "AuthorizeRequestAsync" is called with the entity retrieved from "Repository.ReadAsync", whereas in Update.cs it is called with the entity retrieved from "DeserializeJsonContent". I doubt that this is intentional, because the other AccessControlProvider function call to "EntityIsInView" is made with the correct entity (named "existing").

If I am correct, the fix would be very simple: replace "AuthorizeRequestAsync(TableOperation.Update, entity" with "AuthorizeRequestAsync(TableOperation.Update, existing".

To Reproduce

Steps to reproduce the behavior:

  1. Add a simple AccessControlProvider as detailed in the documentation, that uses a UsedId property to restrict editing access to entities that were created by the user.

Expected behavior

The entity passed to the AccessControlProvider should have all fields that are contained in the database and not just those that were passed with the PUT request.

What platforms?

Note: Any bug or feature request that is opened for an unsupported environment will be automatically closed.

Server:
Version of dotnet being used to compile? net 8.0
Library versions? 8.0.4
What database are you using? sqlite for testing purposes
Where are you running the server? Azure App Service

Additional context

This problem might be related to #162, but I'm not sure

@ryxxxx ryxxxx added Bug Requires Triage This issue has not been checked by the project team. labels Dec 22, 2024
@adrianhall adrianhall added Server Improvements or additions to the server code and removed Requires Triage This issue has not been checked by the project team. labels Dec 28, 2024
@adrianhall adrianhall added this to the 8.0.5 milestone Dec 28, 2024
adrianhall added a commit to adrianhall/CommunityToolkit-Datasync that referenced this issue Jan 6, 2025
@adrianhall
Copy link
Collaborator

  • Updated the AuthorizeRequestAsync() call to use the existing entity instead (BREAKING CHANGE)
  • Added a new "authorized" endpoint to the test server so we can test access control providers.
  • Added a new test to the replace tests against the test server to ensure the right entity has been added.

Ideally, I'd add similar tests for the other operations, but going with the minimal approach right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Server Improvements or additions to the server code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants