Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12326090:
Third Party authentication protocol. (Closed)
|
Created:
7 years, 10 months ago by rmsousa Modified:
7 years, 9 months ago CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, simonmorris+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, sail+watch_chromium.org, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, rmsousa+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@host_key_pair Visibility:
Public. |
DescriptionThird Party authentication protocol.
This adds a new Authenticator, that uses a third-party token service to authenticate clients and hosts and negotiate a shared secret.
The client authenticates with the third-party service, and obtains a token and a shared secret. The token is sent directly to the host, the shared secret is used to initiate a SPAKE authentication.
The host receives the token, and asks the third-party server to exchange it for the shared secret (authenticating itself by signing the request with the host private key). Once it gets the shared secret, client and host are able to finish the SPAKE negotiation.
BUG=115899
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190024
Patch Set 1 #Patch Set 2 : Add the missing new files #
Total comments: 85
Patch Set 3 : Rebase with async process message, separate client and host #Patch Set 4 : TokenFetcher/Validator ownership, move parameters to Validator, remove client/host glue #
Total comments: 60
Patch Set 5 : Split authenticator into base, client, host #
Total comments: 30
Patch Set 6 : Rebase #Patch Set 7 : Reviewer comments #Patch Set 8 : Missing moved file. #
Total comments: 24
Patch Set 9 : Reviewer comments #
Total comments: 72
Patch Set 10 : Reviewer comments #Patch Set 11 : REviewer comments #Messages
Total messages: 18 (0 generated)
Hi Wez, This is the protocol part of the Third Party authentication, moved to a separate CL to ease reviewing. It is built on top of https://codereview.chromium.org/12316083/.
On 2013/02/23 03:18:31, rmsousa wrote: > Hi Wez, > > This is the protocol part of the Third Party authentication, moved to a separate > CL to ease reviewing. It is built on top of > https://codereview.chromium.org/12316083/. nit: Capitalization of "party". Also, please update the description to explain what the CL changes to implement the third-party auth protocol, and include a BUG #.
I suggest separating implementation into two classes - one for host and one for client (and maybe one base class for common parts). It would make this code much easier to read. We used that approach for V1 authenticators. I would usually land a change like this in two CLs: one for implementation and one for integration (i.e. changes in the NegotiatingAuthenticator will go into the second one). https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/negotiat... File remoting/protocol/negotiating_authenticator.h (right): https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/negotiat... remoting/protocol/negotiating_authenticator.h:51: const std::string& third_party_token_url, Maybe define a type that stores all four parameters for the third_party auth and pass them as one argument. E.g. call it ThirdPartyAuthenticator::Config? https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... File remoting/protocol/third_party_authenticator.cc (right): https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... remoting/protocol/third_party_authenticator.cc:17: #if defined(_WIN32) && defined(GetMessage) Do you need this here? https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... remoting/protocol/third_party_authenticator.cc:30: remove extra empty line. https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... remoting/protocol/third_party_authenticator.cc:64: result->expecting_token_ = false; don't need this - it should be already initialized in the constructor. https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... remoting/protocol/third_party_authenticator.cc:68: ThirdPartyAuthenticator::ThirdPartyAuthenticator( Would it make sense two have separate classes - one for Client and one for Host? The protocol is not symmetric (unlike in V2Authenticator), so it may simplify code significantly. If you think that there is still a lot of code shared between the two sides then it may make sense to define a class that contains common logic (I would call it LayeredAuthenticator and put there everything related to wrapping another Authenticator implementation). https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... remoting/protocol/third_party_authenticator.cc:115: if (state_ == WAITING_MESSAGE) { there is a DCHECK for this condition at the top, so you don't really need this if here.. https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... remoting/protocol/third_party_authenticator.cc:156: if (state_ == MESSAGE_READY) { same here - there is a DCHECK for this condition. https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... remoting/protocol/third_party_authenticator.cc:207: if (is_host_side()) { Here the order of client and host parts is reversed from the one used in other methods. Better make it consistent. https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... remoting/protocol/third_party_authenticator.cc:230: const base::Closure& resume_callback, const std::string& third_party_token, nit: one argument per line please when whole function definition doesn't fit in one line. http://dev.chromium.org/developers/coding-style#Code_Formatting https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... remoting/protocol/third_party_authenticator.cc:245: const base::Closure& resume_callback, const std::string& shared_secret) { one argument per line please https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... File remoting/protocol/third_party_authenticator.h (right): https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... remoting/protocol/third_party_authenticator.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. nit: 2013 please https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... remoting/protocol/third_party_authenticator.h:22: class ThirdPartyAuthenticator : public Authenticator { Add comment to explain what this class is for. https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... remoting/protocol/third_party_authenticator.h:24: class TokenFetcher { Need virtual destructor for a class with virtual methods. https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... remoting/protocol/third_party_authenticator.h:32: virtual void FetchThirdPartyToken( I think you also need a way to cancel the request - for the case Authenticator object gets deleted while this request is pending. Also see my comment for ~TokenValidator() https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... remoting/protocol/third_party_authenticator.h:33: const std::string& token_url, maybe use GURL type for the URLs? https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... remoting/protocol/third_party_authenticator.h:36: const base::Callback<void( please add typedef for the callback - that would make this code more readable and allow to document is separately. https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... remoting/protocol/third_party_authenticator.h:38: const std::string& shared_secret)>&on_token_fetched) = 0; space before argument name https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... remoting/protocol/third_party_authenticator.h:50: virtual void ValidateThirdPartyToken( Also need a way to cancel the request. https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... remoting/protocol/third_party_authenticator.h:55: const std::string& scope, Do we expect the content of this string to be in some format? Maybe better to add a class that parses a scope and use it here. https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... remoting/protocol/third_party_authenticator.h:56: const base::Callback<void( typedef for the callback please https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... remoting/protocol/third_party_authenticator.h:58: virtual ~TokenValidator() {} nit: does the destructor need to be public? if not make it protected. If you make destructor public then you can use it as a way to cancel the request, but then the Authenticator should take ownership of the implementation of this interface. IMO that would be the best way to implement request cancellation. https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... remoting/protocol/third_party_authenticator.h:74: // Creates a third-party host authenticator. |local_cert| is used to establish Why do we need to pass local_cert? Can't it be generated using KeyPair::GenerateCertificate(). https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... remoting/protocol/third_party_authenticator.h:83: const std::string& local_cert, Why do we need local_cert here together with key_pair? Can't we generate local_cert from the key pair? https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... remoting/protocol/third_party_authenticator.h:87: const std::string& scope, this argument is called token_scope in .cc file.
Initial comments; apologies for the delay. https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/authenti... File remoting/protocol/authenticator.h (right): https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/authenti... remoting/protocol/authenticator.h:42: // WAITING_MESSAGE -> WAITING_EXTERNAL I don't like "external" - can we call this WAITING_LOCAL, WAITING_ACTION, WAITING_USER or something along those lines? I realise we won't always actually need an action from the user but this is where user-interaction fits in so I think it'd be reasonable. https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/authenti... remoting/protocol/authenticator.h:103: virtual void PerformExternalAction(const base::Closure& resume_callback); nit: We don't provide a no-message default impl for GetNextMessage, so I'm not sure we need one here? Rather than having a separate PerformExternalAction API, could we add a completion callback to ProcessMessage(), and letting that return true if it processed the message immediately, or false if the caller needs to wait for the completion callback? The caller really doesn't care that the Authenticator is performing some other activity as part of the auth process. https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... File remoting/protocol/third_party_authenticator.cc (right): https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... remoting/protocol/third_party_authenticator.cc:46: // static nit: Blank line, plz https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... remoting/protocol/third_party_authenticator.cc:68: ThirdPartyAuthenticator::ThirdPartyAuthenticator( On 2013/02/26 01:14:50, sergeyu wrote: > Would it make sense two have separate classes - one for Client and one for Host? > The protocol is not symmetric (unlike in V2Authenticator), so it may simplify > code significantly. If you think that there is still a lot of code shared > between the two sides then it may make sense to define a class that contains > common logic (I would call it LayeredAuthenticator and put there everything > related to wrapping another Authenticator implementation). Agreed. You don't really need a public class for this at all, just a pair of constructor functions, and internally a shared Base, and Client and Host implementations derived from it. Not sure that there's enough really generic common logic to pull out a LayeredAuthenticator base, though? https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... remoting/protocol/third_party_authenticator.cc:82: if (state_ == ACCEPTED) { nit: No need for {} for simple single-line if. https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... remoting/protocol/third_party_authenticator.cc:115: if (state_ == WAITING_MESSAGE) { On 2013/02/26 01:14:50, sergeyu wrote: > there is a DCHECK for this condition at the top, so you don't really need this > if here.. Is the caller responsible for never calling ProcessMessage() when we're not in the WAITING_MESSAGE state? Or could an extra message sent by the client cause us to be called in the wrong state? https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... remoting/protocol/third_party_authenticator.cc:118: // First message from the host, token URL and scope. nit: Make the above a sentence. Also, please move it after the DCHECK, to match the layout below. super-nit: I also prefer blocks of DCHECKs to be separated from code by a blank line, here and elsewhere. https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... remoting/protocol/third_party_authenticator.cc:129: if (!expecting_token_) { Should this be |has_sent_urls| instead? https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... remoting/protocol/third_party_authenticator.cc:130: // The host hasn't send the token URLs to the client yet, so ignore the typo: send -> sent https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... remoting/protocol/third_party_authenticator.cc:164: // waiting for the host to send the token_url. Surely the client should never be in the MESSAGE_READY state if there's nothing to send, so this is a NOTREACHED? https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... remoting/protocol/third_party_authenticator.cc:182: if (underlying_ && underlying_->state() == MESSAGE_READY) { nit: Blank lines before & after this if...else... chunk https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... File remoting/protocol/third_party_authenticator.h (right): https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... remoting/protocol/third_party_authenticator.h:25: public: Define a virtual dtor for the interface. Since it's lifetime is not recipient-managed that probably needs to be protected. https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... remoting/protocol/third_party_authenticator.h:30: // |on_token_fetched| is called by the TokenFetcher once |token| and Clarify which thread the callback is invoked on - must it be the thread the call is made on? https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... remoting/protocol/third_party_authenticator.h:32: virtual void FetchThirdPartyToken( On 2013/02/26 01:14:50, sergeyu wrote: > I think you also need a way to cancel the request - for the case Authenticator > object gets deleted while this request is pending. > > Also see my comment for ~TokenValidator() You don't necessarily need a cancel method if the Callback is Bind()ed to a WeakPtr to the Authenticator. https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... remoting/protocol/third_party_authenticator.h:38: const std::string& shared_secret)>&on_token_fetched) = 0; nit: Space between & and member name. nit: Best to define a typedef for the Callback (e.g. TokenFetcher::Callback) https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... remoting/protocol/third_party_authenticator.h:38: const std::string& shared_secret)>&on_token_fetched) = 0; nit: on_token_fetched -> fetch_callback or just callback https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... remoting/protocol/third_party_authenticator.h:57: const std::string& shared_secret)>& on_token_validated) = 0; nit: on_token_validated -> validation_callback or just callback https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... remoting/protocol/third_party_authenticator.h:71: ThirdPartyAuthenticator::TokenFetcher* token_fetcher, Does this need to out-live, or could it be scoped_ptr? https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... remoting/protocol/third_party_authenticator.h:77: // used by its |TokenFetcher|. |token_validation_url| is used by Why are |token_url|, |token_validator|, |scope|, |key_pair| passed here rather than to the TokenValidator implementation ctor, which the caller has already had to call? https://codereview.chromium.org/12326090/diff/3001/remoting/protocol/third_pa... remoting/protocol/third_party_authenticator.h:109: const std::string& shared_secret); nit: Separate these methods by blank lines and add a comment to each of the On* methods to explain what they do (it's clear from the name when they are called :)
PTAL. I removed the client/host glue from this CL (negotiating_authenticator and *2me_authenticator_factory), since they make more sense together with the appropriate implementations. https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/a... File remoting/protocol/authenticator.h (right): https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/a... remoting/protocol/authenticator.h:42: // WAITING_MESSAGE -> WAITING_EXTERNAL On 2013/02/27 07:05:30, Wez wrote: > I don't like "external" - can we call this WAITING_LOCAL, WAITING_ACTION, > WAITING_USER or something along those lines? I realise we won't always actually > need an action from the user but this is where user-interaction fits in so I > think it'd be reasonable. Done in another CL. https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/a... remoting/protocol/authenticator.h:103: virtual void PerformExternalAction(const base::Closure& resume_callback); On 2013/02/27 07:05:30, Wez wrote: > nit: We don't provide a no-message default impl for GetNextMessage, so I'm not > sure we need one here? > > Rather than having a separate PerformExternalAction API, could we add a > completion callback to ProcessMessage(), and letting that return true if it > processed the message immediately, or false if the caller needs to wait for the > completion callback? The caller really doesn't care that the Authenticator is > performing some other activity as part of the auth process. Done in another CL. https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/n... File remoting/protocol/negotiating_authenticator.h (right): https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/n... remoting/protocol/negotiating_authenticator.h:51: const std::string& third_party_token_url, On 2013/02/26 01:14:50, sergeyu wrote: > Maybe define a type that stores all four parameters for the third_party auth and > pass them as one argument. E.g. call it ThirdPartyAuthenticator::Config? They can actually all go inside the validator. https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/t... File remoting/protocol/third_party_authenticator.cc (right): https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/t... remoting/protocol/third_party_authenticator.cc:17: #if defined(_WIN32) && defined(GetMessage) On 2013/02/26 01:14:50, sergeyu wrote: > Do you need this here? Done. https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/t... remoting/protocol/third_party_authenticator.cc:30: On 2013/02/26 01:14:50, sergeyu wrote: > remove extra empty line. Done. https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/t... remoting/protocol/third_party_authenticator.cc:46: // static On 2013/02/27 07:05:30, Wez wrote: > nit: Blank line, plz Done. https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/t... remoting/protocol/third_party_authenticator.cc:64: result->expecting_token_ = false; On 2013/02/26 01:14:50, sergeyu wrote: > don't need this - it should be already initialized in the constructor. Done. https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/t... remoting/protocol/third_party_authenticator.cc:68: ThirdPartyAuthenticator::ThirdPartyAuthenticator( On 2013/02/26 01:14:50, sergeyu wrote: > Would it make sense two have separate classes - one for Client and one for Host? > The protocol is not symmetric (unlike in V2Authenticator), so it may simplify > code significantly. If you think that there is still a lot of code shared > between the two sides then it may make sense to define a class that contains > common logic (I would call it LayeredAuthenticator and put there everything > related to wrapping another Authenticator implementation). Done. https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/t... remoting/protocol/third_party_authenticator.cc:68: ThirdPartyAuthenticator::ThirdPartyAuthenticator( On 2013/02/27 07:05:30, Wez wrote: > On 2013/02/26 01:14:50, sergeyu wrote: > > Would it make sense two have separate classes - one for Client and one for > Host? > > The protocol is not symmetric (unlike in V2Authenticator), so it may simplify > > code significantly. If you think that there is still a lot of code shared > > between the two sides then it may make sense to define a class that contains > > common logic (I would call it LayeredAuthenticator and put there everything > > related to wrapping another Authenticator implementation). > > Agreed. > > You don't really need a public class for this at all, just a pair of constructor > functions, and internally a shared Base, and Client and Host implementations > derived from it. > > Not sure that there's enough really generic common logic to pull out a > LayeredAuthenticator base, though? I separated in one public class plus two local subclasses, but kept them all in this file, since most of the implementation is common, and having the client/host in the same file may make it easier to follow the protocol. Let me know if you prefer them separate. https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/t... remoting/protocol/third_party_authenticator.cc:115: if (state_ == WAITING_MESSAGE) { On 2013/02/26 01:14:50, sergeyu wrote: > there is a DCHECK for this condition at the top, so you don't really need this > if here.. the DCHECK is for state(), which is the authenticator's aggregate state (taking into account the underlying state) - that is, ProcessMessage must be called when *someone* has a message. The if block then handles the case where the token autehtnicator itself is waiting for a message. In any case, hopefully the refactor made the distinction clearer. https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/t... remoting/protocol/third_party_authenticator.cc:115: if (state_ == WAITING_MESSAGE) { On 2013/02/27 07:05:30, Wez wrote: > On 2013/02/26 01:14:50, sergeyu wrote: > > there is a DCHECK for this condition at the top, so you don't really need this > > if here.. > > Is the caller responsible for never calling ProcessMessage() when we're not in > the WAITING_MESSAGE state? Or could an extra message sent by the client cause us > to be called in the wrong state? The caller is responsible for not calling if state() is not WAITING_MESSAGE, but see comment about the DCHECK below. https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/t... remoting/protocol/third_party_authenticator.cc:129: if (!expecting_token_) { On 2013/02/27 07:05:30, Wez wrote: > Should this be |has_sent_urls| instead? Done. https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/t... remoting/protocol/third_party_authenticator.cc:130: // The host hasn't send the token URLs to the client yet, so ignore the On 2013/02/27 07:05:30, Wez wrote: > typo: send -> sent Done. https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/t... remoting/protocol/third_party_authenticator.cc:164: // waiting for the host to send the token_url. On 2013/02/27 07:05:30, Wez wrote: > Surely the client should never be in the MESSAGE_READY state if there's nothing > to send, so this is a NOTREACHED? We expect an authentication message on the session-initiate, which comes from the client. Since the client in this protocol doesn't have anything to send, it needs to send an empty authentication message. We can change negotiating_authenticator to not pick an algorithm in the client's first message, but that's a large-ish separate change (it will also involve some client JS code, to ask for the PIN after the connection is started rather than before connecting) https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/t... remoting/protocol/third_party_authenticator.cc:182: if (underlying_ && underlying_->state() == MESSAGE_READY) { On 2013/02/27 07:05:30, Wez wrote: > nit: Blank lines before & after this if...else... chunk Done. https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/t... remoting/protocol/third_party_authenticator.cc:207: if (is_host_side()) { On 2013/02/26 01:14:50, sergeyu wrote: > Here the order of client and host parts is reversed from the one used in other > methods. Better make it consistent. Done. https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/t... remoting/protocol/third_party_authenticator.cc:230: const base::Closure& resume_callback, const std::string& third_party_token, On 2013/02/26 01:14:50, sergeyu wrote: > nit: one argument per line please when whole function definition doesn't fit in > one line. > http://dev.chromium.org/developers/coding-style#Code_Formatting Done. https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/t... remoting/protocol/third_party_authenticator.cc:245: const base::Closure& resume_callback, const std::string& shared_secret) { On 2013/02/26 01:14:50, sergeyu wrote: > one argument per line please Done. https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/t... File remoting/protocol/third_party_authenticator.h (right): https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/t... remoting/protocol/third_party_authenticator.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/02/26 01:14:50, sergeyu wrote: > nit: 2013 please Done. https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/t... remoting/protocol/third_party_authenticator.h:22: class ThirdPartyAuthenticator : public Authenticator { On 2013/02/26 01:14:50, sergeyu wrote: > Add comment to explain what this class is for. Done. https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/t... remoting/protocol/third_party_authenticator.h:24: class TokenFetcher { On 2013/02/26 01:14:50, sergeyu wrote: > Need virtual destructor for a class with virtual methods. Done. https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/t... remoting/protocol/third_party_authenticator.h:25: public: On 2013/02/27 07:05:30, Wez wrote: > Define a virtual dtor for the interface. Since it's lifetime is not > recipient-managed that probably needs to be protected. Done. https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/t... remoting/protocol/third_party_authenticator.h:30: // |on_token_fetched| is called by the TokenFetcher once |token| and On 2013/02/27 07:05:30, Wez wrote: > Clarify which thread the callback is invoked on - must it be the thread the call > is made on? Done. https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/t... remoting/protocol/third_party_authenticator.h:32: virtual void FetchThirdPartyToken( On 2013/02/27 07:05:30, Wez wrote: > On 2013/02/26 01:14:50, sergeyu wrote: > > I think you also need a way to cancel the request - for the case Authenticator > > object gets deleted while this request is pending. > > > > Also see my comment for ~TokenValidator() > > You don't necessarily need a cancel method if the Callback is Bind()ed to a > WeakPtr to the Authenticator. Still useful to have a way to cancel explicitly - e.g. in case this is doing an URLFetch, we can proactively cancel the fetch instead of waiting for it to finish to find that the callback is invalidated. In fact, by having this own the fetcher/validator, we can actually do away with the weakptr. https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/t... remoting/protocol/third_party_authenticator.h:33: const std::string& token_url, On 2013/02/26 01:14:50, sergeyu wrote: > maybe use GURL type for the URLs? Done. https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/t... remoting/protocol/third_party_authenticator.h:36: const base::Callback<void( On 2013/02/26 01:14:50, sergeyu wrote: > please add typedef for the callback - that would make this code more readable > and allow to document is separately. Done. https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/t... remoting/protocol/third_party_authenticator.h:38: const std::string& shared_secret)>&on_token_fetched) = 0; On 2013/02/26 01:14:50, sergeyu wrote: > space before argument name Done. https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/t... remoting/protocol/third_party_authenticator.h:38: const std::string& shared_secret)>&on_token_fetched) = 0; On 2013/02/27 07:05:30, Wez wrote: > nit: Space between & and member name. > nit: Best to define a typedef for the Callback (e.g. TokenFetcher::Callback) Done. https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/t... remoting/protocol/third_party_authenticator.h:38: const std::string& shared_secret)>&on_token_fetched) = 0; On 2013/02/27 07:05:30, Wez wrote: > nit: on_token_fetched -> fetch_callback or just callback Done. https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/t... remoting/protocol/third_party_authenticator.h:50: virtual void ValidateThirdPartyToken( On 2013/02/26 01:14:50, sergeyu wrote: > Also need a way to cancel the request. Done. https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/t... remoting/protocol/third_party_authenticator.h:55: const std::string& scope, On 2013/02/26 01:14:50, sergeyu wrote: > Do we expect the content of this string to be in some format? Maybe better to > add a class that parses a scope and use it here. The only ones that need to know about the string contents are the validator and the fetcher - the authenticator is only a pass-through for it, and needs it to be a pretty much opaque string. I've moved everything that can be determined at tokenvalidator construction time into the validator (and out of this class's constructors). Namely: scope, urls, keypair. https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/t... remoting/protocol/third_party_authenticator.h:56: const base::Callback<void( On 2013/02/26 01:14:50, sergeyu wrote: > typedef for the callback please Done. https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/t... remoting/protocol/third_party_authenticator.h:56: const base::Callback<void( On 2013/02/26 01:14:50, sergeyu wrote: > typedef for the callback please Done. https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/t... remoting/protocol/third_party_authenticator.h:57: const std::string& shared_secret)>& on_token_validated) = 0; On 2013/02/27 07:05:30, Wez wrote: > nit: on_token_validated -> validation_callback or just callback Done. https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/t... remoting/protocol/third_party_authenticator.h:58: virtual ~TokenValidator() {} On 2013/02/26 01:14:50, sergeyu wrote: > nit: does the destructor need to be public? if not make it protected. > > If you make destructor public then you can use it as a way to cancel the > request, but then the Authenticator should take ownership of the implementation > of this interface. IMO that would be the best way to implement request > cancellation. Done. https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/t... remoting/protocol/third_party_authenticator.h:71: ThirdPartyAuthenticator::TokenFetcher* token_fetcher, On 2013/02/27 07:05:30, Wez wrote: > Does this need to out-live, or could it be scoped_ptr? Done. https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/t... remoting/protocol/third_party_authenticator.h:74: // Creates a third-party host authenticator. |local_cert| is used to establish On 2013/02/26 01:14:50, sergeyu wrote: > Why do we need to pass local_cert? Can't it be generated using > KeyPair::GenerateCertificate(). Isn't generating certificates expensive? (If I understood our SSL code correctly, in some OSes each generated certificate will actually have to go into the system cert store) The current code generates a certificate once when RemotingMe2MeHost starts, and uses the same one for every connection - I just kept that behavior. If we want to generate one certificate for every connection, we should do it for every authenticator (i.e. change v2_authenticator to also do it, and remove local_cert from every constructor signature). https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/t... remoting/protocol/third_party_authenticator.h:77: // used by its |TokenFetcher|. |token_validation_url| is used by On 2013/02/27 07:05:30, Wez wrote: > Why are |token_url|, |token_validator|, |scope|, |key_pair| passed here rather > than to the TokenValidator implementation ctor, which the caller has already had > to call? key_pair needs to be here to construct the underlying v2_authenticator (I don't want to bake in an assumption that the token_validator necessarily has a key pair, so I prefer that we pass an explicit reference to the keypair here, even if it's ultimately the same) For the token parameters, yeah, good point, I've moved them into the validator. https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/t... remoting/protocol/third_party_authenticator.h:83: const std::string& local_cert, On 2013/02/26 01:14:50, sergeyu wrote: > Why do we need local_cert here together with key_pair? Can't we generate > local_cert from the key pair? see above https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/t... remoting/protocol/third_party_authenticator.h:87: const std::string& scope, On 2013/02/26 01:14:50, sergeyu wrote: > this argument is called token_scope in .cc file. Done. https://chromiumcodereview.appspot.com/12326090/diff/3001/remoting/protocol/t... remoting/protocol/third_party_authenticator.h:109: const std::string& shared_secret); On 2013/02/27 07:05:30, Wez wrote: > nit: Separate these methods by blank lines and add a comment to each of the On* > methods to explain what they do (it's clear from the name when they are called > :) Done.
Haven't looked at everything yet. Have a quick comment about classes layout. https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... File remoting/protocol/third_party_authenticator.cc (right): https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.cc:20: "third-party-token-url" }; nit: indent https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.cc:30: class ClientThirdPartyAuthenticator : public ThirdPartyAuthenticator { Why does this need to be in .cc file? I suggest renaming ThirdPartyAuthenticator to ThirdPartyAuthenticatorBase and this class should be called ThirdPartyClientAuthenticator and placed in third_party_client_authenticator.[cc|h]. Similarly for the host side. Then you also won't need static Create() methods as these classes can be created directly.
https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... File remoting/protocol/authentication_method.cc (right): https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/authentication_method.cc:95: return "third_party"; nit: No need for {} on single-line if. https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/authentication_method.cc:116: return ToString() == other.ToString(); nit: This creates two std::strings on every compare, which seems extravagant - consider comparing the two fields explicitly. https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/authentication_method.cc:134: return false; nit: This shouldn't have {} either, for consistency. https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... File remoting/protocol/third_party_authenticator.cc (right): https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.cc:34: Authenticator::State initial_state); nit: Need public virtual dtor. https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.cc:53: class HostThirdPartyAuthenticator : public ThirdPartyAuthenticator { nit: I'd normally define one class and implement its methods, then define the other and implement it, and finally follow with the public API implementations. https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.cc:58: Authenticator::State initial_state); nit: Need public virtual dtor. https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.cc:70: const std::string& shared_secret); nit: Blank line after this. https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.cc:135: DCHECK(underlying_->state() == WAITING_MESSAGE); If these are DCHECKs then will ProcessMessage() do the right thing if unexpectedly called in Release code? https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.cc:147: message = CreateEmptyAuthenticatorMessage(); How can we be in the MESSAGE_READY state with no message ready? https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.cc:172: std::string token_url = message->TextNamed(kTokenUrlTag); What happens if we manage to see two consecutive messages, unexpectedly, e.g. host mis-behaves? Do we have a test for that case for the code that drives the Authenticator? https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.cc:176: // |token_fetcher_| is owned, so Unretained() is safe here. nit: Blank line before this comment. https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.cc:188: return; nit: No need for return here. https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.cc:202: state_ = WAITING_MESSAGE; Again, we shouldn't ever reach the MESSAGE_READY state if there's no message ready... https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.cc:239: // first message and send the URLs to the client. Why are we receiving a first message if we're not expecting one? Sounds like we should be starting in the MESSAGE_READY state? https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.cc:246: // Host has already sent the URL and expects a token from the client. Blank line before this comment. https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.cc:250: // This message also contains the client's first SPAKE message. Copy the nit: Blank line before this comment. https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.cc:253: // |token_validator_| is owned, so Unretained() is safe here. Is there ever a possibility that we'd want to have the token & SPAKE messages separate, in which case there would be a race between the first SPAKE message and validation completing? https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.cc:266: return; No need for return here. https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.cc:293: // The other side already started the SPAKE authentication. nit: Clarify that we know this because the message containing the token also contains the first SPAKE message. https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... File remoting/protocol/third_party_authenticator.h (right): https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.h:12: #include "base/memory/weak_ptr.h" Remove this include. https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.h:23: // authentication of both client and host. Suggest "Authenticator implementation that authenticates client and host via a third-party." https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.h:30: // an authentication key, which is used to establish the connection. nit: This looks like it should break down into a set of steps, e.g: C->S: Request token authentication from Server. S->C: Return token & secret. C->H: Pass token to Host. H->S: Sign token & pass to Server. S->H: Pass secret to Host. C<->H: SPAKE2 handshake. (Except with better wording ;) Expressing things this way makes it clearer how many round-trips are involved, for example. https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.h:37: // directly to the host. |shared_secret| should be used by the client to nit: "... will be passed ..." https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.h:38: // create a V2Authenticator. In case of failure, the callback is called with nit: "... will be used at the client to create a V2Authenticator."? i.e. document this as just another part of the ThirdPartyAuthenticator interface, so document what ThirdPartyAuthenticator requires of the TokenFetcher implementation. https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.h:52: // The request is canceled if the TokenFetcher is destroyed. See above, re documenting in terms of what the ThirdyPartyAuthenticator expects of the TokenFetcher impl. https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.h:65: // an empty |shared_secret|. See above re comments. https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.h:80: virtual const GURL& token_url() const = 0; nit: Blank line after this. https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.h:87: // |host_public_key|. |token_fetcher| must outlive this authenticator. |token_fetcher| is being passed, so it's lifetime is out of the caller's control. nit: Document the use of |initial_state|. https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.h:96: // and is used to obtain the shared secret. nit: "... used by ... " -> "... passed to ..." You don't really need the comment re |token_validator| - that should be implicit from the comments on the TokenValidator interface. nit: Document the use of |initial_state|. https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.h:115: explicit ThirdPartyAuthenticator(State initial_state); nit: Blank line after this to separate it from the methods https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.h:122: virtual void GetNextMessageInternal(buzz::XmlElement* message) = 0; nit: Add comments documenting how/why these are overridden. https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.h:124: // Used for both host and client authenticators. nit: Suggest "State comment to client and host authenticators" https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.h:127: RejectionReason rejection_reason_; nit: Should these be private and accessible only via protected setters? https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... File remoting/protocol/third_party_authenticator_unittest.cc (right): https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator_unittest.cc:40: class FakeTokenFetcher nit: Specify visibility explicitly. https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator_unittest.cc:68: virtual ~FakeTokenValidator() {} Indentation here & below. https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator_unittest.cc:105: token_validator_ = static_cast<FakeTokenValidator*>(token_validator.get()); nit: You could new & assign straight to the bare ptr, then wrap to a scoped_ptr<> to pass to CreateForHost. https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator_unittest.cc:109: scoped_ptr<protocol::ThirdPartyAuthenticator::TokenFetcher> nit: Blank line here to separate host & client setup blocks. https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator_unittest.cc:116: FakeTokenFetcher* token_fetcher_; nit: Add a comment to explain that these are owned by the |host_| and |client_| authenticators. https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator_unittest.cc:126: // ASSERT_EQ(Authenticator::PROCESSING_MESSAGE, client_->state()); Why are these commented out? https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator_unittest.cc:131: token_validator_->OnTokenValidated(kSharedSecret)); nit: Blank line after this https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator_unittest.cc:154: // The end result is that the client rejected the connection, since it nit: Blank line before this comment. https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator_unittest.cc:155: // couldn't fetch the secret. nit: Suggest: "The client must reject the connection, since no secret was returned to it." https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator_unittest.cc:168: // The end result is that the host rejected the token. nit: Suggest "The host must reject the connection, since the validator returned no shared secret." https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator_unittest.cc:179: // couldn't fetch the token. See suggestions above re wording. https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator_unittest.cc:195: // The end result is that the host rejected the fake authentication. Ditto https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator_unittest.cc:210: // The end result is that the host rejected the fake authentication. Ditto
Just replying to comments, I'll start preparing another CL with the NegotiatingAuthenticator/Client code changes to handle authenticators starting with a message from the host by default. https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... File remoting/protocol/third_party_authenticator.cc (right): https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.cc:20: "third-party-token-url" }; On 2013/03/05 04:09:11, sergeyu wrote: > nit: indent Done. https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.cc:30: class ClientThirdPartyAuthenticator : public ThirdPartyAuthenticator { On 2013/03/05 04:09:11, sergeyu wrote: > Why does this need to be in .cc file? I suggest renaming ThirdPartyAuthenticator > to ThirdPartyAuthenticatorBase and this class should be called > ThirdPartyClientAuthenticator and placed in > third_party_client_authenticator.[cc|h]. Similarly for the host side. > > Then you also won't need static Create() methods as these classes can be created > directly. Done. https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.cc:34: Authenticator::State initial_state); On 2013/03/05 22:55:53, Wez wrote: > nit: Need public virtual dtor. Done. https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.cc:53: class HostThirdPartyAuthenticator : public ThirdPartyAuthenticator { On 2013/03/05 22:55:53, Wez wrote: > nit: I'd normally define one class and implement its methods, then define the > other and implement it, and finally follow with the public API implementations. separated into different files. https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.cc:135: DCHECK(underlying_->state() == WAITING_MESSAGE); On 2013/03/05 22:55:53, Wez wrote: > If these are DCHECKs then will ProcessMessage() do the right thing if > unexpectedly called in Release code? The DCHECK means (and documents) that this code is only statically reachable in that particular state. If any Release code calls ProcessMessage() unexpectedly, that code has a bug. https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.cc:147: message = CreateEmptyAuthenticatorMessage(); On 2013/03/05 22:55:53, Wez wrote: > How can we be in the MESSAGE_READY state with no message ready? There are two states, the state for the token negotiation, and the state for the underlying V2Authenticator. The aggregate state is MESSAGE_READY if *one* of them has a message to send. https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.cc:172: std::string token_url = message->TextNamed(kTokenUrlTag); On 2013/03/05 22:55:53, Wez wrote: > What happens if we manage to see two consecutive messages, unexpectedly, e.g. > host mis-behaves? Do we have a test for that case for the code that drives the > Authenticator? JingleSession has code that explicitly rejects messages if the authenticator is not in the WAITING_MESSAGE state. There's no unit test for it. https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.cc:202: state_ = WAITING_MESSAGE; On 2013/03/05 22:55:53, Wez wrote: > Again, we shouldn't ever reach the MESSAGE_READY state if there's no message > ready... Please read the previous comments and replies about this. https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.cc:239: // first message and send the URLs to the client. On 2013/03/05 22:55:53, Wez wrote: > Why are we receiving a first message if we're not expecting one? Sounds like we > should be starting in the MESSAGE_READY state? NegotiatingAuthenticator has baked in assumptions about what state each side starts in. Changing that will be a large-ish CL (propagating all the way to the client JS code if done correctly). I'll prepare that separately. https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.cc:253: // |token_validator_| is owned, so Unretained() is safe here. On 2013/03/05 22:55:53, Wez wrote: > Is there ever a possibility that we'd want to have the token & SPAKE messages > separate, in which case there would be a race between the first SPAKE message > and validation completing? No. There are two possibilities here: (1) The client is able to create its V2authenticator, so it is able to include the first SPAKE message together with the token, and it would be a waste of a roundtrip not to do so. (2) The client is unable to create the V2Authenticator, in which it doesn't matter what the host does, because it knows it doesn't have anything else to send in a reply to the client that would help it create the V2Authenticator. If there's any case where the client doesn't create the V2Authenticator when it gets the token, it means it is using a different, incompatible protocol. There's nothing we can do here except reject (which is what will happen when the V2Authenticator on the host processes the empty SPAKE message) https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.cc:293: // The other side already started the SPAKE authentication. On 2013/03/05 22:55:53, Wez wrote: > nit: Clarify that we know this because the message containing the token also > contains the first SPAKE message. We aren't inspecting the message to see what it contains. We know it because it's part of the protocol. We send the message to the V2Authenticator, and if the V2Authenticator doesn't find what it's looking for it's a protocol error. https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... File remoting/protocol/third_party_authenticator.h (right): https://chromiumcodereview.appspot.com/12326090/diff/16007/remoting/protocol/... remoting/protocol/third_party_authenticator.h:12: #include "base/memory/weak_ptr.h" On 2013/03/05 22:55:53, Wez wrote: > Remove this include. Done.
https://codereview.chromium.org/12326090/diff/26001/remoting/protocol/third_p... File remoting/protocol/third_party_authenticator_base.cc (right): https://codereview.chromium.org/12326090/diff/26001/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_base.cc:21: const buzz::StaticQName ThirdPartyAuthenticatorBase::kTokenUrlTag = { nit: it's more readable if you move { to the next line. https://codereview.chromium.org/12326090/diff/26001/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_base.cc:45: const { nit: const must be next to closing bracket. Better to wrap function name: Authenticator::RejectionReason ThirdPartyAuthenticatorBase::rejection_reason() const { https://codereview.chromium.org/12326090/diff/26001/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_base.cc:51: return underlying_->rejection_reason(); Are we sure underlying_ exists here? Shouldn't this method be similar to state() (i.e. underlying is called only when state_ == ACCEPTED). https://codereview.chromium.org/12326090/diff/26001/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_base.cc:79: GetNextMessageInternal(message.get()); Why do we need to call this for messages that we get from underlying_? https://codereview.chromium.org/12326090/diff/26001/remoting/protocol/third_p... File remoting/protocol/third_party_authenticator_base.h (right): https://codereview.chromium.org/12326090/diff/26001/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_base.h:22: // Implements an authentication method that relies on a third party server for Update comments? https://codereview.chromium.org/12326090/diff/26001/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_base.h:33: // XML tag names for third party authentication fields. Do they need to be public? https://codereview.chromium.org/12326090/diff/26001/remoting/protocol/third_p... File remoting/protocol/third_party_authenticator_base_unittest.cc (right): https://codereview.chromium.org/12326090/diff/26001/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_base_unittest.cc:41: class ThirdPartyAuthenticatorTest : public AuthenticatorTestBase { This file should be called third_party_authenticator_unittest.cc https://codereview.chromium.org/12326090/diff/26001/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_base_unittest.cc:127: // ASSERT_EQ(Authenticator::PROCESSING_MESSAGE, client_->state()); remove commented code? https://codereview.chromium.org/12326090/diff/26001/remoting/protocol/third_p... File remoting/protocol/third_party_client_authenticator.cc (right): https://codereview.chromium.org/12326090/diff/26001/remoting/protocol/third_p... remoting/protocol/third_party_client_authenticator.cc:24: : ThirdPartyAuthenticatorBase(initial_state), nit: indent two more spaces. https://codereview.chromium.org/12326090/diff/26001/remoting/protocol/third_p... remoting/protocol/third_party_client_authenticator.cc:47: LOG(WARNING) << "Missing token issue URL/verification URL/scope."; better to log it as ERROR. Also suggest rewording as "Third-party authentication protocol error: missing token verification URL or scope." https://codereview.chromium.org/12326090/diff/26001/remoting/protocol/third_p... remoting/protocol/third_party_client_authenticator.cc:51: return; nit: don't need return statement here. https://codereview.chromium.org/12326090/diff/26001/remoting/protocol/third_p... remoting/protocol/third_party_client_authenticator.cc:56: if (state_ == MESSAGE_READY) { this should be a DCHECK. https://codereview.chromium.org/12326090/diff/26001/remoting/protocol/third_p... File remoting/protocol/third_party_host_authenticator.cc (right): https://codereview.chromium.org/12326090/diff/26001/remoting/protocol/third_p... remoting/protocol/third_party_host_authenticator.cc:24: : ThirdPartyAuthenticatorBase(initial_state), nit: indent 2 more spaces https://codereview.chromium.org/12326090/diff/26001/remoting/protocol/third_p... remoting/protocol/third_party_host_authenticator.cc:62: LOG(WARNING) << "Missing token."; change this to ERROR. Make the message more verbose, e.g. "Third-party authentication protocol error: missing token." https://codereview.chromium.org/12326090/diff/26001/remoting/protocol/third_p... remoting/protocol/third_party_host_authenticator.cc:71: if (state_ == MESSAGE_READY) { This should be a DCHECK(). Why would that method be called if we are not in MESSAGE_READY state? https://codereview.chromium.org/12326090/diff/26001/remoting/protocol/v2_auth... File remoting/protocol/v2_authenticator.h (right): https://codereview.chromium.org/12326090/diff/26001/remoting/protocol/v2_auth... remoting/protocol/v2_authenticator.h:41: const base::Closure& resume_callback) OVERRIDE; I think this was in a separate CL that you've already landed.
PTAL I based this on top of https://codereview.chromium.org/12518027/, so that the first message is always from the host to the client. I also changed it to the same approach as the PIN - of using a callback with a weakptr instead of a Fetcher object. I renamed some attributes and methods to clarify their usage. https://codereview.chromium.org/12326090/diff/26001/remoting/protocol/third_p... File remoting/protocol/third_party_authenticator_base.cc (right): https://codereview.chromium.org/12326090/diff/26001/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_base.cc:21: const buzz::StaticQName ThirdPartyAuthenticatorBase::kTokenUrlTag = { On 2013/03/07 21:20:41, sergeyu wrote: > nit: it's more readable if you move { to the next line. Done. https://codereview.chromium.org/12326090/diff/26001/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_base.cc:45: const { On 2013/03/07 21:20:41, sergeyu wrote: > nit: const must be next to closing bracket. Better to wrap function name: > Authenticator::RejectionReason > ThirdPartyAuthenticatorBase::rejection_reason() const { Done. https://codereview.chromium.org/12326090/diff/26001/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_base.cc:51: return underlying_->rejection_reason(); On 2013/03/07 21:20:41, sergeyu wrote: > Are we sure underlying_ exists here? Shouldn't this method be similar to state() > (i.e. underlying is called only when state_ == ACCEPTED). This can only be called when state() is REJECTED. This is only possible if either state_ is REJECTED, or underlying_->state() is REJECTED (in which case it necessarily exists). https://codereview.chromium.org/12326090/diff/26001/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_base.cc:79: GetNextMessageInternal(message.get()); On 2013/03/07 21:20:41, sergeyu wrote: > Why do we need to call this for messages that we get from underlying_? This adds the token related fields to the message. After the client gets the token/shared secret, it can immediately create the V2Authenticator. Its message to the host, thus, contains both the token related fields (in this case the token), and the spake related fields from the underlying authenticator. https://codereview.chromium.org/12326090/diff/26001/remoting/protocol/third_p... File remoting/protocol/third_party_authenticator_base_unittest.cc (right): https://codereview.chromium.org/12326090/diff/26001/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_base_unittest.cc:41: class ThirdPartyAuthenticatorTest : public AuthenticatorTestBase { On 2013/03/07 21:20:41, sergeyu wrote: > This file should be called third_party_authenticator_unittest.cc Done. https://codereview.chromium.org/12326090/diff/26001/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_base_unittest.cc:127: // ASSERT_EQ(Authenticator::PROCESSING_MESSAGE, client_->state()); On 2013/03/07 21:20:41, sergeyu wrote: > remove commented code? Uncommented https://codereview.chromium.org/12326090/diff/26001/remoting/protocol/third_p... File remoting/protocol/third_party_client_authenticator.cc (right): https://codereview.chromium.org/12326090/diff/26001/remoting/protocol/third_p... remoting/protocol/third_party_client_authenticator.cc:24: : ThirdPartyAuthenticatorBase(initial_state), On 2013/03/07 21:20:41, sergeyu wrote: > nit: indent two more spaces. Done. https://codereview.chromium.org/12326090/diff/26001/remoting/protocol/third_p... remoting/protocol/third_party_client_authenticator.cc:47: LOG(WARNING) << "Missing token issue URL/verification URL/scope."; On 2013/03/07 21:20:41, sergeyu wrote: > better to log it as ERROR. Also suggest rewording as "Third-party authentication > protocol error: missing token verification URL or scope." Done. https://codereview.chromium.org/12326090/diff/26001/remoting/protocol/third_p... remoting/protocol/third_party_client_authenticator.cc:51: return; On 2013/03/07 21:20:41, sergeyu wrote: > nit: don't need return statement here. Done. https://codereview.chromium.org/12326090/diff/26001/remoting/protocol/third_p... remoting/protocol/third_party_client_authenticator.cc:56: if (state_ == MESSAGE_READY) { On 2013/03/07 21:20:41, sergeyu wrote: > this should be a DCHECK. Done. https://codereview.chromium.org/12326090/diff/26001/remoting/protocol/third_p... File remoting/protocol/third_party_host_authenticator.cc (right): https://codereview.chromium.org/12326090/diff/26001/remoting/protocol/third_p... remoting/protocol/third_party_host_authenticator.cc:24: : ThirdPartyAuthenticatorBase(initial_state), On 2013/03/07 21:20:41, sergeyu wrote: > nit: indent 2 more spaces Done. https://codereview.chromium.org/12326090/diff/26001/remoting/protocol/third_p... remoting/protocol/third_party_host_authenticator.cc:62: LOG(WARNING) << "Missing token."; On 2013/03/07 21:20:41, sergeyu wrote: > change this to ERROR. Make the message more verbose, e.g. "Third-party > authentication protocol error: missing token." Done. https://codereview.chromium.org/12326090/diff/26001/remoting/protocol/third_p... remoting/protocol/third_party_host_authenticator.cc:71: if (state_ == MESSAGE_READY) { On 2013/03/07 21:20:41, sergeyu wrote: > This should be a DCHECK(). Why would that method be called if we are not in > MESSAGE_READY state? This if was moved to the caller. https://codereview.chromium.org/12326090/diff/26001/remoting/protocol/v2_auth... File remoting/protocol/v2_authenticator.h (right): https://codereview.chromium.org/12326090/diff/26001/remoting/protocol/v2_auth... remoting/protocol/v2_authenticator.h:41: const base::Closure& resume_callback) OVERRIDE; On 2013/03/07 21:20:41, sergeyu wrote: > I think this was in a separate CL that you've already landed. Done.
https://codereview.chromium.org/12326090/diff/43003/remoting/protocol/authent... File remoting/protocol/authentication_method.cc (right): https://codereview.chromium.org/12326090/diff/43003/remoting/protocol/authent... remoting/protocol/authentication_method.cc:94: if (requires_token_) { nit: drop {} https://codereview.chromium.org/12326090/diff/43003/remoting/protocol/authent... remoting/protocol/authentication_method.cc:116: return ToString() == other.ToString(); It's bad enough that we have this operator at all. It would be even worse if we make it slow. AuthenticationMethod() is a simple value class - comparing all members will give do what we need here. If you type member as I suggested this function can look as follows: return type_ == other.type_ && hash_function_ == other.hash_function_; No need to have ifs above. https://codereview.chromium.org/12326090/diff/43003/remoting/protocol/authent... File remoting/protocol/authentication_method.h (right): https://codereview.chromium.org/12326090/diff/43003/remoting/protocol/authent... remoting/protocol/authentication_method.h:65: protected: why? https://codereview.chromium.org/12326090/diff/43003/remoting/protocol/authent... remoting/protocol/authentication_method.h:71: bool requires_token_; Doesn't look like a good name. I suggest that instead of adding another boolean member here you add "enum Type" with three values (INVALID, SPAKE, THIRD_PARTY) and then you wouldn't need invalid_ https://codereview.chromium.org/12326090/diff/43003/remoting/protocol/third_p... File remoting/protocol/third_party_authenticator_base.cc (right): https://codereview.chromium.org/12326090/diff/43003/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_base.cc:50: } else { nit: Don't need else https://codereview.chromium.org/12326090/diff/43003/remoting/protocol/third_p... File remoting/protocol/third_party_authenticator_base.h (right): https://codereview.chromium.org/12326090/diff/43003/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_base.h:12: #include "googleurl/src/gurl.h" Don't need this. https://codereview.chromium.org/12326090/diff/43003/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_base.h:14: #include "third_party/libjingle/source/talk/xmllite/xmlelement.h" XmlElement can be forward-declared. https://codereview.chromium.org/12326090/diff/43003/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_base.h:18: class RsaKeyPair; not used in this file https://codereview.chromium.org/12326090/diff/43003/remoting/protocol/third_p... File remoting/protocol/third_party_client_authenticator.cc (right): https://codereview.chromium.org/12326090/diff/43003/remoting/protocol/third_p... remoting/protocol/third_party_client_authenticator.cc:65: const base::Closure& resume_callback, const std::string& third_party_token, nit: one argument per line please https://codereview.chromium.org/12326090/diff/43003/remoting/protocol/third_p... File remoting/protocol/third_party_client_authenticator.h (right): https://codereview.chromium.org/12326090/diff/43003/remoting/protocol/third_p... remoting/protocol/third_party_client_authenticator.h:13: #include "googleurl/src/gurl.h" Can forward-declare GURL https://codereview.chromium.org/12326090/diff/43003/remoting/protocol/third_p... remoting/protocol/third_party_client_authenticator.h:38: const TokenFetchedCallback& token_fetched_callback)> FetchTokenCallback; The fetcher will be fetching information from remote server. We do want to be able to cancel these requests when authenticator is destroyed (there might be a possibility of DoS attack if we just keep those requests lingering). So in this case it's better to have the interface (and maybe factory). https://codereview.chromium.org/12326090/diff/43003/remoting/protocol/third_p... File remoting/protocol/third_party_host_authenticator.cc (right): https://codereview.chromium.org/12326090/diff/43003/remoting/protocol/third_p... remoting/protocol/third_party_host_authenticator.cc:45: base::Unretained(this), base::Unretained() is not safe unless you have a way to cancel it.
https://codereview.chromium.org/12326090/diff/43003/remoting/protocol/authent... File remoting/protocol/authentication_method.cc (right): https://codereview.chromium.org/12326090/diff/43003/remoting/protocol/authent... remoting/protocol/authentication_method.cc:94: if (requires_token_) { On 2013/03/20 07:24:12, sergeyu wrote: > nit: drop {} Done. https://codereview.chromium.org/12326090/diff/43003/remoting/protocol/authent... remoting/protocol/authentication_method.cc:116: return ToString() == other.ToString(); On 2013/03/20 07:24:12, sergeyu wrote: > It's bad enough that we have this operator at all. It would be even worse if we > make it slow. AuthenticationMethod() is a simple value class - comparing all > members will give do what we need here. If you type member as I suggested this > function can look as follows: > return type_ == other.type_ && > hash_function_ == other.hash_function_; > No need to have ifs above. Done. https://codereview.chromium.org/12326090/diff/43003/remoting/protocol/authent... File remoting/protocol/authentication_method.h (right): https://codereview.chromium.org/12326090/diff/43003/remoting/protocol/authent... remoting/protocol/authentication_method.h:65: protected: On 2013/03/20 07:24:12, sergeyu wrote: > why? Likely some early design leftover. https://codereview.chromium.org/12326090/diff/43003/remoting/protocol/authent... remoting/protocol/authentication_method.h:71: bool requires_token_; On 2013/03/20 07:24:12, sergeyu wrote: > Doesn't look like a good name. I suggest that instead of adding another boolean > member here you add "enum Type" with three values (INVALID, SPAKE, THIRD_PARTY) > and then you wouldn't need invalid_ Done. https://codereview.chromium.org/12326090/diff/43003/remoting/protocol/third_p... File remoting/protocol/third_party_authenticator_base.cc (right): https://codereview.chromium.org/12326090/diff/43003/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_base.cc:50: } else { On 2013/03/20 07:24:12, sergeyu wrote: > nit: Don't need else Done. https://codereview.chromium.org/12326090/diff/43003/remoting/protocol/third_p... File remoting/protocol/third_party_authenticator_base.h (right): https://codereview.chromium.org/12326090/diff/43003/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_base.h:12: #include "googleurl/src/gurl.h" On 2013/03/20 07:24:12, sergeyu wrote: > Don't need this. Done. https://codereview.chromium.org/12326090/diff/43003/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_base.h:14: #include "third_party/libjingle/source/talk/xmllite/xmlelement.h" On 2013/03/20 07:24:12, sergeyu wrote: > XmlElement can be forward-declared. Done. https://codereview.chromium.org/12326090/diff/43003/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_base.h:18: class RsaKeyPair; On 2013/03/20 07:24:12, sergeyu wrote: > not used in this file Done. https://codereview.chromium.org/12326090/diff/43003/remoting/protocol/third_p... File remoting/protocol/third_party_client_authenticator.cc (right): https://codereview.chromium.org/12326090/diff/43003/remoting/protocol/third_p... remoting/protocol/third_party_client_authenticator.cc:65: const base::Closure& resume_callback, const std::string& third_party_token, On 2013/03/20 07:24:12, sergeyu wrote: > nit: one argument per line please Done. https://codereview.chromium.org/12326090/diff/43003/remoting/protocol/third_p... File remoting/protocol/third_party_client_authenticator.h (right): https://codereview.chromium.org/12326090/diff/43003/remoting/protocol/third_p... remoting/protocol/third_party_client_authenticator.h:13: #include "googleurl/src/gurl.h" On 2013/03/20 07:24:12, sergeyu wrote: > Can forward-declare GURL Done. https://codereview.chromium.org/12326090/diff/43003/remoting/protocol/third_p... remoting/protocol/third_party_client_authenticator.h:38: const TokenFetchedCallback& token_fetched_callback)> FetchTokenCallback; On 2013/03/20 07:24:12, sergeyu wrote: > The fetcher will be fetching information from remote server. We do want to be > able to cancel these requests when authenticator is destroyed (there might be a > possibility of DoS attack if we just keep those requests lingering). So in this > case it's better to have the interface (and maybe factory). The way it works, it's not really cancellable in the js layer - we fire and forget either a window.open() or a launchWebAuthFlow() (which also opens a new window), and then it comes back with the token when it's done. But yeah, it may be a good idea to leave the possibility open anyway. https://codereview.chromium.org/12326090/diff/43003/remoting/protocol/third_p... File remoting/protocol/third_party_host_authenticator.cc (right): https://codereview.chromium.org/12326090/diff/43003/remoting/protocol/third_p... remoting/protocol/third_party_host_authenticator.cc:45: base::Unretained(this), On 2013/03/20 07:24:12, sergeyu wrote: > base::Unretained() is not safe unless you have a way to cancel it. token_validator_ is owned. this object's destructor will destroy token_validator_, ensuring the callback can't run after that.
lgtm https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/authent... File remoting/protocol/authentication_method.cc (right): https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/authent... remoting/protocol/authentication_method.cc:82: } DCHECK_NE(type_, INVALID); https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/authent... remoting/protocol/authentication_method.cc:95: DCHECK(method_type_ == SPAKE2); nit: DCHECK_EQ https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/authent... remoting/protocol/authentication_method.cc:102: default: You should not have default case when there is a case for each enum value (it breaks the warning about not handled values when new enum values are added). https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/authent... remoting/protocol/authentication_method.cc:106: return "invalid"; Why do we need to return non-empty string here? https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/authent... File remoting/protocol/authentication_method.h (right): https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/authent... remoting/protocol/authentication_method.h:73: MethodType method_type_; nit: type_ https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... File remoting/protocol/third_party_authenticator_base.cc (right): https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_base.cc:62: DCHECK(token_state_ == ACCEPTED); DCHECK_EQ, here and below https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... File remoting/protocol/third_party_authenticator_unittest.cc (right): https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_unittest.cc:53: on_token_fetched_.Run(token, shared_secret); ASSERT_FALSE(on_token_fetched_.is_null()); https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_unittest.cc:54: on_token_fetched_.Reset(); nit: it's always good idea to call callbacks last. Here the callback may potentially delete the token fetcher object, and then Reset() would crash. The usual way to handle call and reset is to save the callback, reset it and only than call the saved one. https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_unittest.cc:77: on_token_validated_.Run(shared_secret); same as above https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... File remoting/protocol/third_party_client_authenticator.cc (right): https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... remoting/protocol/third_party_client_authenticator.cc:56: DCHECK(token_state_ == MESSAGE_READY); DCHECK_EQ https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... File remoting/protocol/third_party_host_authenticator.cc (right): https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... remoting/protocol/third_party_host_authenticator.cc:37: if (!token.empty()) { nit: better to handle the error case here first. https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... remoting/protocol/third_party_host_authenticator.cc:59: DCHECK(token_state_ == MESSAGE_READY); DCHECK_EQ
https://chromiumcodereview.appspot.com/12326090/diff/61001/remoting/protocol/... File remoting/protocol/authentication_method.h (right): https://chromiumcodereview.appspot.com/12326090/diff/61001/remoting/protocol/... remoting/protocol/authentication_method.h:30: enum HashFunction { Given that HashFunction only applies to SPAKE2, why not fold it in to MethodType and have four instead of three? https://chromiumcodereview.appspot.com/12326090/diff/61001/remoting/protocol/... File remoting/protocol/third_party_authenticator_base.cc (right): https://chromiumcodereview.appspot.com/12326090/diff/61001/remoting/protocol/... remoting/protocol/third_party_authenticator_base.cc:38: if (token_state_ == ACCEPTED) { nit: no need for {} https://chromiumcodereview.appspot.com/12326090/diff/61001/remoting/protocol/... remoting/protocol/third_party_authenticator_base.cc:48: if (token_state_ == REJECTED) { nit: no need for {} https://chromiumcodereview.appspot.com/12326090/diff/61001/remoting/protocol/... remoting/protocol/third_party_authenticator_base.cc:62: DCHECK(token_state_ == ACCEPTED); nit DCHECK_EQ https://chromiumcodereview.appspot.com/12326090/diff/61001/remoting/protocol/... remoting/protocol/third_party_authenticator_base.cc:64: DCHECK(underlying_->state() == WAITING_MESSAGE); nit: DCHECK_EQ https://chromiumcodereview.appspot.com/12326090/diff/61001/remoting/protocol/... remoting/protocol/third_party_authenticator_base.cc:78: if (token_state_ == MESSAGE_READY) { nit: no need for { https://chromiumcodereview.appspot.com/12326090/diff/61001/remoting/protocol/... remoting/protocol/third_party_authenticator_base.cc:78: if (token_state_ == MESSAGE_READY) { nit: blank lines before and after this if block to separate the token-specific stuff from the preceding underlying calls https://chromiumcodereview.appspot.com/12326090/diff/61001/remoting/protocol/... File remoting/protocol/third_party_authenticator_unittest.cc (right): https://chromiumcodereview.appspot.com/12326090/diff/61001/remoting/protocol/... remoting/protocol/third_party_authenticator_unittest.cc:47: const TokenFetchedCallback& token_fetched_callback) { nit: Is there anything about the other parameters that you can EXPECT here? https://chromiumcodereview.appspot.com/12326090/diff/61001/remoting/protocol/... remoting/protocol/third_party_authenticator_unittest.cc:73: on_token_validated_ = token_validated_callback; nit: indentation https://chromiumcodereview.appspot.com/12326090/diff/61001/remoting/protocol/... remoting/protocol/third_party_authenticator_unittest.cc:97: } nit: {} can be on one line since they're empty. https://chromiumcodereview.appspot.com/12326090/diff/61001/remoting/protocol/... remoting/protocol/third_party_authenticator_unittest.cc:130: token_validator_->OnTokenValidated(kSharedSecret)); nit: blank line before the comment below https://chromiumcodereview.appspot.com/12326090/diff/61001/remoting/protocol/... remoting/protocol/third_party_authenticator_unittest.cc:152: ASSERT_NO_FATAL_FAILURE(token_fetcher_->OnTokenFetched(kToken, "")); nit: blank line before the comment https://chromiumcodereview.appspot.com/12326090/diff/61001/remoting/protocol/... remoting/protocol/third_party_authenticator_unittest.cc:187: ASSERT_EQ(Authenticator::PROCESSING_MESSAGE, client_->state()); nit: remove the blank line above, for consistency with other cases https://chromiumcodereview.appspot.com/12326090/diff/61001/remoting/protocol/... remoting/protocol/third_party_authenticator_unittest.cc:202: ASSERT_EQ(Authenticator::PROCESSING_MESSAGE, client_->state()); nit: as above https://chromiumcodereview.appspot.com/12326090/diff/61001/remoting/protocol/... File remoting/protocol/third_party_client_authenticator.cc (right): https://chromiumcodereview.appspot.com/12326090/diff/61001/remoting/protocol/... remoting/protocol/third_party_client_authenticator.cc:36: std::string token_scope = message->TextNamed(kTokenScopeTag); nit: I think it'd be easier to read if the error case came first. https://chromiumcodereview.appspot.com/12326090/diff/61001/remoting/protocol/... remoting/protocol/third_party_client_authenticator.cc:38: token_state_ = PROCESSING_MESSAGE; nit: blank line before comment https://chromiumcodereview.appspot.com/12326090/diff/61001/remoting/protocol/... remoting/protocol/third_party_client_authenticator.cc:70: if (!token_.empty() && !shared_secret.empty()) { nit: Similarly, checking token.empty() || shared_secret.empty() would be (slightly) easier to read. https://chromiumcodereview.appspot.com/12326090/diff/61001/remoting/protocol/... File remoting/protocol/third_party_client_authenticator.h (right): https://chromiumcodereview.appspot.com/12326090/diff/61001/remoting/protocol/... remoting/protocol/third_party_client_authenticator.h:28: // an authentication key, which is used to establish the connection. Isn't this comment already present on the ThirdPartyAuthenticatorBase? https://chromiumcodereview.appspot.com/12326090/diff/61001/remoting/protocol/... remoting/protocol/third_party_client_authenticator.h:58: // Creates a third-party client authenticator, for the host with the given nit: remove comma https://chromiumcodereview.appspot.com/12326090/diff/61001/remoting/protocol/... File remoting/protocol/third_party_host_authenticator.cc (right): https://chromiumcodereview.appspot.com/12326090/diff/61001/remoting/protocol/... remoting/protocol/third_party_host_authenticator.cc:78: if (!shared_secret.empty()) { nit: as above, consider putting error case first https://chromiumcodereview.appspot.com/12326090/diff/61001/remoting/protocol/... remoting/protocol/third_party_host_authenticator.cc:78: if (!shared_secret.empty()) { nit: as above, handle the error case first and early-exit rather than if...else https://chromiumcodereview.appspot.com/12326090/diff/61001/remoting/protocol/... remoting/protocol/third_party_host_authenticator.cc:79: // The other side already started the SPAKE authentication. nit: as above, handle the error-case first and then early exit rather than if...else https://chromiumcodereview.appspot.com/12326090/diff/61001/remoting/protocol/... remoting/protocol/third_party_host_authenticator.cc:79: // The other side already started the SPAKE authentication. nit: as above, handle the error case first and early exit rather than if...else https://chromiumcodereview.appspot.com/12326090/diff/61001/remoting/protocol/... File remoting/protocol/third_party_host_authenticator.h (right): https://chromiumcodereview.appspot.com/12326090/diff/61001/remoting/protocol/... remoting/protocol/third_party_host_authenticator.h:43: virtual const GURL& token_url() const = 0; nit: blank line between this and comment
https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/authent... File remoting/protocol/authentication_method.cc (right): https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/authent... remoting/protocol/authentication_method.cc:82: } On 2013/03/22 05:58:43, sergeyu wrote: > DCHECK_NE(type_, INVALID); Done. https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/authent... remoting/protocol/authentication_method.cc:95: DCHECK(method_type_ == SPAKE2); On 2013/03/22 05:58:43, sergeyu wrote: > nit: DCHECK_EQ Done. https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/authent... remoting/protocol/authentication_method.cc:102: default: On 2013/03/22 05:58:43, sergeyu wrote: > You should not have default case when there is a case for each enum value (it > breaks the warning about not handled values when new enum values are added). Done. https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/authent... remoting/protocol/authentication_method.cc:106: return "invalid"; On 2013/03/22 05:58:43, sergeyu wrote: > Why do we need to return non-empty string here? No functional reason, it just makes it slightly easier to debug (LOG(ERROR) << method.ToString()). https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/authent... File remoting/protocol/authentication_method.h (right): https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/authent... remoting/protocol/authentication_method.h:30: enum HashFunction { On 2013/03/22 06:17:01, Wez wrote: > Given that HashFunction only applies to SPAKE2, why not fold it in to MethodType > and have four instead of three? > There are some explicit uses of the hashfunction enum (e.g. we call ApplyHashFunction as a static while setting the PIN for the host, also, the SharedSecretHash class below) https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/authent... remoting/protocol/authentication_method.h:73: MethodType method_type_; On 2013/03/22 05:58:43, sergeyu wrote: > nit: type_ Done. https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... File remoting/protocol/third_party_authenticator_base.cc (right): https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_base.cc:38: if (token_state_ == ACCEPTED) { On 2013/03/22 06:17:01, Wez wrote: > nit: no need for {} Done. https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_base.cc:48: if (token_state_ == REJECTED) { On 2013/03/22 06:17:01, Wez wrote: > nit: no need for {} Done. https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_base.cc:62: DCHECK(token_state_ == ACCEPTED); On 2013/03/22 06:17:01, Wez wrote: > nit DCHECK_EQ Done. https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_base.cc:62: DCHECK(token_state_ == ACCEPTED); On 2013/03/22 05:58:43, sergeyu wrote: > DCHECK_EQ, here and below Done. https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_base.cc:64: DCHECK(underlying_->state() == WAITING_MESSAGE); On 2013/03/22 06:17:01, Wez wrote: > nit: DCHECK_EQ Done. https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_base.cc:78: if (token_state_ == MESSAGE_READY) { On 2013/03/22 06:17:01, Wez wrote: > nit: blank lines before and after this if block to separate the token-specific > stuff from the preceding underlying calls Done. https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_base.cc:78: if (token_state_ == MESSAGE_READY) { On 2013/03/22 06:17:01, Wez wrote: > nit: blank lines before and after this if block to separate the token-specific > stuff from the preceding underlying calls Done. https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... File remoting/protocol/third_party_authenticator_unittest.cc (right): https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_unittest.cc:47: const TokenFetchedCallback& token_fetched_callback) { On 2013/03/22 06:17:01, Wez wrote: > nit: Is there anything about the other parameters that you can EXPECT here? Done. https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_unittest.cc:53: on_token_fetched_.Run(token, shared_secret); On 2013/03/22 05:58:43, sergeyu wrote: > ASSERT_FALSE(on_token_fetched_.is_null()); Done. https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_unittest.cc:54: on_token_fetched_.Reset(); On 2013/03/22 05:58:43, sergeyu wrote: > nit: it's always good idea to call callbacks last. Here the callback may > potentially delete the token fetcher object, and then Reset() would crash. The > usual way to handle call and reset is to save the callback, reset it and only > than call the saved one. Done. https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_unittest.cc:73: on_token_validated_ = token_validated_callback; On 2013/03/22 06:17:01, Wez wrote: > nit: indentation Done. https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_unittest.cc:77: on_token_validated_.Run(shared_secret); On 2013/03/22 05:58:43, sergeyu wrote: > same as above Done. https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_unittest.cc:97: } On 2013/03/22 06:17:01, Wez wrote: > nit: {} can be on one line since they're empty. Done. https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_unittest.cc:130: token_validator_->OnTokenValidated(kSharedSecret)); On 2013/03/22 06:17:01, Wez wrote: > nit: blank line before the comment below Done. https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_unittest.cc:152: ASSERT_NO_FATAL_FAILURE(token_fetcher_->OnTokenFetched(kToken, "")); On 2013/03/22 06:17:01, Wez wrote: > nit: blank line before the comment Done. https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_unittest.cc:187: ASSERT_EQ(Authenticator::PROCESSING_MESSAGE, client_->state()); On 2013/03/22 06:17:01, Wez wrote: > nit: remove the blank line above, for consistency with other cases Done. https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... remoting/protocol/third_party_authenticator_unittest.cc:202: ASSERT_EQ(Authenticator::PROCESSING_MESSAGE, client_->state()); On 2013/03/22 06:17:01, Wez wrote: > nit: as above Done. https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... File remoting/protocol/third_party_client_authenticator.cc (right): https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... remoting/protocol/third_party_client_authenticator.cc:36: std::string token_scope = message->TextNamed(kTokenScopeTag); On 2013/03/22 06:17:01, Wez wrote: > nit: I think it'd be easier to read if the error case came first. Done. https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... remoting/protocol/third_party_client_authenticator.cc:38: token_state_ = PROCESSING_MESSAGE; On 2013/03/22 06:17:01, Wez wrote: > nit: blank line before comment Done. https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... remoting/protocol/third_party_client_authenticator.cc:56: DCHECK(token_state_ == MESSAGE_READY); On 2013/03/22 05:58:43, sergeyu wrote: > DCHECK_EQ Done. https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... remoting/protocol/third_party_client_authenticator.cc:70: if (!token_.empty() && !shared_secret.empty()) { On 2013/03/22 06:17:01, Wez wrote: > nit: Similarly, checking token.empty() || shared_secret.empty() would be > (slightly) easier to read. Done. https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... File remoting/protocol/third_party_client_authenticator.h (right): https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... remoting/protocol/third_party_client_authenticator.h:28: // an authentication key, which is used to establish the connection. On 2013/03/22 06:17:01, Wez wrote: > Isn't this comment already present on the ThirdPartyAuthenticatorBase? Indeed, changed for a client specific comment. https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... remoting/protocol/third_party_client_authenticator.h:58: // Creates a third-party client authenticator, for the host with the given On 2013/03/22 06:17:01, Wez wrote: > nit: remove comma Done. https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... File remoting/protocol/third_party_host_authenticator.cc (right): https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... remoting/protocol/third_party_host_authenticator.cc:37: if (!token.empty()) { On 2013/03/22 05:58:43, sergeyu wrote: > nit: better to handle the error case here first. Done. https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... remoting/protocol/third_party_host_authenticator.cc:59: DCHECK(token_state_ == MESSAGE_READY); On 2013/03/22 05:58:43, sergeyu wrote: > DCHECK_EQ Done. https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... remoting/protocol/third_party_host_authenticator.cc:78: if (!shared_secret.empty()) { On 2013/03/22 06:17:01, Wez wrote: > nit: as above, handle the error case first and early-exit rather than if...else Done. https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... remoting/protocol/third_party_host_authenticator.cc:78: if (!shared_secret.empty()) { On 2013/03/22 06:17:01, Wez wrote: > nit: as above, consider putting error case first Done. https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... remoting/protocol/third_party_host_authenticator.cc:79: // The other side already started the SPAKE authentication. On 2013/03/22 06:17:01, Wez wrote: > nit: as above, handle the error-case first and then early exit rather than > if...else Done. https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... remoting/protocol/third_party_host_authenticator.cc:79: // The other side already started the SPAKE authentication. On 2013/03/22 06:17:01, Wez wrote: > nit: as above, handle the error case first and early exit rather than if...else Done. https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... File remoting/protocol/third_party_host_authenticator.h (right): https://codereview.chromium.org/12326090/diff/61001/remoting/protocol/third_p... remoting/protocol/third_party_host_authenticator.h:43: virtual const GURL& token_url() const = 0; On 2013/03/22 06:17:01, Wez wrote: > nit: blank line between this and comment Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rmsousa@chromium.org/12326090/66004
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rmsousa@chromium.org/12326090/66004
Message was sent while issue was closed.
Change committed as 190024 |
