Add support for JaaS #185
Add support for JaaS #185
Conversation
larkox
added
2: Dev Review
Codecov Report
@@ Coverage Diff @@
## master #185 +/- ##
===========================================
- Coverage 44.95% 33.12% -11.84%
===========================================
Files 7 7
Lines 803 1105 +302
===========================================
+ Hits 361 366 +5
- Misses 417 710 +293
- Partials 25 29 +4
Continue to review full report at Codecov.
|
server/api.go
Outdated
| @@ -80,6 +229,11 @@ func (p *Plugin) handleConfig(w http.ResponseWriter, r *http.Request) { | |||
| } | |||
|
|
|||
| func (p *Plugin) handleExternalAPIjs(w http.ResponseWriter, r *http.Request) { | |||
| if p.getConfiguration().UseJaaS { | |||
| p.proxyExternalAPIjsJaaS(w, r) | |||
There was a problem hiding this comment.
We were more include to use a javascript file that is under control to avoid that a security issue in the JaaS service propagates some how in to the mattermost service. @jupenur any opinion here?
| </label> | ||
| <div className='help-text'> | ||
| <span> | ||
| {'(Insecure) If your Jitsi server is not compatible with this plugin, include the JavaScript API hosted on your Jitsi server directly in Mattermost instead of the default API version provided by the plugin. WARNING: Enabling this setting can compromise the security of your Mattermost system, if your Jitsi server is not fully trusted and allows direct modification of program files. Use with caution.'} |
There was a problem hiding this comment.
I would love to have all this texts translated.
There was a problem hiding this comment.
I would love to have all this texts translated.
To add translations for these texts i have to use the Mattermost translation server right? https://translate.mattermost.com/projects/i18n-wip/mattermost-webapp-wip/
There was a problem hiding this comment.
Not really, we haven't integrated the plugin itself with our weblate, it is just marking the text as translatable (like in other places) and adding the string to the i18n/en.json file.
| checked={!this.props.embedded} | ||
| onChange={this.props.onJitsiCompatibilityChange} | ||
| /> | ||
| <span>{'false'}</span> |
There was a problem hiding this comment.
This looks like more like a label for the input
There was a problem hiding this comment.
@jespino What exactly are you expecting here?
There was a problem hiding this comment.
My main concern is about the usage of an external javascript file that can be modified by an attacker and can lead into security problem in our side, I going to wait for the @jupenur opinion here, but I expect we want a similar to the already applied one. If you enable JaaS, use a plugin copy of the js file, unless you enable the Compatiblity Mode, that would use the external one. Other than that, another set of comments, but if QA passes it properly and @larkox approve it to. I would love to merge it.
|
@theunafraid one question, is there any difference between the external api script from the jitsi installation and the Jaas one? |
From what i know, the version JaaS version may contain updates that may not be on meet.jit.si but maybe i'm wrong, have to check with someone else. |
No difference, they are exactly the same. Usually using a bundled version works, but sometimes an update is necessary. Right now I'm pretty sure copying the meeting link (with the button) is broken in Mattermost because we need to pass a new Regarding the external JS file. I understand the concern. I think there are 2 ways to go about it:
Given the simple use of the API the plugin uses, I'd say we could start either way and adapt as needed once it starts to see some use. |
|
@saghul @theunafraid ok, then my proposal is to update the file that is bundled with mattermost, and allow to configure the "Compatibility mode" (it does exactly that, instead of using the copy, we proxy the jitsi server file). Our policy is "secure by default" so the default configuration should be to use our copy of the file. The main problem in security is. If somebody enter in your system and is able to modify that file, automatically can load arbitrary javascript in our application under our same domain, giving access to session information. That means, enabling this, the surface of attack is the union of both products, and one if them is completely out of our control. |
|
Sounds good @jespino. @theunafraid can you please make those changes? |
Sure, i'll take care of it. |
|
@jespino Compatibility mode should be restored now. Can you please take another look? |
Codecov Report
@@ Coverage Diff @@
## master #185 +/- ##
===========================================
- Coverage 44.95% 33.06% -11.90%
===========================================
Files 7 6 -1
Lines 803 1104 +301
===========================================
+ Hits 361 365 +4
- Misses 417 710 +293
- Partials 25 29 +4
Continue to review full report at Codecov.
|
|
👋 @jespino Any chance we can get this moved forward? |
|
@larkox @DHaussermann I've added manifest.go, it seems to be ok. |
|
@theunafraid I think that running |
Yes, that was my assumption, i ran make apply but i did not add the ts manifest, will add it now. I thought those files should not be added. |
|
When will this be merged? Thanks :-) |
|
@DHaussermann Do you know when will you be able to test this one? |
|
@dipak-demansol I have been struggling to find time for this PR. Let's follow up on this change and see if it's something we can have you review instead. |
|
@theunafraid Could you please merge |
|
@dipak-demansol Maybe you can help with testing this PR? Not a high priority. |
|
Hi @theunafraid,
Can you provide any insight into what may have occurred here or some isolation steps?
|
|
Yes, there should be a setting Label asking "Which type of Jitsi server will you be using?" then show those 2 options. |
As per @dipak-demansol the same issue still occurs when creating the token directly on the MM server. |
|
Closing in favor of #219 |
hanzei
added
Lifecycle/3:orphaned
and removed
3: QA Review
Hi everyone,
Summary
This pull request adds support for Jitsi as a Service.
The added code will generate a JaaS JWT for meetings. Mattermost users are moderators while non users are handled as guests.
To add mutually exclusive Jitsi/JaaS option had to create a custom setting for the admin console which is compatible with current version. The previous settings are moved to JitsiSettings key and a few more added for JaaS. The jitsiembedded setting is used by both the Jitsi and JaaS version. JitsiSettings component was added for future improvements and mutual exclusivity selection Jitsi/JaaS. When the user upgrades the Jitsi settings should be imported from the previous version. Jitsi server and client code was reused for JaaS except in cases where the client runs outside of Mattermost UI(open in new tab).
If user enables JaaS for this version has to setup Branding, AppID, ApiKey and RSA keys as shown in the JaaS Api Key tutorial and for more info JaaS Start Guide. After following the Api Key tutorial (no code), branding can be done by going to JaaS Branding where the invite URL can be customised for example: https://mymattermostdomain.com/plugins/jitsi/api/v1/meetings, must include /plugins/jitsi/api/v1/meetings. The AppID, ApiKey and RSA private key can be copied in the admin console https://mymattermostdomain.com/admin_console/plugins/plugin_jitsi if JaaS is enabled.
Ticket Link
Fixes #183