Checking if current device is revoked in CanLogout #16889
Conversation
go/service/user.go
Outdated
| @@ -638,6 +639,29 @@ func (h *UserHandler) LoadHasRandomPw(ctx context.Context, arg keybase1.LoadHasR | |||
| return ret.RandomPW, err | |||
| } | |||
|
|
|||
| func isActiveDeviceRevoked(mctx libkb.MetaContext) (res bool, err error) { | |||
There was a problem hiding this comment.
@mlsteele is there an easier way?
|
It's a simple but rather important change - comments appreciated! We are always erring on the side of caution here, we don't want to log someone out if we are not sure if they can log back in. There is always a workaround by doing |
go/service/user.go
Outdated
| @@ -638,6 +639,29 @@ func (h *UserHandler) LoadHasRandomPw(ctx context.Context, arg keybase1.LoadHasR | |||
| return ret.RandomPW, err | |||
| } | |||
|
|
|||
| func isActiveDeviceRevoked(mctx libkb.MetaContext) (res bool, err error) { | |||
go/service/user.go
Outdated
| @@ -651,6 +675,15 @@ func (h *UserHandler) CanLogout(ctx context.Context, sessionID int) (res keybase | |||
| }) | |||
|
|
|||
| if err != nil { | |||
| isRevoked, err2 := isActiveDeviceRevoked(libkb.NewMetaContext(ctx, h.G())) | |||
There was a problem hiding this comment.
streamline error handling here and make it more go-y. don't have nested-if error conditions
There was a problem hiding this comment.
I wanted it to be nested here so this is only checked if call to hasRandomPW fails, because hasRandomPW can be cached and works offline. So if CheckCurrentUIDDeviceID is only called when hasRandomPW fails, the whole RPC still have the ability to work offline (assuming hasRandomPW can find a cached value).
Unless there is some other control flow simplification that I'm missing
|
OK this RPC turned out to have more issues than I expected, so we are not trying to rush it for the release. Stranded users can still use |
go/service/user.go
Outdated
| // revoked device. If so, green-light logout. | ||
| if checkUIDErr := libkb.CheckCurrentUIDDeviceID(libkb.NewMetaContext(ctx, h.G())); checkUIDErr != nil { | ||
| switch checkUIDErr.(type) { | ||
| case libkb.DeviceNotFoundError, libkb.UserNotFoundError, libkb.KeyRevokedError, libkb.NoDeviceError, libkb.NoUIDError: |
There was a problem hiding this comment.
nit: dropping a newline between these errors makes it easier to read imo
|
@joshblum @maxtaco made the CanLogout check more straightforward but potentially more expensive - Also added a test for the revoked device + canLogout case. |
If we are unable to check HasRandomPW while logging out, check if we are revoked.
Checking HasRandomPW status code would be sufficient (API session error) but this would be server trust, and also prone to other misc errors. And we are trying really hard not to log user out if they don't have passphrase.