Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2507253002:
[net/auth] Treat ERR_INVALID_HANDLE as a identity invalidating error. (Closed)
Description[net/auth] Treat ERR_INVALID_HANDLE as a identity invalidating error.
BUG=648366
Committed: https://crrev.com/1d50f4974f89eebc449b0b8747b299267504e7ed
Cr-Commit-Position: refs/heads/master@{#432922}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Reparent this CL so that it detaches from its dependencies #Messages
Total messages: 26 (12 generated)
asanka@chromium.org changed reviewers: + mmenke@chromium.org
https://codereview.chromium.org/2507253002/diff/1/net/http/http_auth_controll... File net/http/http_auth_controller.cc (right): https://codereview.chromium.org/2507253002/diff/1/net/http/http_auth_controll... net/http/http_auth_controller.cc:482: case ERR_INVALID_HANDLE: How do we get an invalid handle in the first place? I guess this is similar enough to the ERR_INVALID_AUTH_CREDENTIALS case that testing doesn't get us much, since we mock out the thing that's actually returning the error.
On 2016/11/16 22:44:23, mmenke wrote: > https://codereview.chromium.org/2507253002/diff/1/net/http/http_auth_controll... > File net/http/http_auth_controller.cc (right): > > https://codereview.chromium.org/2507253002/diff/1/net/http/http_auth_controll... > net/http/http_auth_controller.cc:482: case ERR_INVALID_HANDLE: > How do we get an invalid handle in the first place? > > I guess this is similar enough to the ERR_INVALID_AUTH_CREDENTIALS case that > testing doesn't get us much, since we mock out the thing that's actually > returning the error. Oh, and one other question...When we try with different credentials here, we're talking system credentials vs user-entered credentials, right? Not some other set of system credentials?
https://codereview.chromium.org/2507253002/diff/1/net/http/http_auth_controll... File net/http/http_auth_controller.cc (right): https://codereview.chromium.org/2507253002/diff/1/net/http/http_auth_controll... net/http/http_auth_controller.cc:482: case ERR_INVALID_HANDLE: On 2016/11/16 at 22:44:23, mmenke wrote: > How do we get an invalid handle in the first place? It's possible for us to get a credentials handle, but then the platform deciding that the handle is not valid for auth target when we get around to generating a token. > I guess this is similar enough to the ERR_INVALID_AUTH_CREDENTIALS case that testing doesn't get us much, since we mock out the thing that's actually returning the error.
On 2016/11/16 at 22:46:53, mmenke wrote: > On 2016/11/16 22:44:23, mmenke wrote: > > https://codereview.chromium.org/2507253002/diff/1/net/http/http_auth_controll... > > File net/http/http_auth_controller.cc (right): > > > > https://codereview.chromium.org/2507253002/diff/1/net/http/http_auth_controll... > > net/http/http_auth_controller.cc:482: case ERR_INVALID_HANDLE: > > How do we get an invalid handle in the first place? > > > > I guess this is similar enough to the ERR_INVALID_AUTH_CREDENTIALS case that > > testing doesn't get us much, since we mock out the thing that's actually > > returning the error. > > Oh, and one other question...When we try with different credentials here, we're talking system credentials vs user-entered credentials, right? Not some other set of system credentials? Yeah. Ambient credentials, if they were used, would be marked as used. So we'd proceed to whichever is next in line. If there aren't any other options, then we'd move on to the next viable scheme or give up.
The CQ bit was checked by asanka@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...
Great, then I feel I understand this well enough not to delve into the True Nature of invalid handles. LGTM! On 2016/11/16 23:02:57, asanka wrote: > On 2016/11/16 at 22:46:53, mmenke wrote: > > On 2016/11/16 22:44:23, mmenke wrote: > > > > https://codereview.chromium.org/2507253002/diff/1/net/http/http_auth_controll... > > > File net/http/http_auth_controller.cc (right): > > > > > > > https://codereview.chromium.org/2507253002/diff/1/net/http/http_auth_controll... > > > net/http/http_auth_controller.cc:482: case ERR_INVALID_HANDLE: > > > How do we get an invalid handle in the first place? > > > > > > I guess this is similar enough to the ERR_INVALID_AUTH_CREDENTIALS case that > > > testing doesn't get us much, since we mock out the thing that's actually > > > returning the error. > > > > Oh, and one other question...When we try with different credentials here, > we're talking system credentials vs user-entered credentials, right? Not some > other set of system credentials? > > Yeah. Ambient credentials, if they were used, would be marked as used. So we'd > proceed to whichever is next in line. If there aren't any other options, then > we'd move on to the next viable scheme or give up. Great, LGTM!
On 2016/11/16 23:45:05, mmenke wrote: > Great, then I feel I understand this well enough not to delve into the True > Nature of invalid handles. > > LGTM! > > On 2016/11/16 23:02:57, asanka wrote: > > On 2016/11/16 at 22:46:53, mmenke wrote: > > > On 2016/11/16 22:44:23, mmenke wrote: > > > > > > > https://codereview.chromium.org/2507253002/diff/1/net/http/http_auth_controll... > > > > File net/http/http_auth_controller.cc (right): > > > > > > > > > > > https://codereview.chromium.org/2507253002/diff/1/net/http/http_auth_controll... > > > > net/http/http_auth_controller.cc:482: case ERR_INVALID_HANDLE: > > > > How do we get an invalid handle in the first place? > > > > > > > > I guess this is similar enough to the ERR_INVALID_AUTH_CREDENTIALS case > that > > > > testing doesn't get us much, since we mock out the thing that's actually > > > > returning the error. > > > > > > Oh, and one other question...When we try with different credentials here, > > we're talking system credentials vs user-entered credentials, right? Not some > > other set of system credentials? > > > > Yeah. Ambient credentials, if they were used, would be marked as used. So we'd > > proceed to whichever is next in line. If there aren't any other options, then > > we'd move on to the next viable scheme or give up. > > Great, LGTM! Oops, that second great was an accident. I think this is only worth one great. Your test refactoring CL, however...
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 asanka@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2505203002 Patch 1). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
Thanks!
The CQ bit was checked by asanka@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2507253002/#ps20001 (title: "Reparent this CL so that it detaches from its dependencies")
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
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_...)
The CQ bit was checked by asanka@chromium.org
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 #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [net/auth] Treat ERR_INVALID_HANDLE as a identity invalidating error. BUG=648366 ========== to ========== [net/auth] Treat ERR_INVALID_HANDLE as a identity invalidating error. BUG=648366 Committed: https://crrev.com/1d50f4974f89eebc449b0b8747b299267504e7ed Cr-Commit-Position: refs/heads/master@{#432922} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/1d50f4974f89eebc449b0b8747b299267504e7ed Cr-Commit-Position: refs/heads/master@{#432922} |
