Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2277553002:
Adding a new authenticator which can be used to validate the remote user (Closed)
DescriptionAdding a new authenticator which validates incoming connection details
This change introduces a new authenticator class which handles two scenarios:
- Connection policy validation (i.e. do the details of the connection match the
current policies set on the machine)
- Interactive connection validation (i.e. prompt the user to allow the
connection to be established)
The first scenario is typically synchronous but the second is async, therefore
this validation mechanism must handle both. The new authenticator class wraps
another, functional authenticator to provide a level of validation before
allowing the wrapped authenticator to take over.
BUG=617185
Committed: https://crrev.com/7dc48990ea0ea98b1afa35697676895307f9c615
Cr-Commit-Position: refs/heads/master@{#415715}
Patch Set 1 #Patch Set 2 : Splitting the initial path into two CLs #Patch Set 3 : fixing a ChromeOS build break #
Total comments: 20
Patch Set 4 : Addressing CR feedback #Patch Set 5 : Addressing Feedback #
Total comments: 2
Dependent Patchsets: Messages
Total messages: 41 (30 generated)
The CQ bit was checked by joedow@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== Adding a new authenticator which can be used to validate the remote user This change introduces a new authenticator class which handles two scenarios: - Connection policy validation (i.e. do the details of the connection match the current policies set on the machine) - Interactive connection validation (i.e. prompt the user to allow the connection to be established) The first scenario is typically synchronous but the second is async. Therefore this validation mechanism must handle both. The new authenticator class wraps the negotiating authenticator and provides a level of validation before allowing the negotiating authenticator to take over. BUG=617185 ========== to ========== Adding a new authenticator which validates the incoming connection details This change introduces a new authenticator class which handles two scenarios: - Connection policy validation (i.e. do the details of the connection match the current policies set on the machine) - Interactive connection validation (i.e. prompt the user to allow the connection to be established) The first scenario is typically synchronous but the second is async, therefore this validation mechanism must handle both. The new authenticator class wraps another, functional authenticator to provide a level of validation before allowing the wrapped authenticator to take over. BUG=617185 ==========
Description was changed from ========== Adding a new authenticator which validates the incoming connection details This change introduces a new authenticator class which handles two scenarios: - Connection policy validation (i.e. do the details of the connection match the current policies set on the machine) - Interactive connection validation (i.e. prompt the user to allow the connection to be established) The first scenario is typically synchronous but the second is async, therefore this validation mechanism must handle both. The new authenticator class wraps another, functional authenticator to provide a level of validation before allowing the wrapped authenticator to take over. BUG=617185 ========== to ========== Adding a new authenticator which validates incoming connection details This change introduces a new authenticator class which handles two scenarios: - Connection policy validation (i.e. do the details of the connection match the current policies set on the machine) - Interactive connection validation (i.e. prompt the user to allow the connection to be established) The first scenario is typically synchronous but the second is async, therefore this validation mechanism must handle both. The new authenticator class wraps another, functional authenticator to provide a level of validation before allowing the wrapped authenticator to take over. BUG=617185 ==========
The CQ bit was checked by joedow@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by joedow@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by joedow@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:80001) has been deleted
joedow@chromium.org changed reviewers: + jamiewalch@chromium.org
PTAL! Usage of this new class and refactoring of It2Me and Me2Me factories will come later. Thanks, Joe
https://codereview.chromium.org/2277553002/diff/100001/remoting/protocol/vali... File remoting/protocol/validating_authenticator.cc (right): https://codereview.chromium.org/2277553002/diff/100001/remoting/protocol/vali... remoting/protocol/validating_authenticator.cc:39: Authenticator::State ValidatingAuthenticator::state() const { It seems we should be falling back on the underlying authenticator here. Something like: if (callback_pending) { return PROCESS_MESSAGE; } else if (callback_rejected) { return REJECTED; } else { return current_authenticator_.state(); } That way there's no need to maintain our own copy of the authenticator state, only the validation state, which seems like a cleaner separation of duties. https://codereview.chromium.org/2277553002/diff/100001/remoting/protocol/vali... remoting/protocol/validating_authenticator.cc:49: return rejection_reason_; Similarly here, the underlying authenticator should have some say in the return value. https://codereview.chromium.org/2277553002/diff/100001/remoting/protocol/vali... remoting/protocol/validating_authenticator.cc:53: DCHECK_EQ(state_, ACCEPTED); I don't think you need this check; state() should never return ACCEPTED unless the underlying state is ACCEPTED, in which case it's the responsibility of the underlying authenticator to check this. https://codereview.chromium.org/2277553002/diff/100001/remoting/protocol/vali... remoting/protocol/validating_authenticator.cc:90: result = CreateEmptyAuthenticatorMessage(); I don't think this code should be reachable; if the underlying authenticator does not have a message ready, state() should not return MESSAGE_READY. https://codereview.chromium.org/2277553002/diff/100001/remoting/protocol/vali... remoting/protocol/validating_authenticator.cc:128: // Log an error and use the default value for |rejection_result_|. By defaulting to INVALID_CREDENTIALS here, aren't we making the same mistake we did with third-party auth? It seems like we should have an "unknown" error for cases like this. Maybe too large a change for this CL, though. https://codereview.chromium.org/2277553002/diff/100001/remoting/protocol/vali... File remoting/protocol/validating_authenticator.h (right): https://codereview.chromium.org/2277553002/diff/100001/remoting/protocol/vali... remoting/protocol/validating_authenticator.h:23: // the client. If the connection details are valid (i.e. conform to the current Since the validation is arbitrary, I think you mean e.g. here, not i.e. (the caller could do something other than check policies, presumably). https://codereview.chromium.org/2277553002/diff/100001/remoting/protocol/vali... remoting/protocol/validating_authenticator.h:41: ValidatingAuthenticator(const std::string& remote_jid, This class doesn't need to know the JID--it just passes it to the callback. Since the callback might not need to know the JID, it seems like it would be more flexible for the caller to bind() the JID into the callback if necessary.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by joedow@chromium.org to run a CQ dry run
PTAL! https://codereview.chromium.org/2277553002/diff/100001/remoting/protocol/vali... File remoting/protocol/validating_authenticator.cc (right): https://codereview.chromium.org/2277553002/diff/100001/remoting/protocol/vali... remoting/protocol/validating_authenticator.cc:39: Authenticator::State ValidatingAuthenticator::state() const { On 2016/08/26 23:12:08, Jamie wrote: > It seems we should be falling back on the underlying authenticator here. > Something like: > > if (callback_pending) { > return PROCESS_MESSAGE; > } else if (callback_rejected) { > return REJECTED; > } else { > return current_authenticator_.state(); > } > > That way there's no need to maintain our own copy of the authenticator state, > only the validation state, which seems like a cleaner separation of duties. I started off using a branching structure like this but I didn't like having the additional flags to track state when this is what |state_| already does. The advantage of the approach I have is that there is a single state in this class which is updated by the underlying authenticator (this is similar to the NegotiatingAuthenticator which also wraps an authenticator). The disadvantage of my approach is that a wrapper is needed around the resume_callback passed to ProcessMessage(). I'd prefer to keep this mechanism as I think it is cleaner. https://codereview.chromium.org/2277553002/diff/100001/remoting/protocol/vali... remoting/protocol/validating_authenticator.cc:49: return rejection_reason_; On 2016/08/26 23:12:07, Jamie wrote: > Similarly here, the underlying authenticator should have some say in the return > value. Same as above, I think it is cleaner to just use |state_| and |rejection_reason_| instead of multiple flags. https://codereview.chromium.org/2277553002/diff/100001/remoting/protocol/vali... remoting/protocol/validating_authenticator.cc:53: DCHECK_EQ(state_, ACCEPTED); On 2016/08/26 23:12:08, Jamie wrote: > I don't think you need this check; state() should never return ACCEPTED unless > the underlying state is ACCEPTED, in which case it's the responsibility of the > underlying authenticator to check this. I'm fine removing this as you are correct that the underling authenticator should check/DCHECK this as needed. Other authenticator classes have strict checking so I wanted to include that here as well but I'm ok removing it for the methods which solely rely on the wrapped authenticator. https://codereview.chromium.org/2277553002/diff/100001/remoting/protocol/vali... remoting/protocol/validating_authenticator.cc:90: result = CreateEmptyAuthenticatorMessage(); On 2016/08/26 23:12:07, Jamie wrote: > I don't think this code should be reachable; if the underlying authenticator > does not have a message ready, state() should not return MESSAGE_READY. I think that's a reasonable assumption, I'll leave it to the underlying authenticator to decide what to give the caller (and possibly DCHECK as needed). https://codereview.chromium.org/2277553002/diff/100001/remoting/protocol/vali... remoting/protocol/validating_authenticator.cc:128: // Log an error and use the default value for |rejection_result_|. On 2016/08/26 23:12:08, Jamie wrote: > By defaulting to INVALID_CREDENTIALS here, aren't we making the same mistake we > did with third-party auth? It seems like we should have an "unknown" error for > cases like this. Maybe too large a change for this CL, though. What should happen is that the person adding the new value would see the NOTREACHED macro trigger on their debug build and add it to the enum. My concern with adding an Unknown error is that it could be abused just as easily and become a grab-bag error. Perhaps the right thing to do is to force a failure on both retail and debug builds, that way someone introducing a new validation error can't miss the NOTREACHED macro on CHK builds. Let me know if this is something you'd prefer. https://codereview.chromium.org/2277553002/diff/100001/remoting/protocol/vali... File remoting/protocol/validating_authenticator.h (right): https://codereview.chromium.org/2277553002/diff/100001/remoting/protocol/vali... remoting/protocol/validating_authenticator.h:23: // the client. If the connection details are valid (i.e. conform to the current On 2016/08/26 23:12:08, Jamie wrote: > Since the validation is arbitrary, I think you mean e.g. here, not i.e. (the > caller could do something other than check policies, presumably). True, I'll turn i.e. into e.g. and then I think this the comment is accurate and generic (albeit with an example). https://codereview.chromium.org/2277553002/diff/100001/remoting/protocol/vali... remoting/protocol/validating_authenticator.h:41: ValidatingAuthenticator(const std::string& remote_jid, On 2016/08/26 23:12:08, Jamie wrote: > This class doesn't need to know the JID--it just passes it to the callback. > Since the callback might not need to know the JID, it seems like it would be > more flexible for the caller to bind() the JID into the callback if necessary. The component providing the ValidationCallback does not know the JID at the time the method is bound and the ProcessMessage interface method does not take that param (i'd hate to increase the number of params just for this scenario). At this point the Authenticator would be used for It2Me and Me2Me and both require the remote JID to validate the incoming connection.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ping!
LGTM unless either of my outstanding comments leads to significant changes. https://codereview.chromium.org/2277553002/diff/100001/remoting/protocol/vali... File remoting/protocol/validating_authenticator.cc (right): https://codereview.chromium.org/2277553002/diff/100001/remoting/protocol/vali... remoting/protocol/validating_authenticator.cc:39: Authenticator::State ValidatingAuthenticator::state() const { On 2016/08/29 23:43:22, joedow wrote: > On 2016/08/26 23:12:08, Jamie wrote: > > It seems we should be falling back on the underlying authenticator here. > > Something like: > > > > if (callback_pending) { > > return PROCESS_MESSAGE; > > } else if (callback_rejected) { > > return REJECTED; > > } else { > > return current_authenticator_.state(); > > } > > > > That way there's no need to maintain our own copy of the authenticator state, > > only the validation state, which seems like a cleaner separation of duties. > > I started off using a branching structure like this but I didn't like having the > additional flags to track state when this is what |state_| already does. > > The advantage of the approach I have is that there is a single state in this > class which is updated by the underlying authenticator (this is similar to the > NegotiatingAuthenticator which also wraps an authenticator). > > The disadvantage of my approach is that a wrapper is needed around the > resume_callback passed to ProcessMessage(). > > I'd prefer to keep this mechanism as I think it is cleaner. I'll make a couple of points in favour of my approach; if you're still not convinced then I'm fine with leaving it this way :) Firstly, since this class is responsible for invoking a specified callback before proceeding with an existing validator, I think it's cleaner for it to maintain state only about that callback. Secondly, storing the state in this class means it's essentially being stored twice--once in this authenticator and once in the underlying one--which violates the general design principle of only storing things once. https://codereview.chromium.org/2277553002/diff/100001/remoting/protocol/vali... remoting/protocol/validating_authenticator.cc:128: // Log an error and use the default value for |rejection_result_|. On 2016/08/29 23:43:22, joedow wrote: > On 2016/08/26 23:12:08, Jamie wrote: > > By defaulting to INVALID_CREDENTIALS here, aren't we making the same mistake > we > > did with third-party auth? It seems like we should have an "unknown" error for > > cases like this. Maybe too large a change for this CL, though. > > What should happen is that the person adding the new value would see the > NOTREACHED macro trigger on their debug build and add it to the enum. My > concern with adding an Unknown error is that it could be abused just as easily > and become a grab-bag error. > > Perhaps the right thing to do is to force a failure on both retail and debug > builds, that way someone introducing a new validation error can't miss the > NOTREACHED macro on CHK builds. Let me know if this is something you'd prefer. Better still, if you fold the SUCCESS case into the switch and get rid of default, can't the compiler detect missing cases automatically? Also, s/rejection_result_/rejection_reason_/ https://codereview.chromium.org/2277553002/diff/100001/remoting/protocol/vali... File remoting/protocol/validating_authenticator.h (right): https://codereview.chromium.org/2277553002/diff/100001/remoting/protocol/vali... remoting/protocol/validating_authenticator.h:41: ValidatingAuthenticator(const std::string& remote_jid, On 2016/08/29 23:43:22, joedow wrote: > On 2016/08/26 23:12:08, Jamie wrote: > > This class doesn't need to know the JID--it just passes it to the callback. > > Since the callback might not need to know the JID, it seems like it would be > > more flexible for the caller to bind() the JID into the callback if necessary. > > The component providing the ValidationCallback does not know the JID at the time > the method is bound and the ProcessMessage interface method does not take that > param (i'd hate to increase the number of params just for this scenario). > > At this point the Authenticator would be used for It2Me and Me2Me and both > require the remote JID to validate the incoming connection. You could rebind the callback with the JID at the point of construction of this class. I think that's a little cleaner than adding an otherwise unnecessary dependency on the remote JID, but maybe I've just been writing too much JS lately. I'll leave it to your discretion.
Thanks! https://codereview.chromium.org/2277553002/diff/100001/remoting/protocol/vali... File remoting/protocol/validating_authenticator.cc (right): https://codereview.chromium.org/2277553002/diff/100001/remoting/protocol/vali... remoting/protocol/validating_authenticator.cc:39: Authenticator::State ValidatingAuthenticator::state() const { On 2016/08/31 00:28:34, Jamie wrote: > On 2016/08/29 23:43:22, joedow wrote: > > On 2016/08/26 23:12:08, Jamie wrote: > > > It seems we should be falling back on the underlying authenticator here. > > > Something like: > > > > > > if (callback_pending) { > > > return PROCESS_MESSAGE; > > > } else if (callback_rejected) { > > > return REJECTED; > > > } else { > > > return current_authenticator_.state(); > > > } > > > > > > That way there's no need to maintain our own copy of the authenticator > state, > > > only the validation state, which seems like a cleaner separation of duties. > > > > I started off using a branching structure like this but I didn't like having > the > > additional flags to track state when this is what |state_| already does. > > > > The advantage of the approach I have is that there is a single state in this > > class which is updated by the underlying authenticator (this is similar to the > > NegotiatingAuthenticator which also wraps an authenticator). > > > > The disadvantage of my approach is that a wrapper is needed around the > > resume_callback passed to ProcessMessage(). > > > > I'd prefer to keep this mechanism as I think it is cleaner. > > I'll make a couple of points in favour of my approach; if you're still not > convinced then I'm fine with leaving it this way :) > > Firstly, since this class is responsible for invoking a specified callback > before proceeding with an existing validator, I think it's cleaner for it to > maintain state only about that callback. Secondly, storing the state in this > class means it's essentially being stored twice--once in this authenticator and > once in the underlying one--which violates the general design principle of only > storing things once. Discussed offline, I have tried this approach first but it wasn't as clean. https://codereview.chromium.org/2277553002/diff/100001/remoting/protocol/vali... remoting/protocol/validating_authenticator.cc:128: // Log an error and use the default value for |rejection_result_|. On 2016/08/31 00:28:34, Jamie wrote: > On 2016/08/29 23:43:22, joedow wrote: > > On 2016/08/26 23:12:08, Jamie wrote: > > > By defaulting to INVALID_CREDENTIALS here, aren't we making the same mistake > > we > > > did with third-party auth? It seems like we should have an "unknown" error > for > > > cases like this. Maybe too large a change for this CL, though. > > > > What should happen is that the person adding the new value would see the > > NOTREACHED macro trigger on their debug build and add it to the enum. My > > concern with adding an Unknown error is that it could be abused just as easily > > and become a grab-bag error. > > > > Perhaps the right thing to do is to force a failure on both retail and debug > > builds, that way someone introducing a new validation error can't miss the > > NOTREACHED macro on CHK builds. Let me know if this is something you'd > prefer. > > Better still, if you fold the SUCCESS case into the switch and get rid of > default, can't the compiler detect missing cases automatically? > > Also, s/rejection_result_/rejection_reason_/ Fixed the comment. It looks like the compiler doesn't pick up missing enum values by default so I have made this a CHECK error (to prevent someone from missing this will implementing their change). https://codereview.chromium.org/2277553002/diff/100001/remoting/protocol/vali... File remoting/protocol/validating_authenticator.h (right): https://codereview.chromium.org/2277553002/diff/100001/remoting/protocol/vali... remoting/protocol/validating_authenticator.h:41: ValidatingAuthenticator(const std::string& remote_jid, On 2016/08/31 00:28:34, Jamie wrote: > On 2016/08/29 23:43:22, joedow wrote: > > On 2016/08/26 23:12:08, Jamie wrote: > > > This class doesn't need to know the JID--it just passes it to the callback. > > > Since the callback might not need to know the JID, it seems like it would be > > > more flexible for the caller to bind() the JID into the callback if > necessary. > > > > The component providing the ValidationCallback does not know the JID at the > time > > the method is bound and the ProcessMessage interface method does not take that > > param (i'd hate to increase the number of params just for this scenario). > > > > At this point the Authenticator would be used for It2Me and Me2Me and both > > require the remote JID to validate the incoming connection. > > You could rebind the callback with the JID at the point of construction of this > class. I think that's a little cleaner than adding an otherwise unnecessary > dependency on the remote JID, but maybe I've just been writing too much JS > lately. I'll leave it to your discretion. Discussed offline. I could do this but it wouldn't be very clean.
The CQ bit was checked by joedow@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jamiewalch@chromium.org Link to the patchset: https://codereview.chromium.org/2277553002/#ps130001 (title: "Addressing Feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:130001)
Message was sent while issue was closed.
Description was changed from ========== Adding a new authenticator which validates incoming connection details This change introduces a new authenticator class which handles two scenarios: - Connection policy validation (i.e. do the details of the connection match the current policies set on the machine) - Interactive connection validation (i.e. prompt the user to allow the connection to be established) The first scenario is typically synchronous but the second is async, therefore this validation mechanism must handle both. The new authenticator class wraps another, functional authenticator to provide a level of validation before allowing the wrapped authenticator to take over. BUG=617185 ========== to ========== Adding a new authenticator which validates incoming connection details This change introduces a new authenticator class which handles two scenarios: - Connection policy validation (i.e. do the details of the connection match the current policies set on the machine) - Interactive connection validation (i.e. prompt the user to allow the connection to be established) The first scenario is typically synchronous but the second is async, therefore this validation mechanism must handle both. The new authenticator class wraps another, functional authenticator to provide a level of validation before allowing the wrapped authenticator to take over. BUG=617185 Committed: https://crrev.com/7dc48990ea0ea98b1afa35697676895307f9c615 Cr-Commit-Position: refs/heads/master@{#415715} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/7dc48990ea0ea98b1afa35697676895307f9c615 Cr-Commit-Position: refs/heads/master@{#415715}
Message was sent while issue was closed.
sergeyu@chromium.org changed reviewers: + sergeyu@chromium.org
Message was sent while issue was closed.
couple drive-by comments https://codereview.chromium.org/2277553002/diff/130001/remoting/protocol/vali... File remoting/protocol/validating_authenticator.cc (right): https://codereview.chromium.org/2277553002/diff/130001/remoting/protocol/vali... remoting/protocol/validating_authenticator.cc:118: default: It's best to avoid default case for enum switch statments. Otherwise compiler will not help when someone adds another value in the enum and doesn't update this switch() statement. replace this with Result::SUCCESS? https://codereview.chromium.org/2277553002/diff/130001/remoting/protocol/vali... remoting/protocol/validating_authenticator.cc:120: CHECK(false) << "Unknown validation result value: " I think NOTREACHED() would be appropriate here - the connection is REJECTED anyway. See https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
Message was sent while issue was closed.
Good comments, I will address in a follow-up CL. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
