Re: [asterisk-dev] Adding headers to NOTIFY responses

[Joshua C. Colp]

On Tue, Nov 15, 2022 at 7:12 PM  wrote:

> On 11/15/2022 9:56 AM, Joshua C. Colp wrote:
> > On Tue, Nov 15, 2022 at 10:50 AM  > > wrote:
> >
> >
> > If res_pjsip_pubsub would need to be extended to support this,
> > would it
> > reasonable to add a callback to a pubsub module that allows it
> > access to
> > the pjsip_tx_data, so it can do whatever it needs to with it,
> > before the
> > response gets sent? Or another preferred method of allowing
> > modules to
> > add headers?
> >
> >
> > At a surface it is probably fine.
>
> Thanks, doing that allowed just what I needed to do.
>
> Next limitation... the new_subscribe callback is supposed to return 200
> (or some other code) to accept or reject the subscription. The only
> arguments are the endpoint name and resource name.
> This is not really always sufficient; it may be necessary to approve or
> reject the subscription using some information present in the
> subscription itself (for example, a header). I think this is all
> consequent of the very narrow range of scenarios that res_pjsip_pubsub
> was written for originally.
>
> The subscription_established callback is actually perfectly set up for
> this. We have a handle on the ast_sip_subscription, and can call
> ast_sip_subscription_get_header if needed to get the header.
> However, this requires approving all subscriptions with a 200 in the
> new_subscribe callback, only to potentially realize it should be
> rejected in the subscription_established callback. This is too late
> because the 200 OK already gets sent to the endpoint before
> subscription_established gets called.
>
> So, the only good solution is to extend new_subscribe to accept a third
> argument: rdata, since a subscription hasn't yet been created at that
> point so we could not use ast_sip_subscription_get_header to fetch
> headers. Yuck, since it's a public API... there could also be a
> new_subscribe_with_rdata callback that gets executed instead if a module
> defines one. Or maybe we can break ABI and go master only here if that
> would be too inelegant.
>

Even adding a new callback will break the ABI. I think fundamentally this
work should only occur in master where changing ABI is fine. I have
concerns that it will cause unintended consequences in some way and I've
had enough of those in the past year in release branches.

-- 
Joshua C. Colp
Asterisk Project Lead
Sangoma Technologies
Check us out at www.sangoma.com and www.asterisk.org
-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] Adding headers to NOTIFY responses

[asterisk]

On 11/15/2022 9:56 AM, Joshua C. Colp wrote:
On Tue, Nov 15, 2022 at 10:50 AM > wrote:



If res_pjsip_pubsub would need to be extended to support this,
would it
reasonable to add a callback to a pubsub module that allows it
access to
the pjsip_tx_data, so it can do whatever it needs to with it,
before the
response gets sent? Or another preferred method of allowing
modules to
add headers?


At a surface it is probably fine.


Thanks, doing that allowed just what I needed to do.

Next limitation... the new_subscribe callback is supposed to return 200 
(or some other code) to accept or reject the subscription. The only 
arguments are the endpoint name and resource name.
This is not really always sufficient; it may be necessary to approve or 
reject the subscription using some information present in the 
subscription itself (for example, a header). I think this is all 
consequent of the very narrow range of scenarios that res_pjsip_pubsub 
was written for originally.


The subscription_established callback is actually perfectly set up for 
this. We have a handle on the ast_sip_subscription, and can call 
ast_sip_subscription_get_header if needed to get the header.
However, this requires approving all subscriptions with a 200 in the 
new_subscribe callback, only to potentially realize it should be 
rejected in the subscription_established callback. This is too late 
because the 200 OK already gets sent to the endpoint before 
subscription_established gets called.


So, the only good solution is to extend new_subscribe to accept a third 
argument: rdata, since a subscription hasn't yet been created at that 
point so we could not use ast_sip_subscription_get_header to fetch 
headers. Yuck, since it's a public API... there could also be a 
new_subscribe_with_rdata callback that gets executed instead if a module 
defines one. Or maybe we can break ABI and go master only here if that 
would be too inelegant.


In any case, the problem could be solved by passing rdata into all of 
the functions in res_pjsip_pubsub that call the new_subscribe callback, 
since it's not currently available in those functions.
I think the only reason it isn't done this way now is so that 
new_subscribe can be unit tested, without having a pjsip_rx_data. The 
unit test could simply pass in NULL for rdata, though, since it won't 
actually need it for what it does (nor would any existing modules).


Any thoughts/objections?
Thanks!

--
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
  http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] Adding headers to NOTIFY responses

[Joshua C. Colp]

On Tue, Nov 15, 2022 at 10:50 AM  wrote:

> I am working on something where I need to be able to add a custom header
> to an outgoing NOTIFY response for a subscription.
>
> We have the concept of "body generators" for adding stuff to the body of
> an outgoing NOTIFY, but I'm not seeing anything akin to "header
> generators" that would easily allow doing this.
>
> I believe typically this is done by adding the header to the tdata pool,
> e.g. ast_sip_add_header(tdata, "Expires", buf) as one way of doing it.
>

To be specific, it allocates a header FROM the tdata pool and sets it on
tdata.


>
> However, res_pjsip_pubsub doesn't generate/initialize a pjsip_tx_data
> until after ast_sip_subscription_notify is already called, so it
> certainly can't be done before a pubsub module calls on the pubsub core
> to send the notify. Is there any way I've missed that allows us to
> access the tdata from a pubsub module, so we can add a header to the
> NOTIFY response?
>

No.


>
> I do see there is an ast_sip_subscription_response_data struct defined
> in res_pjsip_pubsub.h. However, this is used literally nowhere in the
> code, only defined, so it's not clear to me how this is supposed to be
> used, since nothing currently uses it. Was this intended to be used for
> this purpose, or something else?
>

What you see is the information there is.


>
> If res_pjsip_pubsub would need to be extended to support this, would it
> reasonable to add a callback to a pubsub module that allows it access to
> the pjsip_tx_data, so it can do whatever it needs to with it, before the
> response gets sent? Or another preferred method of allowing modules to
> add headers?
>

At a surface it is probably fine.

-- 
Joshua C. Colp
Asterisk Project Lead
Sangoma Technologies
Check us out at www.sangoma.com and www.asterisk.org
-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev