Improve testing of SRTP external auth code paths.

Improve testing of SRTP external auth code paths.

Previously code behind ENABLE_EXTERNAL_AUTH was only compiled with Chromium
but developed in WebRTC, which made testing rather complicated. This caused
some trouble in the past (e.g. https://crbug.com/628400#c1)

This CL helps in that the external auth code is now compiled with WebRTC
and the srtpfilter integration gets tested.

BUG=chromium:628400
Review-Url: https://codereview.webrtc.org/2722423003
Cr-Commit-Position: refs/heads/master@{#17030}
Committed: https://chromium.googlesource.com/external/webrtc/+/ac170d5c214e1f487ba82b888e5c3d5000118de0

[joachim, 2017-03-03 00:32:58]

jbauch@webrtc.org changed reviewers:
+ deadbeef@webrtc.org

[joachim, 2017-03-03 00:32:59]

Ptal, this is a follow-up to https://codereview.webrtc.org/2720663003/

[Taylor Brandstetter, 2017-03-03 02:20:35]

https://codereview.webrtc.org/2722423003/diff/1/webrtc/pc/srtpfilter.cc
File webrtc/pc/srtpfilter.cc (right):

https://codereview.webrtc.org/2722423003/diff/1/webrtc/pc/srtpfilter.cc#newco...
webrtc/pc/srtpfilter.cc:57: #endif
Another option, which I slightly prefer, would be to have two methods called
"EnableExternalAuth" and "ExternalAuthEnabled", and put the #ifdef in
webrtcsession.cc where it constructs the SrtpFilter.

#ifdef ENABLE_EXTERNAL_AUTH
srtp_filter_.EnableExternalAuth();
#endif

That way no "for testing" method would be needed, and the SrtpFilter unit tests
would succeed even when ENABLE_EXTERNAL_AUTH is defined.

https://codereview.webrtc.org/2722423003/diff/1/webrtc/pc/srtpfilter.h
File webrtc/pc/srtpfilter.h (right):

https://codereview.webrtc.org/2722423003/diff/1/webrtc/pc/srtpfilter.h#newcod...
webrtc/pc/srtpfilter.h:120: // it has been explicitly activated for tests.
Is there any comment in general explaining what external auth is? If not it
would be nice, in case someone encounters this code without context.

https://codereview.webrtc.org/2722423003/diff/1/webrtc/pc/srtpfilter_unittest.cc
File webrtc/pc/srtpfilter_unittest.cc (right):

https://codereview.webrtc.org/2722423003/diff/1/webrtc/pc/srtpfilter_unittest...
webrtc/pc/srtpfilter_unittest.cc:110: void
TestRtpAuthParams(cricket::SrtpFilter* filter, const std::string cs) {
nit: Think you meant to use const ref.

https://codereview.webrtc.org/2722423003/diff/1/webrtc/pc/srtpfilter_unittest...
webrtc/pc/srtpfilter_unittest.cc:171: TestRtpAuthParams(&f2_, cs2);
Can you leave a comment explaining why this method is called when external auth
is enabled?

https://codereview.webrtc.org/2722423003/diff/1/webrtc/pc/srtpfilter_unittest...
webrtc/pc/srtpfilter_unittest.cc:605: TestProtectSetParamsDirect(false,
rtc::SRTP_AES128_CM_SHA1_80,
nit: We often do this:

bool external_auth = false;
TestSetParamsDirect(external_auth, ...

To make it more clear what the boolean parameter means.

Also, you don't have to do it, but this would be a good place to use
parameterized tests: 
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...

https://codereview.webrtc.org/2722423003/diff/1/webrtc/pc/srtpfilter_unittest...
webrtc/pc/srtpfilter_unittest.cc:669: TEST_F(SrtpFilterTest,
TestGetSendAuthParams) {
Is this test still necessary, now that the above tests will test
GetRtpAuthParams?

[joachim, 2017-03-03 20:42:57]

Thanks for your feedback! All comments addressed, ptal.

https://codereview.webrtc.org/2722423003/diff/1/webrtc/pc/srtpfilter.cc
File webrtc/pc/srtpfilter.cc (right):

https://codereview.webrtc.org/2722423003/diff/1/webrtc/pc/srtpfilter.cc#newco...
webrtc/pc/srtpfilter.cc:57: #endif
On 2017/03/03 02:20:34, Taylor Brandstetter wrote:
> Another option, which I slightly prefer, would be to have two methods called
> "EnableExternalAuth" and "ExternalAuthEnabled", and put the #ifdef in
> webrtcsession.cc where it constructs the SrtpFilter.
> 
> #ifdef ENABLE_EXTERNAL_AUTH
> srtp_filter_.EnableExternalAuth();
> #endif

Done (gets enabled in channel.cc). I used "IsExternalAuthEnabled" to match other
methods that check for something.

> That way no "for testing" method would be needed, and the SrtpFilter unit
tests
> would succeed even when ENABLE_EXTERNAL_AUTH is defined.

Yes, that's better - the define is only used in channel.cc now.

https://codereview.webrtc.org/2722423003/diff/1/webrtc/pc/srtpfilter.h
File webrtc/pc/srtpfilter.h (right):

https://codereview.webrtc.org/2722423003/diff/1/webrtc/pc/srtpfilter.h#newcod...
webrtc/pc/srtpfilter.h:120: // it has been explicitly activated for tests.
On 2017/03/03 02:20:34, Taylor Brandstetter wrote:
> Is there any comment in general explaining what external auth is? If not it
> would be nice, in case someone encounters this code without context.

There is a comment in channel.cc (BaseChannel::SendPacket) where the code is
different depending on whether external auth is active.

Added another comment here.

https://codereview.webrtc.org/2722423003/diff/1/webrtc/pc/srtpfilter_unittest.cc
File webrtc/pc/srtpfilter_unittest.cc (right):

https://codereview.webrtc.org/2722423003/diff/1/webrtc/pc/srtpfilter_unittest...
webrtc/pc/srtpfilter_unittest.cc:110: void
TestRtpAuthParams(cricket::SrtpFilter* filter, const std::string cs) {
On 2017/03/03 02:20:35, Taylor Brandstetter wrote:
> nit: Think you meant to use const ref.

Done.

https://codereview.webrtc.org/2722423003/diff/1/webrtc/pc/srtpfilter_unittest...
webrtc/pc/srtpfilter_unittest.cc:171: TestRtpAuthParams(&f2_, cs2);
On 2017/03/03 02:20:35, Taylor Brandstetter wrote:
> Can you leave a comment explaining why this method is called when external
auth
> is enabled?

Done.

https://codereview.webrtc.org/2722423003/diff/1/webrtc/pc/srtpfilter_unittest...
webrtc/pc/srtpfilter_unittest.cc:605: TestProtectSetParamsDirect(false,
rtc::SRTP_AES128_CM_SHA1_80,
On 2017/03/03 02:20:35, Taylor Brandstetter wrote:
> nit: We often do this:
> 
> bool external_auth = false;
> TestSetParamsDirect(external_auth, ...
> 
> To make it more clear what the boolean parameter means.
> 
> Also, you don't have to do it, but this would be a good place to use
> parameterized tests: 
>
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...

Done.

https://codereview.webrtc.org/2722423003/diff/1/webrtc/pc/srtpfilter_unittest...
webrtc/pc/srtpfilter_unittest.cc:669: TEST_F(SrtpFilterTest,
TestGetSendAuthParams) {
On 2017/03/03 02:20:35, Taylor Brandstetter wrote:
> Is this test still necessary, now that the above tests will test
> GetRtpAuthParams?

True, the same is already tested above.

[Taylor Brandstetter, 2017-03-03 23:03:57]

lgtm

https://codereview.webrtc.org/2722423003/diff/1/webrtc/pc/srtpfilter.cc
File webrtc/pc/srtpfilter.cc (right):

https://codereview.webrtc.org/2722423003/diff/1/webrtc/pc/srtpfilter.cc#newco...
webrtc/pc/srtpfilter.cc:57: #endif
On 2017/03/03 20:42:57, joachim wrote:
> On 2017/03/03 02:20:34, Taylor Brandstetter wrote:
> > Another option, which I slightly prefer, would be to have two methods called
> > "EnableExternalAuth" and "ExternalAuthEnabled", and put the #ifdef in
> > webrtcsession.cc where it constructs the SrtpFilter.
> > 
> > #ifdef ENABLE_EXTERNAL_AUTH
> > srtp_filter_.EnableExternalAuth();
> > #endif
> 
> Done (gets enabled in channel.cc). I used "IsExternalAuthEnabled" to match
other
> methods that check for something.
> 
> > That way no "for testing" method would be needed, and the SrtpFilter unit
> tests
> > would succeed even when ENABLE_EXTERNAL_AUTH is defined.
> 
> Yes, that's better - the define is only used in channel.cc now.

Yep, channel.cc is what I meant, sorry for the mixup.

[joachim, 2017-03-04 08:33:22]

The CQ bit was checked by jbauch@webrtc.org

[commit-bot: I haz the power, 2017-03-04 08:33:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-04 08:54:45]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1488616402439640,
"parent_rev": "ca818683fc8b12e921d3c15c22adff8a865c87d0", "commit_rev":
"ac170d5c214e1f487ba82b888e5c3d5000118de0"}

[commit-bot: I haz the power, 2017-03-04 08:54:48]

Description was changed from

==========
Improve testing of SRTP external auth code paths.

Previously code behind ENABLE_EXTERNAL_AUTH was only compiled with Chromium
but developed in WebRTC, which made testing rather complicated. This caused
some trouble in the past (e.g. https://crbug.com/628400#c1)

This CL helps in that the external auth code is now compiled with WebRTC
and the srtpfilter integration gets tested.

BUG=chromium:628400
==========

to

==========
Improve testing of SRTP external auth code paths.

Previously code behind ENABLE_EXTERNAL_AUTH was only compiled with Chromium
but developed in WebRTC, which made testing rather complicated. This caused
some trouble in the past (e.g. https://crbug.com/628400#c1)

This CL helps in that the external auth code is now compiled with WebRTC
and the srtpfilter integration gets tested.

BUG=chromium:628400

Review-Url: https://codereview.webrtc.org/2722423003
Cr-Commit-Position: refs/heads/master@{#17030}
Committed:
https://chromium.googlesource.com/external/webrtc/+/ac170d5c214e1f487ba82b888...
==========

[commit-bot: I haz the power, 2017-03-04 08:54:50]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/external/webrtc/+/ac170d5c214e1f487ba82b888...

[joachim, 2017-03-04 09:37:13]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.webrtc.org/2734643002/ by jbauch@webrtc.org.

The reason for reverting is: Breaks compilation in FYI bots, e.g. here:
http://build.chromium.org/p/chromium.webrtc.fyi/builders/Win%20Builder/builds...

FAILED: obj/third_party/webrtc/pc/rtc_pc/channel.obj 
ninja -t msvc -e environment.x86 -- E:\b\c\goma_client/gomacc.exe
"E:\b\depot_tools\win_toolchain\vs_files\d3cb0e37bdd120ad0ac4650b674b09e81be45616\VC\bin\amd64_x86/cl.exe"
/nologo /showIncludes /FC @obj/third_party/webrtc/pc/rtc_pc/channel.obj.rsp /c
../../third_party/webrtc/pc/channel.cc
/Foobj/third_party/webrtc/pc/rtc_pc/channel.obj
/Fd"obj/third_party/webrtc/pc/rtc_pc_cc.pdb"
e:\b\c\b\win_builder\src\third_party\webrtc\pc\channel.cc(176): error C2819:
type 'cricket::SrtpFilter' does not have an overloaded member 'operator ->'
e:\b\c\b\win_builder\src\third_party\webrtc\pc\srtpfilter.h(45): note: see
declaration of 'cricket::SrtpFilter'
e:\b\c\b\win_builder\src\third_party\webrtc\pc\channel.cc(176): note: did you
intend to use '.' instead?
e:\b\c\b\win_builder\src\third_party\webrtc\pc\channel.cc(176): error C2232:
'->cricket::SrtpFilter::EnableExternalAuth': left operand has 'class' type, use
'.'.