Restcomm 1512 - Extensions API changes needed for FAC by gvagenas · Pull Request #2757 · RestComm/Restcomm-Connect

gvagenas · 2018-01-16T10:03:17Z

Changes needed to the Extensions API to support Feature Access Control feature

This close #RESTCOMM-1519

jaimecasero · 2018-01-16T10:09:59Z

     * @return ExtensionResponse see ExtensionResponse
     */
-    ExtensionResponse preInboundAction(SipServletRequest request);
+    ExtensionResponse preInboundAction(IExtensionRequest extensionRequest);


i like the idea of being independent to underlying framework (servlet,akka), so good choice on having our own request. It would be still a good idea to be able to recover ref to original framework objects using generic container of objects, so extension impl may use them if required. I noticed in the "init" method we still inject a ServletContext, so the extension is still tied to specific ServletAPI. not sure if is time to have an specific ExtensionContext as well, and pass it as part of the request...

ExtensionContext sounds a good idea @jaimecasero. Not sure also if its the proper time to do it.
I think that since now the team will be more familiar with the Extensions API, for the next task we can plan and estimate enhancements like the one you suggest

jaimecasero · 2018-01-16T10:10:52Z

    }
+
+    public ExtensionResponse executePreInboundAction(final IExtensionRequest er, List<RestcommExtensionGeneric> extensions) {
+        ExtensionResponse response = new ExtensionResponse();


what is the meaning of this default response?, how do we detect if this response is coming from an actual extension or not? maybe the empty response is good enough for the framework, just wondering...

@jaimecasero The ExtensionResponse object will be set by each extension. The response holds an object that can be used to pass data from one extension back to RC, such as custom headers, custom proxy headers etc.

Currently, we don't use the post-*-action methods so we just create a response object and return it.

The idea for the post-*-action is to use them for maintenance and logistic, such as update logs, update db, notify services (charging) etc.....

jaimecasero · 2018-01-16T10:13:23Z

+        return response;
+    }
+
+    public ExtensionResponse executePostInboundAction(final IExtensionRequest er,  List<RestcommExtensionGeneric> extensions) {


this extension API is starting to feel very specific ,with many variants of same purpose. Not sure if we could push all this executePre/PostXAction methods into the request interface, so the request tells the type of exec point X by a using a getter in the req interface... I think we could have a single pre and post methods...

@jaimecasero we can discuss pros/cons about this for the next Extensions API enhancements

jaimecasero · 2018-01-16T10:20:09Z

+        ExtensionController ec = ExtensionController.getInstance();
+        ApiRequest apiRequest = new ApiRequest(sid.toString(), data, ApiRequest.Type.CREATE_SUBACCOUNT);
+
+        if (executePostApiAction(apiRequest)) {


is this really post,or should be pre?

@jaimecasero good catch, this must be pre. I will fix that

jaimecasero · 2018-01-16T10:22:28Z

+                    call.tell(new Hangup(errMsg), source);
+                    return;
+                }
+                ec.executePostOutboundAction(er, extensions);


should we delay the invocation of PostAction to when the ASR is actually executed?

@jaimecasero, in general, the post action should be executed after the pre action.

At the point of your comment, the call is already answered and maybe had already executed some RCML verbs, the interpreter started executions of the gather verb with asr which account was not permitted and thus interpreter hangup the call. I think this is the proper place for post action method.

jaimecasero · 2018-01-16T10:24:06Z

+                    final NotificationsDao notifications = storage.getNotificationsDao();
+                    final Notification notification = notification(WARNING_NOTIFICATION, 11001, "Outbound SMS is now allowed");
+                    notifications.addNotification(notification);
+                    fsm.transition(message, finished);


are we covering postaction invocation later as part of FSM?

@jaimecasero no, I missed the post action and I will add it now

jaimecasero · 2018-01-16T10:24:40Z

+                    }
+                    final NotificationsDao notifications = storage.getNotificationsDao();
+                    final Notification notification = notification(WARNING_NOTIFICATION, 11001, "Outbound SMS is now allowed");
+                    notifications.addNotification(notification);


are we covering postaction invocation as part of fsm lateR?

@jaimecasero no, I missed the post action and I will add it now

jaimecasero · 2018-01-16T10:28:16Z

+                    sendNotification(errMsg, 11001, "warning", true);
+                    final SipServletResponse resp = request.createResponse(SC_FORBIDDEN, "Inbound USSD session is not Allowed");
+                    resp.send();
+                    ec.executePostOutboundAction(far, extensions);


are we invoking postAction here if feature isAllowed in the other branch?

@jaimecasero we need to invoke post action in any case, I will fix that

jaimecasero · 2018-01-16T10:45:10Z

+                            + " is not Allowed";
+                    sendNotification(errMsg, 11001, "warning", true);
+                    final SipServletResponse resp = request.createResponse(SC_FORBIDDEN, "Call not allowed");
+                    resp.send();


where do we cover postAction in this execution point (for both allowed and notAllowed options)?

@jaimecasero I missed that, I will fix this

maria-farooq · 2018-01-16T11:39:20Z

my comment on the topic is related to RestcommExtensionGeneric.java I think for me

postApiAction
postInboundAction
postOutboundAction
preApiAction
preInboundAction
preOutboundAction

are same method and should have similar approach on applying a sequence of extensions to a given Action/Request regardless of it being pre or post, inbound or out bound, api request or any-other request

This refer to #RESTCOMM-1519

gvagenas · 2018-01-17T10:28:12Z

@maria-farooq @jaimecasero about your comments on improvements for Extensions API, I think those should definitely be discussed but are out of the scope of this feature and this sprint.

Any changes in the API will have an impact on existing extensions. As I suggested earlier, we should record any suggestion, discuss them and in the next sprints plan them

maria-farooq · 2018-01-17T13:14:08Z

@@ -0,0 +1,14 @@
+package org.restcomm.connect.extension.api;


Should we add copyright statement in begenning of this file?

maria-farooq · 2018-01-17T13:17:11Z

@@ -0,0 +1,79 @@
+package org.restcomm.connect.telephony.api;


shoudl we adde copyright statement in begening of this file

gvagenas added 3 commits January 12, 2018 17:15

New Extension Request class and interface for Feature Access Control

d315862

This close #RESTCOMM-1519

New execution points Feature Access Control

9e32538

This close #RESTCOMM-1519

Updates on execution points for Feature Access Control

2b5c3c9

This close #RESTCOMM-1519

jaimecasero reviewed Jan 16, 2018

View reviewed changes

Updates on execution points for Feature Access Control

666e1f6

This refer to #RESTCOMM-1519

maria-farooq reviewed Jan 17, 2018

View reviewed changes

added copyright statements

ebe2002

gvagenas merged commit b7874f6 into master Jan 19, 2018

gvagenas deleted the restcomm-1512 branch January 19, 2018 11:29

Conversation

gvagenas commented Jan 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maria-farooq commented Jan 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gvagenas commented Jan 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

maria-farooq commented Jan 16, 2018 •

edited

Loading