[Asterisk-code-review] res xmpp: Google OAuth 2.0 protocol support for XMPP / Motif (asterisk[13])

[Joshua Colp]

Joshua Colp has posted comments on this change. ( https://gerrit.asterisk.org/6158 )

Change subject: res_xmpp: Google OAuth 2.0 protocol support for XMPP / Motif
......................................................................


Patch Set 3: Code-Review-1

(9 comments)

https://gerrit.asterisk.org/#/c/6158/3/configs/samples/xmpp.conf.sample
File configs/samples/xmpp.conf.sample:

https://gerrit.asterisk.org/#/c/6158/3/configs/samples/xmpp.conf.sample@24
PS3, Line 24: ;oauth_secret=OAUTH_SECRET_VALUE	; The application's secret to authorize using Google OAuth 2.0 protocol.
            : 					; Create new one on https://console.cloud.google.com/apis/credentials/oauthclient
I think we need a more complete guide on how to actually use this for users.


https://gerrit.asterisk.org/#/c/6158/3/res/res_xmpp.c
File res/res_xmpp.c:

https://gerrit.asterisk.org/#/c/6158/3/res/res_xmpp.c@3658
PS3, Line 3658: 		ast_log(LOG_NOTICE , "Connecting to client token: %s\n" , clientcfg->refresh_token);
I don't think this should be a notice but instead a debug message, and it's also not completely useful in its current form. This should also include the client name.


https://gerrit.asterisk.org/#/c/6158/3/res/res_xmpp.c@3904
PS3, Line 3904: static void fetch_access_token(struct ast_xmpp_client_config *cfg) {
The { should be on the next line.


https://gerrit.asterisk.org/#/c/6158/3/res/res_xmpp.c@3912
PS3, Line 3912: 	snprintf(cmd, sizeof(cmd) - 1 , "CURL(%s,client_id=%s&client_secret=%s&refresh_token=%s&grant_type=refresh_token)",
ast_asprintf can be used here to produce the cmd, so that a 4k buffer doesn't need to be used.


https://gerrit.asterisk.org/#/c/6158/3/res/res_xmpp.c@3915
PS3, Line 3915: 	ast_log(LOG_NOTICE, "Command %s\n" , cmd);
This appears to be a debug message while developing this.


https://gerrit.asterisk.org/#/c/6158/3/res/res_xmpp.c@3918
PS3, Line 3918: 	ast_func_read(NULL, cmd, cBuf, sizeof(cBuf) - 1);
This adds a dependency on CURL for this which isn't documented and should be better handled here - if the dependency isn't met then a message needs to be output so the user understands what is going on.


https://gerrit.asterisk.org/#/c/6158/3/res/res_xmpp.c@3919
PS3, Line 3919: 	ast_log(LOG_NOTICE, "Command status: %s\n", cBuf);
This appears to be a debug message while developing this.


https://gerrit.asterisk.org/#/c/6158/3/res/res_xmpp.c@3925
PS3, Line 3925: 			ast_string_field_set(cfg, password, token);
Is the token guaranteed to be the same or less than the existing token? If not this can have a memory growth issue. String fields will only reuse the existing memory if the string is less than or equal to the length of the existing string that is there. If not then more memory is used, and the other allocation goes unused until the stringfields are released.


https://gerrit.asterisk.org/#/c/6158/3/res/res_xmpp.c@3926
PS3, Line 3926: 			ast_log(LOG_NOTICE, "Access Token: %s\n", token);
              : 		} else {
              : 			ast_log(LOG_ERROR, "OAuth object is wrong\n");
              : 		}
              : 	} else {
              : 		ast_log(LOG_ERROR, "OAuth object is NULL\n");
These log messages aren't useful to users for when something goes wrong. It doesn't elaborate on what really went wrong or how to solve the problem.



-- 
To view, visit https://gerrit.asterisk.org/6158
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: comment
Gerrit-Change-Id: I07f7052a502457ab55010a4d3686653b60f4c8db
Gerrit-Change-Number: 6158
Gerrit-PatchSet: 3
Gerrit-Owner: Andrey <andr06 at gmail.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Comment-Date: Mon, 07 Aug 2017 11:13:46 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20170807/b0a7b812/attachment-0001.html>