fix(transcriptions,recording): Allows non moderators with features to dial, record or transcribe. #15074

damencho · 2024-08-30T14:11:13Z

No description provided.

damencho · 2024-08-30T20:07:35Z

Hum, I'm getting forbidden when I'm a moderator without a token. It still needs more testing.

damencho · 2024-09-03T13:37:56Z

Hum, I'm getting forbidden when I'm a moderator without a token. It still needs more testing.

It is fixed now.

bgrozev · 2024-09-04T20:58:20Z

react/features/subtitles/middleware.ts

+/**
+ * The dial command to use for starting a transcriber.
+ */
+const TRANSCRIBER_DIAL_COMMAND = 'jitsi_meet_transcribe';


Suggest something other than "command" which we already used for other things. I looked it up and we use "number" (in the ljm API), "to" (in the rayo xml), and "destination" (in our java rayo impl).

bgrozev · 2024-09-04T21:04:26Z

resources/prosody-plugins/mod_filter_iq_jibri.lua

+
+            -- if current user is not allowed, but was granted moderation by a user
+            -- that is allowed by its features we want to allow it
+            local is_granting_allowed = false;


I don't think you care about the distinction of whether it was "naturally" allowed or because the features were granted. I suggest simplifying to something like

local is_allowed = is_feature_allowed(session.jitsi_meet_context_features, feature) or is_feature_allowed(session.granted_jitsi_meet_context_features, feature)

But I want to check that only if there are features set.

A right, we have changed behavior of is_feature_allowed, so this will work.

Nope, this is not correct as it will check for moderator rights if you have the feature = "false".
so we have two cases, are features available and is the feature set to false.

bgrozev · 2024-09-04T21:12:29Z

resources/prosody-plugins/mod_filter_iq_jibri.lua

+                then
+                    if not session.jitsi_meet_context_features and not session.granted_jitsi_meet_context_features then
+                        -- we need to check for moderator rights
+                        -- when there are no features and the occupant is moderator we allow recording


Why don't we extract the set of features in a central place? I think it would be less error prone. For example, we can do it at token verification time. We can read the token and set the features accordingly. If the moderator flag is raised and no features are specified we can populate the defaults. That would also be the place to set features to an empty set when no token is present. Then, everywhere else we can assume that the set of features is set accordingly, and we don't have to know the details.

This whole section would simplify to:

local is_allowed = ... if ~is_allowed then module:log(...) session.send(st.error_reply(stanza, "auth", "forbidden")); end

We do not have a common rule for defining moderator, and ther rules we use in deploments are outside of this project.
The extraction time of features is before we know whether it is a moderator.

I see. Then can we extract a utility function like is_feature_allowed(feature, features, granted_features, isModerator)? So that all that logic is consistent and in one place?

I think I addressed that now.

We lost some small optimizations cause before we were looking up the room to search for the occupant to check the role only when needed.

resources/prosody-plugins/mod_filter_iq_rayo.lua

bgrozev · 2024-09-04T21:17:26Z

resources/prosody-plugins/mod_filter_iq_rayo.lua

+    local data, key, occupant, session = event.data, event.key, event.actor, event.session;
+
+    if occupant.role == 'moderator' then
+        return data;


What about the case of a moderator which doesn't have transcription rights?

bgrozev · 2024-09-04T21:26:09Z

react/features/recording/components/Recording/web/StartRecordingDialogContent.tsx

@@ -37,9 +37,11 @@ class StartRecordingDialogContent extends AbstractStartRecordingDialogContent {
     * @returns {React$Component}
     */
    render() {
+        const _renderRecording = this.props._isModerator || this.props._hasRecordingFeature;


Similar as above, it should be based on the content of features, with the moderator flag only being used to infer defaults

bgrozev · 2024-09-04T21:26:18Z

react/features/recording/functions.ts

@@ -267,8 +267,8 @@ export function getRecordButtonProps(state: IReduxState) {

    if (localRecordingEnabled) {
        visible = true;
-    } else if (isModerator) {
-        visible = recordingEnabled ? isJwtFeatureEnabled(state, 'recording', true) : false;
+    } else if (isModerator || isJwtFeatureEnabled(state, 'recording', false, false)) {


Same as above

bgrozev · 2024-09-04T21:27:00Z

react/features/recording/functions.ts

-    if (localParticipantIsModerator && !isInBreakoutRoom) {
-        visible = liveStreamingEnabled ? liveStreamingEnabledInJwt : false;
+    if (!isInBreakoutRoom) {
+        visible = liveStreamingEnabled ? localParticipantIsModerator || liveStreamingEnabledInJwt : false;


Similar to above, I think

bgrozev · 2024-09-04T21:27:19Z

react/features/subtitles/middleware.ts

+    if (enabled && conference?.getTranscriptionStatus() === 'off') {
+        const featureAllowed = isJwtFeatureEnabled(getState(), 'transcription', false, false);
+
+        if (isLocalParticipantModerator(state) || featureAllowed) {


Same as above

bgrozev · 2024-09-04T21:28:18Z

react/features/transcribing/functions.ts


    if (!transcription?.enabled) {
        return false;
    }

-    if (isJwtTranscribingEnabled) {
+    if (isLocalParticipantModerator(state) || isJwtTranscribingEnabled) {


Same as above

bgrozev · 2024-09-05T20:47:00Z

resources/prosody-plugins/mod_filter_iq_jibri.lua

-                    or not is_feature_allowed(session.jitsi_meet_context_features,
-                    (jibri.attr.recording_mode == 'file' and 'recording' or 'livestreaming')
-                ) then
+                if token == nil or not is_allowed then


Do we still need to check token here? What are we supposed to do if the user is a moderator with no token? Or if the user was granted features, but has no token?

If some form of check for token is necessary can we move it to is_feature_allowed?

What are we supposed to do if the user is a moderator with no token?

The case when you are not using jwt for authentication, but you are moderator. Hum but this is explicit check if there is no token ... I will drop it.

Or if the user was granted features, but has no token?

Most of the cases are like this, you grant moderation to guests that has no token.

… dial, record or transcribe.

It works now and without a token and no features, but being moderator.

…tering metadata service.

hristoterezov · 2024-09-11T20:52:47Z

react/features/base/jwt/constants.ts

@@ -20,10 +20,10 @@ export const MEET_FEATURES = {

 /**
 * A mapping between jwt features and toolbar buttons keys.
+ * We don't need recording in here, as it will disable and local recording.


Not sure I understand the message. Can you elaborate?

Cause even when feature recording(the feature is for server-side recording) is disabled you see local recording.

Or I'm wrong ... ?

hristoterezov · 2024-09-11T21:12:04Z

react/features/recording/hooks.web.ts

-            })) {
+            && !_isInBreakoutRoom
+            && liveStreaming?.enabled
+            && liveStreamingAllowed) {


Why did we remove the isLiveStreamingButtonVisible function isn't the condition the same here and in the component?

Nope, as it depends on the isModerator only if the token is missing or there are no features in the token.

The changes requested have been implemented.

damencho requested a review from bgrozev August 30, 2024 14:26

damencho force-pushed the non-moderator-features branch from b99a1de to 9ca166e Compare September 3, 2024 14:02

bgrozev previously requested changes Sep 4, 2024

View reviewed changes

damencho force-pushed the non-moderator-features branch 3 times, most recently from c373ff7 to 47ee211 Compare September 4, 2024 23:02

bgrozev reviewed Sep 5, 2024

View reviewed changes

damencho added 10 commits September 10, 2024 16:03

fix(transcriptions): Uses dial command to invite transcriber.

e5f6c78

fix(transcriptions,recording): Allows non moderators with features to…

1f4ae52

… dial, record or transcribe.

sqaush: Make sure filtering works when only is a moderator.

1a0164f

It works now and without a token and no features, but being moderator.

squash: Rename constant.

3770eab

squash: Checks features first before defaulting to moderator when fil…

1756c43

…tering metadata service.

squash: Checks features first before defaulting to moderator in UI.

08eff23

squash: Fixes lint and one other check.

936e35e

squash: Moves more logic to is_feature_allowed.

853ad68

squash: Drops unnecessary check.

ef7ef96

squash: Uses constant coming from ljm.

bccbb5b

damencho force-pushed the non-moderator-features branch from 655f283 to bccbb5b Compare September 10, 2024 21:03

squash: Toggles back captions button on error.

7e7b17e

hristoterezov reviewed Sep 11, 2024

View reviewed changes

damencho added 2 commits September 12, 2024 08:50

squash: Fix comment.

708d1d2

squash: Reverting back isLiveStreamingButtonVisible.

d00219c

damencho force-pushed the non-moderator-features branch from baea250 to d00219c Compare September 12, 2024 14:22

squash: Fix imports.

c48a94c

hristoterezov approved these changes Sep 12, 2024

View reviewed changes

damencho merged commit b3742a3 into master Sep 13, 2024
9 checks passed

damencho deleted the non-moderator-features branch September 13, 2024 16:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(transcriptions,recording): Allows non moderators with features to dial, record or transcribe. #15074

fix(transcriptions,recording): Allows non moderators with features to dial, record or transcribe. #15074

damencho commented Aug 30, 2024

damencho commented Aug 30, 2024

damencho commented Sep 3, 2024

bgrozev Sep 4, 2024

bgrozev Sep 4, 2024

damencho Sep 4, 2024

damencho Sep 4, 2024

damencho Sep 4, 2024

bgrozev Sep 4, 2024

damencho Sep 4, 2024

bgrozev Sep 4, 2024 •

edited

Loading

damencho Sep 4, 2024

damencho Sep 4, 2024

bgrozev Sep 4, 2024

bgrozev Sep 4, 2024

bgrozev Sep 4, 2024

bgrozev Sep 4, 2024

bgrozev Sep 4, 2024

bgrozev Sep 4, 2024

bgrozev Sep 5, 2024

damencho Sep 5, 2024

damencho Sep 5, 2024

hristoterezov Sep 11, 2024

damencho Sep 11, 2024

damencho Sep 11, 2024

hristoterezov Sep 11, 2024

damencho Sep 11, 2024

fix(transcriptions,recording): Allows non moderators with features to dial, record or transcribe. #15074

fix(transcriptions,recording): Allows non moderators with features to dial, record or transcribe. #15074

Conversation

damencho commented Aug 30, 2024

damencho commented Aug 30, 2024

damencho commented Sep 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgrozev Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgrozev Sep 4, 2024 •

edited

Loading