New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SIP outbound proxy and auth username #283

Closed
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@mehturt

mehturt commented Apr 12, 2016

No description provided.

mehturt added some commits Apr 8, 2016

Add SIP outbound proxy and auth username.
Added optional SIP outbound proxy and SIP auth user name.  Tested with getonsip.com provider.
Added class SIPProfileConfig for configuration, instead of hashmap.
Permission improvements.
Added callback to connection listener in case a permission is not
granted.
Added new set of mandatory permissions for video call.
@@ -370,6 +370,10 @@ public RCConnection connect(Map<String, Object> parameters, RCConnectionListener
connection.state = RCConnection.ConnectionState.PENDING;
DeviceImpl.GetInstance().sipuaConnectionListener = connection;
if (enableVideo == null) {

This comment has been minimized.

@atsakiridis

atsakiridis Apr 19, 2016

Collaborator

@mehturt: Above where the assignment is made we are casting to Boolean so it should have the same effect. Did you have any issues with that?

This comment has been minimized.

@mehturt

mehturt Apr 19, 2016

@atsakiridis The issue is when "video-enabled" is not present, the enableVideo is null and enableVideo.booleanValue() results in NPE.

This comment has been minimized.

@atsakiridis

atsakiridis Apr 20, 2016

Collaborator

@mehturt: have you tested that? https://docs.oracle.com/javase/7/docs/api/java/util/HashMap.html#get(java.lang.Object) says it returns null if key is not found, which afterwards is casted to Boolean in the code above which should make it false no?

This comment has been minimized.

@mehturt

mehturt Apr 20, 2016

Of course I did :) That's why I added this check.
When enableVideo is null, then enableVideo.booleanValue() throws NPE:
java.lang.NullPointerException: Attempt to invoke virtual method 'boolean java.lang.Boolean.booleanValue()' on a null object reference

This comment has been minimized.

@atsakiridis

atsakiridis Apr 20, 2016

Collaborator

Cool, it's settled then ;)

* @param deviceListener The listener for upcoming RCDevice events
* @return The newly created RCDevice
* @see RCDevice
*/
public static RCDevice createDevice(HashMap<String, Object> parameters, RCDeviceListener deviceListener)
public static RCDevice createDevice(RCDevice.SIPProfileConfig profileConfig, RCDeviceListener deviceListener)

This comment has been minimized.

@atsakiridis

atsakiridis Apr 19, 2016

Collaborator

@mehturt: not sure it's a good idea to change the API at this point. People are already using it and we 'll break all those apps if we change that. If you want to add more options, I would suggest that you do so in the context of the existing data structure by adding new members in the HashMap

This comment has been minimized.

@mehturt

mehturt Apr 19, 2016

@atsakiridis ok I'll (try to) revert this

@@ -33,11 +33,13 @@
private static final String TAG = "SipProfile";
private String localIp;
private int localPort = 5080;
private String transport = "tcp";
private String transport = "udp";

This comment has been minimized.

@atsakiridis

atsakiridis Apr 19, 2016

Collaborator

@mehturt: Any specific rationale for changing to udp? We used to have udp but changed to tcp as its more robust.

This comment has been minimized.

@mehturt

mehturt Apr 19, 2016

@atsakiridis yes, I can revert this, I was testing with udp on my side..

@@ -878,6 +882,11 @@ else if (response.getStatusCode() == Response.RINGING) {
//RCLogger.i(TAG, "BUSY");
dispatchSipEvent(new SipEvent(this,
SipEventType.SERVICE_UNAVAILABLE, "", ""));
} else if (response.getStatusCode() == Response.FORBIDDEN) {

This comment has been minimized.

@atsakiridis

atsakiridis Apr 19, 2016

Collaborator

@mehturt: good call 👍. However keep in mind that FORBIDDEN could be returned in all of REGISTER, INVITE, MESSAGE requests, so for better error handling I would suggest returning:

dispatchSipError(ISipEventListener.ErrorContext.ERROR_CONTEXT_NON_CALL,RCClient.ErrorCodes.SIGNALLING_REGISTER_AUTH_ERROR, "Error authenticating " + tid.getRequest().getMethod());

for REGISTER/MESSAGE requests, BUT for INVITE requests instead return:

dispatchSipError(ISipEventListener.ErrorContext.ERROR_CONTEXT_CALL, RCClient.ErrorCodes.SIGNALLING_REGISTER_AUTH_ERROR, "Error authenticating " + tid.getRequest().getMethod());

Which differentiates between ERROR_CONTEXT_CALL and ERROR_CONTEXT_NON_CALL

This comment has been minimized.

@mehturt

mehturt Apr 19, 2016

@atsakiridis Should the error code here change as well? Something like SIGNALLING_INSTANT_MESSAGE_ERROR for MESSAGE and SIGNALLING_CALL_ERROR for INVITE?

This comment has been minimized.

@atsakiridis

atsakiridis Apr 20, 2016

Collaborator

@mehturt: sounds right 👍

@atsakiridis

This comment has been minimized.

Collaborator

atsakiridis commented Apr 19, 2016

@mehturt: just finished reviewing, please checkt

mehturt added some commits Apr 19, 2016

Reverted configuration changes.
The configuration is now done using HashMap, same as before, but I at
least added string constants for config parameter names
(RCDevice.Prefs).
@mehturt

This comment has been minimized.

mehturt commented Apr 21, 2016

@atsakiridis any more open issues?

@atsakiridis

This comment has been minimized.

Collaborator

atsakiridis commented Apr 22, 2016

@mehturt will try to review next week, thanks for the effort 👍

@@ -560,10 +563,19 @@ void initializeWebrtc(boolean videoEnabled)
for (String permission : MANDATORY_PERMISSIONS) {
if (context.checkCallingOrSelfPermission(permission) != PackageManager.PERMISSION_GRANTED) {
logAndToast("Permission " + permission + " is not granted");
// TODO: return error to RCConnection listener
listener.onPermissionNotGranted(this, permission);

This comment has been minimized.

@atsakiridis

atsakiridis Apr 25, 2016

Collaborator

@mehturt: Please prefer using existing error reporting, by utilizing onDisconnected(connection, errorCode, errorText), as can be seen in line 510 above, instead of introducing a new callback (i.e. onPermissionGranted). Reason for this is that we want to be compatible with Twilio's Android Client API, plus we keep the API simpler and expect RCConnection errors from a single interface.

This comment has been minimized.

@mehturt

mehturt Apr 25, 2016

OK, should I add new error code to RCClient.ErrorCodes or use an existing one?

This comment has been minimized.

@mehturt

mehturt Apr 25, 2016

I also think that RCConnection.connect() should return null when this fails (permission error, or some other error) - what do you think? Right now the onDisconnected() would be called, but the connect() still returns a non-null object, so the application will see non-null object and use that, because onDisconnected() would be executed before connect() is finished, correct?

This comment has been minimized.

@atsakiridis

atsakiridis Apr 26, 2016

Collaborator

Yes please add a new error code, like PERMISSION_NOT_GRANTED_MANIFEST_ERROR and maybe differentiate the error text between the audio and video case.

This comment has been minimized.

@atsakiridis

atsakiridis Apr 26, 2016

Collaborator

True, please return null from connect() to make it more clear. I will revisit the whole solution when I review the whole thing

This comment has been minimized.

@atsakiridis

atsakiridis May 12, 2016

Collaborator

Oh, but of course! I thought you were referring to the RCConnection state, my bad. Yes please update the RCDevice state to be READY again right before you notify the App that it has no suitable permissions, so that subsequent requests can be made without issues.

This comment has been minimized.

@mehturt

mehturt May 12, 2016

Ok, should be checked in now.

This comment has been minimized.

@atsakiridis

atsakiridis May 12, 2016

Collaborator

Great @mehturt, so you 're done with all changes from your side? Should I schedule a re-review?

This comment has been minimized.

@mehturt

mehturt May 12, 2016

Yes. I also planned to update the example application so that it compiles, but perhaps only after the SDK is reviewed.

This comment has been minimized.

@atsakiridis

atsakiridis May 12, 2016

Collaborator

Yeah, sounds like a plan, let's get done with the SDK before update the App

On Thu, May 12, 2016 at 1:41 PM, mehturt notifications@github.com wrote:

In
restcomm.android.client.sdk/src/main/java/org/mobicents/restcomm/android/client/sdk/RCConnection.java
#283 (comment)
:

@@ -560,10 +563,19 @@ void initializeWebrtc(boolean videoEnabled)
for (String permission : MANDATORY_PERMISSIONS) {
if (context.checkCallingOrSelfPermission(permission) != PackageManager.PERMISSION_GRANTED) {
logAndToast("Permission " + permission + " is not granted");

  •            // TODO: return error to RCConnection listener
    
  •            listener.onPermissionNotGranted(this, permission);
    

Yes. I also planned to update the example application so that it compiles,
but perhaps only after the SDK is reviewed.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/RestComm/restcomm-android-sdk/pull/283/files/7e91af930a845199f3ed94e753307d408df57d0f#r62999416

Antonis Tsakiridis
Lead Developer, Mobile SDKs at TeleStax
antonis.tsakiridis@telestax.com
http://www.telestax.com

@atsakiridis

This comment has been minimized.

Collaborator

atsakiridis commented Apr 25, 2016

@mehturt: added one more comment about error reporting, but I still need to review the logic of the new configuration options. Will ping you when I get around to it, as I'm very busy right now :(. Thanks again for your efforts!

@atsakiridis

This comment has been minimized.

Collaborator

atsakiridis commented May 3, 2016

@mehturt: please let me know when you have finished addressing the issues we discussed so that I can review the latest and we can move forward.

Thanks

mehturt added some commits May 3, 2016

Handling of permission error
Permission error now calls onDisconnected().
Also added some more pref constants.
Update device state when permissions are not granted.
Add return value to setupWebrtcAndCall() to be able to determine
status of the operation.  When it fails, device state is changed back
to READY, so that followup call of RCDevice.connect() tries to connect
again.
/**
* General status.
*/
public enum Status {

This comment has been minimized.

@atsakiridis

atsakiridis May 19, 2016

Collaborator

@mehturt how come you didn't just use a bool?

This comment has been minimized.

@mehturt

mehturt May 20, 2016

I don't really like to use bool for things which are not really true / false, yes / no, where it's not obvious from the method name what true and false means. So for methods like haveConnectivity() it would be ok, because true means we have connectivity, false means we don't. But for setupWebrtcAndCall() I would not be sure what true means.
But it's just me, you can change it if you want :)

This comment has been minimized.

@atsakiridis

atsakiridis May 25, 2016

Collaborator

I see your point, but on the other hand true / false also has a meaning of general success or not. Anyway, I would be ok to have this so long as we have followed this convention from the start, which I'm afraid isn't the case with this codebase so we 'll just end up having many ways to convey the same thing :(

This comment has been minimized.

@mehturt

mehturt May 30, 2016

Ok, good comment. So should I change it back to boolean?

This comment has been minimized.

@atsakiridis

atsakiridis May 30, 2016

Collaborator

Don't worry bout that will fix it when I do the final merge. Please check my last comment though

* @param connection Connection
* @param permission The permission that is not granted.
*/
public void onPermissionNotGranted(RCConnection connection, String permission);

This comment has been minimized.

@atsakiridis

atsakiridis May 19, 2016

Collaborator

@mehturt do you still need this? I thought we talked about using existing error reporting callbacks. Am I missing sth?

This comment has been minimized.

@mehturt

mehturt May 20, 2016

@atsakiridis sorry looks like I missed this, it's fixed now

@atsakiridis

This comment has been minimized.

Collaborator

atsakiridis commented May 19, 2016

@mehturt your changes look good :). I added a couple more comments, please have a look and then I think I can test them and after that integrate them with the SDK. The timing is great cause I was planning to start working on next Android release ;)

@atsakiridis

This comment has been minimized.

Collaborator

atsakiridis commented May 25, 2016

Great, resuming review

@atsakiridis

This comment has been minimized.

Collaborator

atsakiridis commented May 25, 2016

@mehturt: I merged your changes and did some small modifications to integrate the whole thing and pushed a new branch, called mehturt-master in our repo with the integrated result which builds successfully :). However, there seems to be an issue with the changes you did in Register.java. Please check:

https://github.com/RestComm/restcomm-android-sdk/pull/283/files#diff-ee139f14df707c0994a2d2f2243d3b0fR37

If the SipDomain also contains a port (which happens a lot) the registration breaks. Remember that we need to be backward compatible. Ideally we should have a way to parse the Domain and only return the host part, similarly to how we did it in SipProfile.getRemoteIp(). Can you please fix that as well?

Also, please use a new PR on top of mehturt-master branch to avoid having to re-merge again, ok? Here's the branch to create a PR from:

https://github.com/RestComm/restcomm-android-sdk/tree/mehturt-master

@atsakiridis

This comment has been minimized.

Collaborator

atsakiridis commented May 25, 2016

Closing this PR per last comment above

@deruelle deruelle added this to the 1.0.0 BETA5 release milestone May 25, 2016

@deruelle

This comment has been minimized.

Member

deruelle commented May 25, 2016

Thanks @mehturt. Your contribution has been acknowledged at https://telestax.com/acknowledgements/ !

@atsakiridis atsakiridis modified the milestones: 1.0.0 BETA5 release, CI step 1 Feb 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment