[reSIProcate] RE: ServerAuthManager ?
Some comments inline:
Note: I'm adding the developer list to the CC list - so if anyone else
had any comments - feel free.
> -----Original Message-----
> From: Daniel Pocock [mailto:daniel@xxxxxxxxxxxxxxxxxxxxx]
> Sent: Wednesday, February 22, 2006 12:56 PM
> To: Jason Fischl; Scott Godin
> Subject: Re: ServerAuthManager ?
>
> I've started working on async, I've also reported a new issue that I
> will fix as part of the async patch:
>
> http://track.sipfoundry.org/browse/RSP-29
[Scott] We should be really careful about this. We should only issue a
407 if we haven't already done so for this request. If we don't do this
check we could get into an endless loop with poorly behaved UA's. ie.
UA's that always just send a bad realm, when challenged.
> It occurs to me that the current `requiresChallenge()' method gets
> called twice - once when the original request arrives, and a second
time
> when the request with ProxyAuthorization arrives. I might re-arrange
> things a little to avoid calling requiresChallenge() until the
challenge
> is just about to be issued. This will avoid duplicating all the
> requests when operating in async mode.
[Scott] Sounds good. - In fact I will fix the double call today - since
repro is using this already.
> It also occured to me that the logic for ACK and CANCEL could
> potentially be put into the requiresChallenge method in the base
class.
> Would this be neater than the current logic?
[Scott] I don't really like this - it will force an inherited method to
ensure that the base method is called in order to avoid challenging
ACK's and CANCEL's. Not challenging these is part of the RFC behaviour
(section 22.1) - so they are probably best left out of that method.
> I hope no one else is doing any heavy changes to
ServerAuthManager.cxx,
> as this won't be a trivial patch.
>
> >
> >On 2/20/06, Scott Godin <slgodin@xxxxxxxxxxxx> wrote:
> >
> >
> >>Sounds good to me. I don't think we should force the asynchronous
> >>version though - for efficiency reasons when it is not required.
> >>
> >>Note: I've just finished committing the other virtual method
changes to
> >>ServerAuthManager.
> >>
> >>Scott
> >>
> >>
> >>
> >>>-----Original Message-----
> >>>From: Daniel Pocock [mailto:daniel@xxxxxxxxxxxxxxxxxxxxx]
> >>>Sent: Monday, February 20, 2006 4:05 PM
> >>>To: Scott Godin
> >>>Cc: Jason Fischl
> >>>Subject: Re: ServerAuthManager ?
> >>>
> >>>
> >>>
> >>>Scott Godin wrote:
> >>>
> >>>>Daniel,
> >>>>
> >>>>a) sounds good I will add this
> >>>>b) sounds like a good idea - but I will save this one for future
work.
> >>>>
> >>>I'm willing to attempt b), should I just subclass DumFeatureMessage
in
> >>>the way you have implemented UserAuthInfo? Perhaps I would create
a
> >>>class `UserAuthRequirement' that has a method bool
isAuthRequired().
> >>>
> >>>Then everything could go in a single patch, and nobody will start
coding against an API that will be changing. We have a rather immediate
need for this functionality too, so I'll probably have it done in 24
hours.
> >>>
> >>>Regards,
> >>>
> >>>Daniel
> >>>
> >>>>>-----Original Message-----
> >>>>>From: Daniel Pocock [mailto:daniel@xxxxxxxxxxxxxxxxxxxxx]
> >>>>>Sent: Monday, February 20, 2006 11:59 AM
> >>>>>To: Scott Godin
> >>>>>Cc: Jason Fischl
> >>>>>Subject: Re: ServerAuthManager ?
> >>>>>
> >>>>>a) mDum.isMyDomain(it->param(p_realm))
> >>>>>The above call could be replaced with a call to a virtual
function,
> >>>>>`isChallengeDomain(Data& domain)'. The default implementation
would
> >>>>>call mDum.isMyDomain(domain)
> >>>>>
> >>>>>b) aynchronous responses - do you think any of these virtual
methods
> >>>>>should be able to respond asynchronously? In particular,
> >>>>>requiresChallenge() might need to consult a database to determine
if
> >>>>the[Scott] source IP of the message is on a list of trusted
peers. The database lookup may take time, so the request might need to
be [Scott] queued/dealt with later.