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

Support for Scalelite Tagged Servers #5790

Merged
merged 32 commits into from
Jul 24, 2024

Conversation

Ithanil
Copy link
Contributor

@Ithanil Ithanil commented Apr 26, 2024

This PR adds frontend support for "Tagged Servers" in Scalelite, a feature which is described here: blindsidenetworks/scalelite#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
Screenshot from 2024-04-29 15-48-56

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 29c0c49 and further improved by 2928003
  • 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 2928003 & ffcd972, 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 (29c0c49)
  • 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 (2928003 / ffcd972)
  • 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)

@Ithanil Ithanil force-pushed the tagged_servers branch 3 times, most recently from ac857ee to beedc4b Compare April 29, 2024 10:54
@Ithanil Ithanil marked this pull request as ready for review April 29, 2024 12:21
@Ithanil Ithanil changed the title Support for Scalelite Tagged Servers [WIP] Support for Scalelite Tagged Servers Apr 29, 2024
@Ithanil Ithanil force-pushed the tagged_servers branch 3 times, most recently from 1ab2201 to 29c0c49 Compare May 6, 2024 15:37
Copy link

sonarcloud bot commented Jun 5, 2024

Quality Gate Passed 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
Copy link
Collaborator

farhatahmad commented Jul 12, 2024

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
Copy link
Contributor Author

Ithanil commented Jul 12, 2024

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
Copy link
Contributor Author

Ithanil commented Jul 12, 2024

@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 Ithanil marked this pull request as draft July 15, 2024 13:03
@Ithanil Ithanil marked this pull request as ready for review July 16, 2024 07:22
@Ithanil
Copy link
Contributor Author

Ithanil commented Jul 16, 2024

@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.

@Ithanil Ithanil marked this pull request as draft July 17, 2024 21:20
@Ithanil Ithanil marked this pull request as ready for review July 18, 2024 08:20
@farhatahmad
Copy link
Collaborator

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

…s in the expected state (i.e. no tags related options present)
Copy link

sonarcloud bot commented Jul 24, 2024

@farhatahmad farhatahmad merged commit f123946 into bigbluebutton:master Jul 24, 2024
1 of 2 checks passed
@Ithanil Ithanil deleted the tagged_servers branch July 24, 2024 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants