Support for Scalelite Tagged Servers

Ithanil commented 3 months ago

This PR adds frontend support for "Tagged Servers" in Scalelite, a feature which is described here: https://github.com/blindsidenetworks/scalelite/pull/1049

Description

This PR allows user to configure two new options for each room: 1) The server tags from a pre-defined list 2) Whether the the server tag is hard required or fallback is allowed when no server is available

In terms of UI, it presents itself as seen in the following screenshots: Screenshot from 2024-04-29 15-48-45

The list of available options is defined by the Greenlight administrator and certain tags can be restricted to certain role ids. In the current iteration of this feature, this is done via .env config file and is documented in sample.env:

## Support for Tagged Servers
# If your Greenlight instance is connected to Scalelite or another Loadbalancer with enabled support for the 'meta_server-tag'
# parameter on create calls, you can use the following variables to configure support for this feature via the Greenlight UI.
# When this configuration is changed later, disallowed tags can be removed from the DB via `bundle exec rake server_tags_sync`
# Example configuration (delimiters are , : and /):
# SERVER_TAG_NAMES=tag1:Name 1,tag2:Name2  # defines available tags and their friendly names
# SERVER_TAG_ROLES=tag2:xyz-123-321-aaaa-zyx/abc-321-123-zzzz-cba  # allow tag only for given role ids (see role ids in DB)

The required database migration is included and well tested. No RSpec tests have been added, but the feature is tested in deployment for multiple rooms, configuration combinations etc.

Notes

Currently, if a room has been configured with a tag and then later the tag is removed from the site-wide configuration or permission is revoked for the user's role, the room's configured tag will remain in the database and will still be used for create calls. However, the configuration field will show as empty in the UI and only allow changes to valid/allowed tags. Solved by commit 29c0c4935de48f7ad8bc7e454d081072b85d50ac and further improved by 2928003293411faf1de74af77d316759fa0c0efd
Currently, the role-based restriction of tags is only done by sending a list of allowed tags to the JavaScript code and not again validated on the backend side. This seems OK as long as the impact of potential malicious users abusing known "VIP" tags is tolerable. Solved by commits 2928003293411faf1de74af77d316759fa0c0efd & ffcd97280a5232593eb33cb73fd5a0245e025fb4, invalid/forbidden tags will be dropped by the meeting_starter and they show as default type / untagged in the UI.
The two server tag options have already been added as global room configuration options (for consistency and for not requiring any future migrations), but are neither available for configuration in the admin UI nor does their value override the individual room's configuration.

What is "missing" / Future Development

~~Add a sync task for the administrator to remove invalid tags from rooms after configuration file change~~ Done (29c0c4935de48f7ad8bc7e454d081072b85d50ac)
~~Validate role-based tag restriction on backend side (I could use a hint on how and at which point this should be done ideally, would also be happy if someone else could contribute this)~~ Done (2928003293411faf1de74af77d316759fa0c0efd / ffcd97280a5232593eb33cb73fd5a0245e025fb4)
Make the global server tag options configurable, via the global room setting UI, and make their values actually work as overrides. (I'm not going to contribute this)
Translate the current file/env-based configuration to UI-based configuration in the site and role setting menus. (I'm not going to contribute this)

sonarcloud[bot] commented 2 months ago

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

farhatahmad commented 3 weeks ago

There are some architecture changes that I would make here to match the way the rest of the application works:

Don't add the env vars to react unless they are necessary
- What I would do instead is create a ServerTagsController and push all of the logic related to fetching the tags into there (including the role stuff)
- then, if server tags is not enabled, just return [ ] instead of a list of available tags (and dont show the dropdown)
- You can then remove it completely from CurrentUserSerializer

Once this change is made, I think it would simplify the logic quite a bit

Ithanil commented 3 weeks ago

There are some architecture changes that I would make here to match the way the rest of the application works:
* Don't add the env vars to react unless they are necessary

* What I would do instead is create a `ServerTagsController` and push all of the logic related to fetching the tags into there (including the role stuff)

* then, if server tags is not enabled, just return [ ] instead of a list of available tags (and dont show the dropdown)

* You can then remove it completely from `CurrentUserSerializer`
Once this change is made, I think it would simplify the logic quite a bit

Thanks four your review and I agree with your suggestions. I knew my iteratively developed solution would need a refactoring, but I wanted to have your opinion first to avoid doing it twice. However, I think I won't get it done today as it's basically weekend already here. ;-)

Ithanil commented 3 weeks ago

@farhatahmad That said, do you think the implementation can stay as is in terms of UI/functionality and DB migrations? Because then I would soon be able to deploy the feature for our users independent of the upstream review/merge/release status.

Ithanil commented 3 weeks ago

@farhatahmad I'm done with refactoring. It required me to understand more of the architecture, but as a result I think the code has greatly improved in quality. No env variables are passed to React anymore. Also, now invalid/forbidden tags will silently be dropped by meeting_starter (i.e. falling back to untagged), which matches what a user would see in that case (default label, i.e. untagged). This makes the solution more robust in case an admin forgot to use the sync task after changes and also against malicious users.

farhatahmad commented 2 weeks ago

Definitely a lot cleaner and in line with the way Greenlight is designed. I'll do a full indepth review tomorrow

sonarcloud[bot] commented 1 week ago

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

bigbluebutton / greenlight