ARI: Correct error codes for bridge operations

This patch adds error checking to ARI bridge operations, when
adding/removing channels to/from bridges.

In general, the error codes fall out as follows:
 * Bridge not found - 404 Not Found
 * Bridge not in Stasis - 409 Conflict
 * Channel not found - 400 Bad Request
 * Channel not in Stasis - 422 Unprocessable Entity
 * Channel not in this bridge (on remove) - 422 Unprocessable Entity

(closes issue ASTERISK-22036)
Review: https://reviewboard.asterisk.org/r/2769/

git-svn-id: https://origsvn.digium.com/svn/asterisk/trunk@397565 65c4cc65-6c06-0410-ace0-fbb531ad65f3

diff --git a/include/asterisk/stasis_app.h b/include/asterisk/stasis_app.h
index 33e5c26..528c480 100644 (file)
--- a/include/asterisk/stasis_app.h
+++ b/include/asterisk/stasis_app.h
@@ -361,8 +361,10 @@ int stasis_app_bridge_moh_stop(
  *
  * \param control Control whose channel should be added to the bridge
  * \param bridge Pointer to the bridge
+ * \return non-zero on failure
+ * \return zero on success
  */
-void stasis_app_control_add_channel_to_bridge(
+int stasis_app_control_add_channel_to_bridge(
        struct stasis_app_control *control, struct ast_bridge *bridge);
 
 /*!
@@ -370,11 +372,23 @@ void stasis_app_control_add_channel_to_bridge(
  *
  * \param control Control whose channel should be removed from the bridge
  * \param bridge Pointer to the bridge
+ * \return non-zero on failure
+ * \return zero on success
  */
-void stasis_app_control_remove_channel_from_bridge(
+int stasis_app_control_remove_channel_from_bridge(
        struct stasis_app_control *control, struct ast_bridge *bridge);
 
 /*!
+ * \since 12
+ * \brief Gets the bridge currently associated with a control object.
+ *
+ * \param control Control object for the channel to query.
+ * \return Associated \ref ast_bridge.
+ * \return \c NULL if not associated with a bridge.
+ */
+struct ast_bridge *stasis_app_get_bridge(struct stasis_app_control *control);
+
+/*!
  * \brief Destroy the bridge.
  *
  * \param bridge_id Uniqueid of bridge to be destroyed

diff --git a/include/asterisk/stasis_app_impl.h b/include/asterisk/stasis_app_impl.h
index e9b93a8..d4b4677 100644 (file)
--- a/include/asterisk/stasis_app_impl.h
+++ b/include/asterisk/stasis_app_impl.h
@@ -85,14 +85,4 @@ void *stasis_app_send_command(struct stasis_app_control *control,
 int stasis_app_send_command_async(struct stasis_app_control *control,
        stasis_app_command_cb command, void *data);
 
-/*!
- * \since 12
- * \brief Gets the bridge currently associated with a control object.
- *
- * \param control Control object for the channel to query.
- * \return Associated \ref ast_bridge.
- * \return \c NULL if not associated with a bridge.
- */
-struct ast_bridge *stasis_app_get_bridge(struct stasis_app_control *control);
-
 #endif /* _ASTERISK_RES_STASIS_H */

diff --git a/res/ari/resource_bridges.c b/res/ari/resource_bridges.c
index 670792f..ef7767f 100644 (file)
--- a/res/ari/resource_bridges.c
+++ b/res/ari/resource_bridges.c
@@ -99,6 +99,18 @@ static struct stasis_app_control *find_channel_control(
 
        control = stasis_app_control_find_by_channel_id(channel_id);
        if (control == NULL) {
+               /* Distinguish between 400 and 422 errors */
+               RAII_VAR(struct ast_channel_snapshot *, snapshot, NULL,
+                       ao2_cleanup);
+               snapshot = ast_channel_snapshot_get_latest(channel_id);
+               if (snapshot == NULL) {
+                       ast_log(LOG_DEBUG, "Couldn't find '%s'\n", channel_id);
+                       ast_ari_response_error(response, 400, "Bad Request",
+                               "Channel not found");
+                       return NULL;
+               }
+
+               ast_log(LOG_DEBUG, "Found non-stasis '%s'\n", channel_id);
                ast_ari_response_error(response, 422, "Unprocessable Entity",
                        "Channel not in Stasis application");
                return NULL;
@@ -145,6 +157,7 @@ static struct control_list *control_list_create(struct ast_ari_response *respons
                list->controls[list->count] =
                        find_channel_control(response, channels[i]);
                if (!list->controls[list->count]) {
+                       /* response filled in by find_channel_control() */
                        return NULL;
                }
                ++list->count;
@@ -166,7 +179,7 @@ void ast_ari_add_channel_to_bridge(struct ast_variable *headers, struct ast_add_
        size_t i;
 
        if (!bridge) {
-               /* Response filled in by find_bridge */
+               /* Response filled in by find_bridge() */
                return;
        }
 
@@ -200,7 +213,7 @@ void ast_ari_remove_channel_from_bridge(struct ast_variable *headers, struct ast
        size_t i;
 
        if (!bridge) {
-               /* Response filled in by find_bridge */
+               /* Response filled in by find_bridge() */
                return;
        }
 
@@ -210,17 +223,22 @@ void ast_ari_remove_channel_from_bridge(struct ast_variable *headers, struct ast
                return;
        }
 
-       /* BUGBUG this should make sure the bridge requested for removal is actually
-        * the bridge the channel is in. This will be possible once the bridge uniqueid
-        * is added to the channel snapshot. A 409 response should be issued if the bridge
-        * uniqueids don't match */
+       /* Make sure all of the channels are in this bridge */
        for (i = 0; i < list->count; ++i) {
-               stasis_app_control_remove_channel_from_bridge(list->controls[i],
-                       bridge);
+               if (stasis_app_get_bridge(list->controls[i]) != bridge) {
+                       ast_log(LOG_WARNING, "Channel %s not in bridge %s\n",
+                               args->channel[i], args->bridge_id);
+                       ast_ari_response_error(response, 422,
+                               "Unprocessable Entity",
+                               "Channel not in this bridge");
+                       return;
+               }
        }
 
-       if (response->response_code) {
-               return;
+       /* Now actually remove it */
+       for (i = 0; i < list->count; ++i) {
+               stasis_app_control_remove_channel_from_bridge(list->controls[i],
+                       bridge);
        }
 
        ast_ari_response_no_content(response);

diff --git a/res/res_ari_bridges.c b/res/res_ari_bridges.c
index fe72df7..bc8e200 100644 (file)
--- a/res/res_ari_bridges.c
+++ b/res/res_ari_bridges.c
@@ -345,9 +345,10 @@ static void ast_ari_add_channel_to_bridge_cb(
                break;
        case 500: /* Internal Server Error */
        case 501: /* Not Implemented */
+       case 400: /* Channel not found */
        case 404: /* Bridge not found */
        case 409: /* Bridge not in Stasis application */
-       case 422: /* Channel not found, or not in Stasis application */
+       case 422: /* Channel not in Stasis application */
                is_valid = 1;
                break;
        default:
@@ -444,6 +445,10 @@ static void ast_ari_remove_channel_from_bridge_cb(
                break;
        case 500: /* Internal Server Error */
        case 501: /* Not Implemented */
+       case 400: /* Channel not found */
+       case 404: /* Bridge not found */
+       case 409: /* Bridge not in Stasis application */
+       case 422: /* Channel not in this bridge */
                is_valid = 1;
                break;
        default:

diff --git a/res/stasis/control.c b/res/stasis/control.c
index df27916..8530abd 100644 (file)
--- a/res/stasis/control.c
+++ b/res/stasis/control.c
@@ -510,6 +510,9 @@ static void bridge_after_cb_failed(enum ast_bridge_after_cb_reason reason,
                ast_bridge_after_cb_reason_string(reason));
 }
 
+static int OK = 0;
+static int FAIL = -1;
+
 static void *app_control_add_channel_to_bridge(
        struct stasis_app_control *control,
        struct ast_channel *chan, void *data)
@@ -542,7 +545,7 @@ static void *app_control_add_channel_to_bridge(
                bridge_after_cb_failed, control);
        if (res != 0) {
                ast_log(LOG_ERROR, "Error setting after-bridge callback\n");
-               return NULL;
+               return &FAIL;
        }
 
        {
@@ -569,22 +572,24 @@ static void *app_control_add_channel_to_bridge(
                        ast_log(LOG_ERROR, "Error adding channel to bridge\n");
                        ast_channel_pbx_set(chan, control->pbx);
                        control->pbx = NULL;
-                       return NULL;
+                       return &FAIL;
                }
 
                ast_assert(stasis_app_get_bridge(control) == NULL);
                control->bridge = bridge;
        }
-       return NULL;
+       return &OK;
 }
 
-void stasis_app_control_add_channel_to_bridge(
+int stasis_app_control_add_channel_to_bridge(
        struct stasis_app_control *control, struct ast_bridge *bridge)
 {
+       int *res;
        ast_debug(3, "%s: Sending channel add_to_bridge command\n",
                        stasis_app_control_get_channel_id(control));
-       stasis_app_send_command_async(control,
+       res = stasis_app_send_command(control,
                app_control_add_channel_to_bridge, bridge);
+       return *res;
 }
 
 static void *app_control_remove_channel_from_bridge(
@@ -594,7 +599,7 @@ static void *app_control_remove_channel_from_bridge(
        struct ast_bridge *bridge = data;
 
        if (!control) {
-               return NULL;
+               return &FAIL;
        }
 
        /* We should only depart from our own bridge */
@@ -606,20 +611,22 @@ static void *app_control_remove_channel_from_bridge(
                ast_log(LOG_WARNING, "%s: Not in bridge %s; not removing\n",
                        stasis_app_control_get_channel_id(control),
                        bridge->uniqueid);
-               return NULL;
+               return &FAIL;
        }
 
        ast_bridge_depart(chan);
-       return NULL;
+       return &OK;
 }
 
-void stasis_app_control_remove_channel_from_bridge(
+int stasis_app_control_remove_channel_from_bridge(
        struct stasis_app_control *control, struct ast_bridge *bridge)
 {
+       int *res;
        ast_debug(3, "%s: Sending channel remove_from_bridge command\n",
                        stasis_app_control_get_channel_id(control));
-       stasis_app_send_command_async(control,
+       res = stasis_app_send_command(control,
                app_control_remove_channel_from_bridge, bridge);
+       return *res;
 }
 
 const char *stasis_app_control_get_channel_id(

diff --git a/rest-api/api-docs/bridges.json b/rest-api/api-docs/bridges.json
index b698dcf..9f8af9e 100644 (file)
--- a/rest-api/api-docs/bridges.json
+++ b/rest-api/api-docs/bridges.json
@@ -131,6 +131,10 @@
                                        ],
                                        "errorResponses": [
                                                {
+                                                       "code": 400,
+                                                       "reason": "Channel not found"
+                                               },
+                                               {
                                                        "code": 404,
                                                        "reason": "Bridge not found"
                                                },
@@ -140,7 +144,7 @@
                                                },
                                                {
                                                        "code": 422,
-                                                       "reason": "Channel not found, or not in Stasis application"
+                                                       "reason": "Channel not in Stasis application"
                                                }
                                        ]
                                }
@@ -172,6 +176,24 @@
                                                        "allowMultiple": true,
                                                        "dataType": "string"
                                                }
+                                       ],
+                                       "errorResponses": [
+                                               {
+                                                       "code": 400,
+                                                       "reason": "Channel not found"
+                                               },
+                                               {
+                                                       "code": 404,
+                                                       "reason": "Bridge not found"
+                                               },
+                                               {
+                                                       "code": 409,
+                                                       "reason": "Bridge not in Stasis application"
+                                               },
+                                               {
+                                                       "code": 422,
+                                                       "reason": "Channel not in this bridge"
+                                               }
                                        ]
                                }
                        ]