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

Allow VideoRoom to choose whether signed tokens should be used #2825

Merged
merged 1 commit into from
Dec 7, 2021

Conversation

lminiero
Copy link
Member

After a recent fix contributed in #2812, some users experienced issues when using the VideoRoom with tokens. Specifically, when Janus was used in this specific configuration:

  • signed tokens used in the core
  • regular tokens used in the VideoRoom (ACL)

access would always fail, because the plugin was hardcoded to assume signed tokens would be used there too when enabled in the core. This patch tries to address that, by adding a new core callback to check if signed-mode is active, and adding a new property when creating VideoRoom to choose whether they should be used in the plugin too. By default, signed tokens are never used, which means you need to set signed_tokens:true when creating a room yourself if you do want them instead. Of course, trying to enable signed tokens when they're not active in the core will not work, and a warning will be displayed.

I haven't tested this yet, but I'd ask @timsolov to make sure I didn't make any change that invalidates the PR he contributed (noting again that signed_token must be manuallyset to true when creating the room or they won't be used).

@lminiero
Copy link
Member Author

lminiero commented Dec 7, 2021

Merging.

@lminiero lminiero merged commit f9906da into master Dec 7, 2021
@lminiero lminiero deleted the videoroom-signed branch December 7, 2021 15:03
@timsolov
Copy link
Contributor

Hello, I think this is good behavior. I just checked it and it works fine except signed_tokens field doesn't save to janus.plugin.videoroom.jcfg config file when room is permanent. We need to fix it.

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