feat: allow permission checks for signals

[drodil]

Hey, I just made a Pull Request!

This allows the signal payload to have permissions check included for things that require it.

✔️ Checklist

• A changeset describing the change and affected packages. (more info)
• Added or updated documentation
• Tests for new functionality and regression tests for bug fixes
• Screenshots attached (for UI changes)
• All your commits have a Signed-off-by line in the message. (more info)

[backstage-goalie]

Changed Packages

Package Name	Package Path	Changeset Bump	Current Version
example-backend	packages/backend	none	v0.2.98-next.0
@backstage/plugin-signals-backend	plugins/signals-backend	patch	v0.1.4-next.0
@backstage/plugin-signals-node	plugins/signals-node	patch	v0.1.4-next.0

[github-actions]

Uffizzi Ephemeral Environment - Virtual Cluster

Your cluster pr-23171 was successfully created. Learn more about Uffizzi virtual clusters
To connect to this cluster, follow these steps:

1. Download and install the Uffizzi CLI from https://docs.uffizzi.com/install
2. Login to Uffizzi, then select the backstage account and project:

uffizzi login

Select an account: 
  ‣ backstage
    jdoe

Select a project or create a new project: 
  ‣ backstage-6783521

3. Update your kubeconfig: uffizzi cluster update-kubeconfig pr-23171 --kubeconfig=[PATH_TO_KUBECONFIG]
   After updating your kubeconfig, you can manage your cluster with kubectl, kustomize, helm, and other tools that use kubeconfig files: kubectl get namespace --kubeconfig [PATH_TO_KUBECONFIG]

Access the backstage endpoint at https://backstage-default-pr-23171-c5294.uclusters.app.uffizzi.com

[vinzscam]

shouldn't this check be done in the plugin sending the event instead? 🤔
What's the advantage of passing the plain permissions, performing the check at signals-backend level?

[drodil]

This is not possible as the signals can have many receivers. Of course, if you send the signal to only the current user, the permission check can be done in the sender end. An example for this:

1. DevTools wants to broadcast updates to info data (CPU/mem usage etc.) for all users at the devtools page
2. To see the info in the first place, the user must have devToolsInfoReadPermission
3. devtools-backend cannot see which users have subscribed to the signals
4. Thus the permission must be checked during sending and not in the devtools-backend

[vinzscam]

This is not possible as the signals can have many receivers

ah I see, thank you for clarifying!
How does the subscription flow work? For instance, as a Backstage user, how can I subscribe to updates for the devtools' plugin?

[drodil]

Maybe this 5-minute drawing is the easiest way to explain the functionality of the signals. Does it make sense?

[drodil]

Or maybe even better, here's PR for devtools that explains it in code: #23194 :)

[vinzscam]

thank you so much. It makes sense.

Do you have any suggestions about how to potentially deal with an expired client token here?

[drodil]

It's a bit of a hard thing to do. One option would be to send the token via websocket channel instead in headers and refresh it on the client side when needed. I have to think about this a bit but I think it's in another PR. If you guys have ideas, they are more than welcome 🙏

[Rugvip]

Briefly mentioned in #23194, but I wonder if it's better to authorize permissions when subscribing to a signal channel? Just to avoid hammering the permissions backend for small frequent signals. That could also come with the added benefit that an up-to-date user token can be included in that request, fixing the token expiration problem.

Since there's no concept of persistent channels with metadata or anything like that, the permission would simply have to be matched against permissions required when sending a signal later on. i.e. if a user subscribes to channel example.a with permission example.read, then the signal backend would allow signals through on that channel that either do not require any permission, or require example.read, signals with any other permission would be rejected.

I think that flow might have the additional benefit that we can set up a request/response flow for the signal subscription where the frontend can get feedback on whether the user has permission or not. Ofc you'd usually want to check that upfront in the frontend with a separate check, but with this the failure mode can be a bit better than just complete silence, even if we just forward permissions errors to the ErrorApi.

[drodil]

The problem with this is exactly with the channels as the signal manager does not know which channel would require which kind of permission. IMHO I don't think it's a good idea to mix the channel name with permissions as it:

1. Gets really confusing to the signal sender as they have to match the permission name and the signal channel. This can lead to errors especially when refactoring things
2. Doesn't allow resource permission checks. For example, some plugins might want to send information in the same channel about different resources and with this architecture, it's not possible

For the performance issue you mentioned in the other PR, I think this should be taken care of in the PermissionService instead. Checking the same access rights multiple times for the same token should be very fast as it happens also in other places in the system. Another option would be to cache the authorization results per connection but I think that's not the right way to go either as it only applies to signals.

Personally, I would like to keep this as light as possible in the frontend side, you can subscribe to anything but there's no guarantee that you will ever get data from that channel.

[github-actions]

This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution!

[drodil]

Not stale, needs a rebase

[github-actions]

This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution!

[freben]

Hi! Sorry but there's a conflict now.

[drodil]

Hey, will check once back at the hotel

[github-actions]

This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution!

[drodil]

Not stale

[github-actions]

This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution!

[drodil]

Not stale

[github-actions]

This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution!

[freben]

I'll put my tentative approval on here. With the caveats that:

• Indeed token expiration must be handled. Maybe re-send the credentials over the channel, or simply re-establish. The latter sounds simpler, it's not like it happens all that often.
• Likewise, the new userInfo service should be used for getting ownership refs, and you want to re-fetch those periodically. If you re-establishment of the connection as per above, then that happens over and over by design, but otherwise you'll want a slow loop to update it over time
• I don't have a strong opinion on where and when the permissions are checked. I'll leave that to your discretion to choose. If it can be done early, then @Rugvip is right that that feels more robust and clear, and afaik matches how subscriptions work in pubsub systems out there etc. But it's also to some degree a "UX optimization issue" and can be changed to be more strict in the future, right? Even though it's technically a breaking change

But maybe those can all be iterated on separately.

This also needs a rebase. And it would be good to have a few 👀 from @vinzscam and @Rugvip once more

[drodil]

Rebased

[vinzscam]

I feel like bringing authentication support to signals brought some technical issues that it would be nice to address. As pointed out it would be nice to check the permissions at subscription time. If the current implementation of signals doesn’t allow us to do this, perhaps something can be improved in the design of the current signal solution? Shall we have a design session in order to see if it is possible to revisit and improve the overall design of signals/events?

[drodil]

@vinzscam the problem with subscription time checks is that we don't know the topics sent through signals at that time. This would require the backend plugins to register their topics before sending signals to them with necessary access rights. Of course, this is possible but would mean some extra steps for the signal sender.

[drodil]

@vinzscam I added a new way to register channels to the Signals that can include the permissions needed to subscribe to that channel. This way we can handle the checks during subscription phase for signals that need permissions. Adds a bit more complexity but I think there's not so many cases we need the permissions, maybe :)

[freben]

Sorry but there are conflicts. Maybe after the release tomorrow there'll be some more - could rebase after that perhaps

[drodil]

Yeah, I'll rebase after the release!

[drodil]

Hmm, after thinking about this, it will not work if there are multiple nodes running the backend and some of them are restarted individually as it will lose all channel registrations. One option would be to have a database for the channels but that would require a bit more changes and I am not sure if that's a better option either.

The first version of this where the permissions were sent as part of the signal didn't have this kind of issue but of course, it requires a bit more runtime resources as the permissions need to be checked for each signal individually. Have to think what would be the best course for this.

[github-actions]

This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution!

[drodil]

Not stale