[Asterisk-code-review] res pjsip caller id: Anonymize 'From' when caller id present... (asterisk[master])

[Walter Doekes]

Walter Doekes has posted comments on this change.

Change subject: res_pjsip_caller_id: Anonymize 'From' when caller id presentation is prohibited
......................................................................


Patch Set 3:

(7 comments)

I think that should do it.

Going by the graph only, I'll say this:

- the graph looks good,
- the From header is consistent (regardless of the trust_id_outbound option),
- you'll be able to tell whether the callerid was probitited by caller name even if there is no PAI/RPID sent,
- it's consistent with the RFCs (no sending of the header if there is no trust) and with chan_sip (non-legacy mode),


Just a few nits below.

https://gerrit.asterisk.org/#/c/2294/3//COMMIT_MSG
Commit Message:

Line 30: Y   N               |YES    "Caller name" <sip:<caller_exten>@<ip_address>>
Consistency typo: "Caller Name" (leading uppercase)

Another one below.


https://gerrit.asterisk.org/#/c/2294/3/res/res_pjsip.c
File res/res_pjsip.c:

Line 3914: 	id_uri = pjsip_uri_get_uri(id_name_addr->uri);
You could move this line to before 3925.


https://gerrit.asterisk.org/#/c/2294/3/res/res_pjsip_caller_id.c
File res/res_pjsip_caller_id.c:

Line 497: 			add_privacy_header(tdata, id);
The add_privacy_header looks buggy in that it would keep "Privacy: none" when you would in fact want "Privacy: id". But perhaps that is beyond the scope of this fix.

(It only checks for the existence of the Privacy header, but not its value.)


Line 649:  * For an initial INVITE request, we may change the From header to appropriately
        :  * reflect the identity information. 
Updating From-header is not done here anymore.


https://gerrit.asterisk.org/#/c/2294/3/res/res_pjsip_session.c
File res/res_pjsip_session.c:

Line 1139: 	/* Now set up dlg->local.info so pjsip can correctly generate From */
         : 
         : 	id_name_addr = (pjsip_name_addr *) session->inv_session->dlg->local.info->uri;
         : 	id_uri = pjsip_uri_get_uri(id_name_addr);
Adding an alias/temporary pointer to both "dlg->local.info" and "dlg->pool" would be beneficial for readability here.

It wasn't immediately obvious to me that the updates to  "session->inv_session->dlg->local.info" on line 1146 are superseded by the updates of id_uri and id_name_addr below.


Line 1171: 	}
         : 
         : 	if (restricted &&
No need to exit the previous if{} here.


Line 1179: 	 * the invite message.  All future message should also have the correct From.
future message+s


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2c82a5ca1413c2c00fb62ea95b0ae8e97af54dc9
Gerrit-PatchSet: 3
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: George Joseph <george.joseph at fairview5.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: George Joseph <george.joseph at fairview5.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Walter Doekes <walter+asterisk at wjd.nu>
Gerrit-HasComments: Yes