List private rooms if valid admin_key was provided. #2161
Conversation
|
Thanks for your contribution, @robby2016! Please make sure you sign our CLA, as it's a required step before we can merge this. |
|
Thanks for the contribution! Makes sense, and to apply it to other plugins as well. That said, please use tabs, not spaces: if your editor supports it, the repo has an |
plugins/janus_videoroom.c
Outdated
| continue; | ||
| } | ||
| if(room->is_private) { | ||
| /* only if admin_key isset */ |
There was a problem hiding this comment.
Comments should start with a capital letter (code style).
plugins/janus_videoroom.c
Outdated
| if(room->is_private) { | ||
| /* only if admin_key isset */ | ||
| if(admin_key != NULL) { | ||
| json_t *admin_key_json = json_object_get(root, "admin_key"); |
There was a problem hiding this comment.
I don't understand why you're checking this property any time you hit a private room. I'd just to it once, before iterating on the rooms at all, and this way you know when you need it if a valid key was provided (and return an error right away if it's wrong, rather than adding a goto). You can refer to other methods that use it for that.
plugins/janus_videoroom.c
Outdated
| JANUS_VIDEOROOM_ERROR_MISSING_ELEMENT, JANUS_VIDEOROOM_ERROR_INVALID_ELEMENT, JANUS_VIDEOROOM_ERROR_UNAUTHORIZED); | ||
| if(error_code != 0) { | ||
| JANUS_LOG(LOG_VERB, "No room list, wrong admin_key provided\n"); | ||
| goto end_loop; |
There was a problem hiding this comment.
See above: this can be done without the loop (the way it's done currently leaks memory, by the way).
plugins/janus_videoroom.c
Outdated
| JANUS_LOG(LOG_VERB, "Skipping private room '%s'\n", room->room_name); | ||
| janus_refcount_decrease(&room->ref); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Pre-checking the admin_key you also simplify this duplicate error management.
|
Thank you very much for the great feedback. Of course! You're absolutely right, the check should happen outside of the loop. Hope this second version is better. |
plugins/janus_videoroom.c
Outdated
| /* Skip private room if no valid admin_key was provided */ | ||
| JANUS_LOG(LOG_VERB, "Skipping private room '%s'\n", room->room_name); | ||
| janus_refcount_decrease(&room->ref); | ||
| continue; |
There was a problem hiding this comment.
Looks like you added an extra tab for the lines above?
plugins/janus_videoroom.c
Outdated
| janus_mutex_unlock(&rooms_mutex); | ||
| goto prepare_response; | ||
| } | ||
| else { |
There was a problem hiding this comment.
Please put theelse on the same line as the closing bracket (only code style, sorry for the bother).
plugins/janus_videoroom.c
Outdated
| if(admin_key != NULL) { | ||
| json_t *admin_key_json = json_object_get(root, "admin_key"); | ||
| /* Verify admin_key if it was provided */ | ||
| if(admin_key_json != NULL && strlen(json_string_value(admin_key_json)) > 0) { |
There was a problem hiding this comment.
This should also have a check on json_is_string(admin_key_json), as there's no guarantee admin_key is a string in the JSON (it's not part of the objects we use for validation in "list", since list requires no arguments).
|
Not a problem at all and sorry for the styles issues. |
|
No problem, we don't really have these rules written anywhere... I probably should do that! |
|
FYI, I just added the same changes to other plugins with private resources as well. |
|
Awesome! Thank you :) |
This simple enhancement allows admins to list private rooms as well using the existing "list" request.
To make this fully backwards compatible, if no "admin_key" is provided the list gets returned like before (without the private rooms).
The same solution could be applied to other plugins like the audiobrige too.