Enable fine-grained access control for full-stack signals

[Legioth]

Describe your motivation

There should be a way of only allow specific modification operations for specific signals that are returned to the client.

Use cases

The generic feature is to let server-side application logic decide which operations are allowed in a specific case for a specific user. In a poll application, there could be a NumberSignal for each option containing the number of users that have selected that option. In this case, users should only be allowed to submit operations to increment the number by 1 but not to do bigger increments or reset the signal to an arbitrary value. If the user is allowed to change their mind, then "incrementing" by -1 should also be allowed. Another way of implementing the same use case is that there's a list of the choice made by each user which means that users should only be allowed to add an entry to the list if the entry contains the user id of that user and if it's allowed to change their mind then it should only be possible to remove one's own entry.

Furthermore, it should be possible to close a poll so that no additional updates are accepted from any user. This means that it needs to be possible to take other parts of the application's overall state into account when deciding whether an operation should be accepted. It's not enough to only use information about the currently logged in user. It should also be possible to configure the access control so that the poll owner can reset the value to any value, even if the poll is closed.

A simpler case is if some signal should be read-only for some users but fully modifiable for other users. A poll application could have a ValueSignal containing an object that describes the currently active question. All participants should be able to subscribe to events when the question is changes but only the poll owner can change the question. This seems like a simpler variant of the generic functionality that could work in the same way but potentially have a shorthand API.

There's also an even more complex case to take into account in the design even if we don't implement it right away (or ever). If a bank account balance is represented by a NumberSignal, then any withdrawal should have the requirement that the withdrawn amount is larger than the current bank balance. Because of potential race conditions in a clustered setup, the access control logic cannot know what the signal value will be when the operation is applied which means that the submitted operation would have to be a "transaction" that conditionally updates the value only if the value at that time is large enough. One way of doing this is that the client must submit a proper conditional transaction and the access control logic verifies that this is the case. But it's redundant that the client even needs to know about this so it would be even better if the client could just submit regular incrementBy(-100) operation and then the server-side access control logic will inspect that operation and actually submit a corresponding conditional operation to the underlying system.

Additional requirements

• An explicitly rejected operation will lead to an exception being thrown from the .result promise of the operation. If we ever implement support for replacing operations to e.g. include some additional conditions, then this should not lead to an exception from the promise but instead resolving the promise based on the status of the actually applied operation.
• Latency compensation should remain functional even if access control is used. This means that the operations should be applied through regular client-side operations such as incrementBy so that the client can know how to show feedback to the user immediately.
• Even though the described use cases are centered around the simple number signal, the actual functionality should be implemented for all signal types.

Out of scope

• There's no granular access control for read operations. A user who is allowed to subscribe to a signal will see all operations applied for that signal and all signals "in the same event log" (i.e. both a top-level list signal and all value signals that are children of that list) which in turn means that all values stored inside the signal will be visible to all subscribers.
• It's not a goal to be able to express granular access control rules in a declarative way so that we could generate TypeScript that knows what will be allowed. But if a declarative API design is otherwise a good fit, then there's no reason why we couldn't use a declarative API.

Describe the solution you'd like

No response

Describe alternatives you've considered

No response

Additional context

No response

[peholmst]

FYI, Spring Security is now recommending a declarative approach (the @PreAuthorize annotation) that we may be able to parse and use on the client side as well: https://docs.spring.io/spring-security/reference/servlet/authorization/method-security.html

[Legioth]

Spring Security is now recommending a declarative approach (the @PreAuthorize annotation)

What would the three use cases from the ticket description look like as a string inside an annotation like @PreAuthorize?

1. Regular users can only increment by 1 and decrement by 1, owner can make arbitrary changes
2. Regular uses can only read, owner can make arbitrary changes
3. Owner can write, but only with a matching condition to ensure that any withdrawal is smaller than the current account balance

[peholmst]

The third case is actually two different things. The security check is about making sure only the owner can write. The balance check is a business rule that has nothing to do with security. I don't think we should try to shoehorn both of them into the same mechanism.

As for the first two use cases, applied on individual signal methods, they would probably be doable without any custom SpEL syntax. However, since the annotation is about to be placed on the method that returns the signal, it probably gets more complicated. Spring uses AOP to implement the security checks, and in this case, the security check would be applied to the call to the method that returns the signal, not the signal itself.

We may have to introduce our own annotation, or multiple annotations, that mimic @PreAuthorize. Maybe something like this for the first use case:

@AuthorizeSignalWrite("hasPermission('signal:owner') || (hasPermission('signal:increment') && #incrementBy == 1)")
@AuthorizeSignalRead("hasPermission('signal:read')")
public NumberSignal mySignal() {...}

And something like this for the second use case:

@AuthorizeSignalWrite("hasPermission('signal:owner')")
@AuthorizeSignalRead("hasPermission('signal:read')")
public NumberSignal mySignal() {...}

[Legioth]

I guess the IDE wouldn't offer the developer much help in correctly writing hasPermission('signal:owner') || (hasPermission('signal:increment') && #incrementBy == 1) since it's just a string from the IDE's point of view?

The security check is about making sure only the owner can write. The balance check is a business rule that has nothing to do with security.

Couldn't you also argue that the limitation on allowable increment deltas for the poll is also a business rule rather than a security rule?

[peholmst]

I hope modern IDE:s with Spring support also offer help for SpEL. Personally, I prefer writing Java code to scripting in strings like that since you don't notice if you've made a mistake until runtime (if even then).

Couldn't you also argue that the limitation on allowable increment deltas for the poll is also a business rule rather than a security rule?

Yes, you could.

[Legioth]

Basic SeEL could be supported, but is it supported in a custom annotation like @AuthorizeSignalWrite? And how about signal-specific names inside that string, i.e. signal:increment and #incrementBy? Could it support that as well without a custom IDE plugin?

[peholmst]

I don't know. We have to investigate that.

[cromoteca]

My proposal is to allow to extend signals. Suppose that NumberSignal provides 3 operations: update, replace, and incrementBy, and that its corresponding TS provides latency compensation for those operations. Then, there would be 2 ways of implementing a VoteSignal:

@RolesAllowed(Role.ADMIN) // to restrict "update" and "replace"
public class VoteSignal extends NumberSignal {
  /**
   * Restricts the vote amount to 1 or -1
   */
  @AnonymousAllowed
  @Override
  public void incrementBy(double amount) {
    return super.incrementBy(Math.signum(amount));
  }
}

In this case, you still get the default latency compensation from the superclass on the client, which works fine as long as the voting amount is correct.

@RolesAllowed(Role.ADMIN) // to restrict "update", "replace", and "incrementBy"
public class VoteSignal extends NumberSignal {
  /**
   * Express the business action of voting
   */
  @AnonymousAllowed
  public void vote(double amount) {
    return incrementBy(1);
  }
  // ... same for "unvote"
}

This one expresses the intention more clearly, but it needs to allow the developer to define a latency compensation action for the generated vote and unvote functions in TypeScript, otherwise the UI will only be updated after server confirmation.

[Legioth]

One aspect that isn't directly supported when overriding methods is to intercept transactions in a way that allows you to review all the operations in the transaction as a whole. We can still solve that by also providing a more low-level transaction listener for that purpose while keeping signal subclasses as the main API to use for all of the most common cases.

[Legioth]

A potential future problem with overriding those methods is that there will eventually be a non-trivial return type from each operation rather than only void. That's fine if you either throw an exception or call a compatible super method but I'm not sure if there would also be cases where you couldn't have an easy way of creating the value that you're supposed to return in the hypothetical special cases when you apply a different operation than the original one.

[Legioth]

What should the TS type be for a service method that returns a VoteSignal?

• If it's NumberSignal, then it's potentially confusing that it's different than the Java type.
• if it's a generated VoteSignal type, then it's potentially confusing if it doesn't also contain any new public methods that I have added to the Java subclass.

[cromoteca]

That would definitely need some improvements to the generator, as I do expect a VoteSignal to be generated, along with all public methods I added and hooks to provide custom TS for latency compensation on those.

[cromoteca]

A potential future problem with overriding those methods is that there will eventually be a non-trivial return type from each operation rather than only void. That's fine if you either throw an exception or call a compatible super method but I'm not sure if there would also be cases where you couldn't have an easy way of creating the value that you're supposed to return in the hypothetical special cases when you apply a different operation than the original one.

Given that, in the long run, we can't afford to send the whole updated value each time and we rather need an operation log to apply them incrementally, all things a method can do must go down to a sequence of those basic well-known operations. So, a method would be a simple logical wrapper around a sequence of one or more additions to the log.

[Legioth]

There's one thing we're forgetting: signals can be nested. As a reminder ListSignal<T> corresponds to Signal<List<Signal<T>>>. I'm not sure how subclasses could be used when it's the inner signal type that has some restrictions.

As a simple made-up example, let's say that we have a ListSignal<String> and want to enforce that all the strings are at least 3 characters long.

[peholmst]

For validation, I think there are (at least) two alternatives here:

1. Validation is handled by combining signals. For instance, the raw input ListSignal<String> can be mapped to another signal whose value is the validation state.
2. Validation is a feature that can be built into every signal, with all the added complexity.

[Legioth]

I propose alternative 3: you can wrap a signal instance to create a new signal with the same type but also using an interceptor callback that will be called for all operations to that signal or any of its child signals.

In practice, combining signals does also mean that you always need to have one "write signal" and then somehow copy the values over to a "read signal" with the validated values. This copying is probably more application code than an interceptor and it does also have the drawback that you lose latency compensation unless there's also an API for combining both signals into one. But that combined signal is basically the same as the interceptor wrapper that I'm suggesting.

[cromoteca]

As a simple made-up example, let's say that we have a ListSignal<String> and want to enforce that all the strings are at least 3 characters long.

ListSignal wraps the value in a ValueSignal, so this case could be solved by allowing the ListSignal to customize the item signal type. If I define a NotTooShortStringSignal extends ValueSignal<String>, I should then be able to customize the wrapper and instantiate it as new ListSignal<String>(NotTooShortStringSignal.class) instead of new ListSignal<String>(String.class).

Although I'm not sure that Java generics are good enough to allow this.

[Legioth]

Can't overload the existing ListSignal(Class itemType) constructor but I think the generic type could be handled with a static factory method like this
public static <T, S extends ValueSignal<T>> ListSignal<T> withChildSignalType(Class<S> childSignalType)

But there will be another level of complexity with this approach when we get to TreeSignal since the whole point there is that different child signals can have different types.

[peholmst]

I don't warm up to the idea that you have to extend a signal class in order to do any of this, including adding annotations to it. I would prefer an approach where I can do everything I need by composition.

[Legioth]

One option with composition rather than subclassing could look like this for the max string length option.

ListSignal<String> unrestrictedStrings = new ListSignal<>(String.class);

ListSignal<String> restrictedStrings = unrestrictedStrings.withValueValidator(value -> value.length() >= 3);

With this approach, restrictions would be applied for all client to which restrictedStrings is returned whereas clients to which unrestrictedStrings is returned wouldn't have any restrictions.

withValueValidator is a shorthand for wrapping the signal instance with an interceptor that is run for all incoming change operations through that instance (or any of its child signals). Directly implementing an interceptor is somewhat tedious since the implementation needs to take into account all possible operations that any type of signal may receive (since that's how the upcoming tree signal type works).

The same example with a low-level interceptor could look like this:

ListSignal<String> restrictedStrings = unrestrictedStrings.withInterceptor(operation -> {
  if (operation instanceof ValueOperation valueOp) {
    // ValueOperation covers all operations that carry a value, e.g. insert (for list signals), set (for value signals), and put (for map signals) as well as various conditional variants of those.
    // Should actually also check for tree signal operations like inserting into a child but not doing that here for brevity.
    String value = valueOp.getValue(String.class);
    if (value == null || value.length() < 3) { 
      return SignalOperation.deny();
    }
  }

  // Accept all other operations, e.g. removing or re-ordering entires
  return operation;
});

One benefit of this model is that also applies to usage directly through the signal instance's API and not only to Hilla clients. This means that a service could return signal with an interceptor to e.g. some Flow UI logic and this would apply exactly the same restrictions that would be used for operations from a Hilla client.

[peholmst]

That looks better!

[cromoteca]

Composition is proving to be a better programming model than inheritance, at least that's where most modern programming languages and libraries are going. But, even with composition, it should be possible to communicate an intent, like "vote", or "withdraw". That restricted string could be an username, for example.

[Legioth]

Communicating intent is a different feature than restricting which values / operations are allowed. The inheritance-based approach focused on the intent that was suggested earlier did still also need annotations to define restrictions.

[platosha]

Would it be a good constraint to only support predicates at this point?

ListSignal<String> readonlyStrings = unrestrictedStrings.withOperationValidator(operation -> false);

[Legioth]

A validator returning boolean should indeed be sufficient for most reasonable cases. Might be some cases where it would be more convenient to inject e.g. transaction restrictions from the server instead of adding them on the client and then also separately validating them on the server but we can add an API for that later on if we needed.

We would need these operation types for the initial implementation based on the available operations on the currently implemented signal types.

• SetValueOperation (not named SetOperation since that might be mistaken for an operation on a set)
• ReplaceValueOperation (note that update on the client is actually one or several replace operations)
• IncrementOperation
• ListInsertOperation
• ListRemoveOperation

Note that the ValueOperation that was suggested in a code snippet above is not a fundamental operation type but instead an interface implemented by all operations that carry a value, i.e. SetValueOperation, ReplaceValueOperation and ListInsertOperation. This also implies that all those operations should use the same method name to get the new value.

The initial implementation can assume that a legitimate client only sends operations that will be accepted so that the server-side validator is there only to protect against tampering. This means that the server can silently ignore operations that do not pass the validator. We will make sure the client remains in sync with the server even after sending an invalid operation only after we have introduced client-side latency compensation based on a stack of unconfirmed operations.

Each withOperationValidator invocation creates a new instance that wraps the original signal instance. The original signal instance should remain without the validator. Value changes applied to the new instance should be applied to the original instance (if they pass the validator). Multiple levels of wrappers should go through each validator in order until getting to the original signal instance. Subscribing to changes from the new instance should subscribe to changes to the original instance.

[taefi]

Apart from introducing new methods for the above mentioned operations, would it be OK if we just provide an extra asReadonly() method for the signals that adds all the necessary rejection validators at once?

[Legioth]

If we're going to have any shorthand API on top of the low-level feature, then asReadonly() is probably the most impactful one to have.