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

Constrain available operations #1560

Closed
bkoelman opened this issue Jun 18, 2024 · 0 comments · Fixed by #1561
Closed

Constrain available operations #1560

bkoelman opened this issue Jun 18, 2024 · 0 comments · Fixed by #1561

Comments

@bkoelman
Copy link
Member

bkoelman commented Jun 18, 2024

DESCRIPTION

The current behavior is that, by default, all operations on all resource types are exposed. To reduce this, we've advised users to write custom filter logic by overriding the operations action method. See here for an example.

While this works, it's unfortunate that JADNC can't "see" which operations are exposed. OpenAPI generation should be able to take this into account.

Furthermore, it's an odd default to always expose everything, even when a resource defines a subset of endpoints. For example:

[Resource(GenerateControllerEndpoints = JsonApiEndpoints.Post | JsonApiEndpoints.Patch)]
public class Article : Identifiable<long>
{
}

It would make more sense to apply the subset from GenerateControllerEndpoints by default, to determine which operations are exposed. But it must be possible to override that, because custom controllers or custom action methods may exist, which are excluded from controller auto-generation.

PROPOSED SOLUTION

Provide an extensibility point to indicate which operations are exposed. This enables OpenAPI generation to take this into account when producing schemas. The default implementation would look at GenerateControllerEndpoints. JADNC should call this extensibility point from the base operations controller action method, for each operation in the request. And produce an error with the list of operations in the request that aren't accessible.

public interface IAtomicOperationFilter
{
    bool IsEnabled(ResourceType resourceType, WriteOperationKind writeOperation);
}

IMPACT

This is a breaking change, which improves security. GenerateControllerEndpoints defaults to All, so users that have never set this attribute won't be affected. Users that did, but weren't aware they should have written custom logic, will now silently benefit from a more consistent REST API surface. Any custom logic that users already wrote to reduce the surface will remain intact. But users that don't use auto-generated controllers (GenerateControllerEndpoints set to None, or no [Resource] attribute) and have added an operations controller will now need to implement the interface. Failing to do so will block all operations on explicit (non-auto-generated) controllers. The good thing is that updating will result in a compile error, because BaseJsonApiOperationsController gets an extra constructor parameter for the interface above. This should trigger users to investigate what happened and adapt accordingly. This breaking change needs to be mentioned in the documentation and the release notes.

To simplify reverting to the old behavior, we can provide a static property on the interface:

public interface IAtomicOperationFilter
{
    public static IAtomicOperationFilter AlwaysEnabled { get; } = new AlwaysEnabledOperationFilter(); // always returns true
}

So that users can pass it for the newly added constructor parameter:

public sealed class OperationsController(
    IJsonApiOptions options, IResourceGraph resourceGraph, ILoggerFactory loggerFactory,
    IOperationsProcessor processor, IJsonApiRequest request, ITargetedFields targetedFields)
    : JsonApiOperationsController(options, resourceGraph, loggerFactory, processor, request, targetedFields,
    /* --> */ IAtomicOperationFilter.AlwaysEnabled /* <-- */);
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

1 participant