Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Checking if current device is revoked in CanLogout #16889

Merged
merged 6 commits into from
Apr 10, 2019
Merged

Checking if current device is revoked in CanLogout #16889

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7928efc
Checking if current device is revoked in CanLogout
zapu Apr 8, 2019
10d72b7
Fix test crash due to incomplete mock
zapu Apr 8, 2019
d2c262f
Use CheckCurrentUIDDeviceID
zapu Apr 8, 2019
293579c
Style fix
zapu Apr 9, 2019
d3b69e2
Move CheckCurrentUIDDeviceID check
zapu Apr 10, 2019
0396ac4
Improve test
zapu Apr 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion go/libkb/active_device.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ func (a *ActiveDevice) Dump(m MetaContext, prefix string) {
m.Debug("%sUsername (via env): %s", prefix, a.Username(m))
m.Debug("%sDeviceID: %s", prefix, a.deviceID)
m.Debug("%sDeviceName: %s", prefix, a.deviceName)
m.Debug("%sDeviceCtime: %s", prefix, a.deviceCtime)
m.Debug("%sDeviceCtime: %s", prefix, keybase1.FormatTime(a.deviceCtime))
if a.signingKey != nil {
m.Debug("%sSigKey: %s", prefix, a.signingKey.GetKID())
}
Expand Down
35 changes: 34 additions & 1 deletion go/service/user.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package service

import (
"errors"
"fmt"
"sort"
"time"
Expand Down Expand Up @@ -572,7 +573,7 @@ func (h *UserHandler) LoadHasRandomPw(ctx context.Context, arg keybase1.LoadHasR
m = m.WithLogTag("HASRPW")
defer m.TraceTimed(fmt.Sprintf("UserHandler#LoadHasRandomPw(forceRepoll=%t)", arg.ForceRepoll), func() error { return err })()

meUID := m.G().ActiveDevice.UID()
meUID := m.ActiveDevice().UID()
cacheKey := libkb.DbKey{
Typ: libkb.DBHasRandomPW,
Key: meUID.String(),
Expand Down Expand Up @@ -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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mlsteele is there an easier way?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checkDeviceValidForUID

if !mctx.ActiveDevice().Valid() {
return false, errors.New("ActiveDevice is not valid")
}
deviceID := mctx.ActiveDevice().DeviceID()
me, err := libkb.LoadMe(libkb.NewLoadUserArgWithMetaContext(mctx))
if err != nil {
return false, err
}
devices := me.GetComputedKeyFamily().GetAllActiveDevices()
if len(devices) == 0 {
return false, errors.New("GetAllActiveDevices returned empty list")
}
for _, device := range devices {
if device.ID.Eq(deviceID) {
// Device found in ActiveDevices list, we are not revoked.
return false, nil
}
}
// Current device not found, we are likely revoked.
return true, nil
}

func (h *UserHandler) CanLogout(ctx context.Context, sessionID int) (res keybase1.CanLogoutRes, err error) {
if !h.G().ActiveDevice.Valid() {
h.G().Log.CDebugf(ctx, "CanLogout: looks like user is not logged in")
Expand All @@ -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()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

streamline error handling here and make it more go-y. don't have nested-if error conditions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if err2 == nil && isRevoked {
// We are revoked, green-light logging out.
h.G().Log.CDebugf(ctx, "CanLogout: Current device is revoked, allowing logout")
return keybase1.CanLogoutRes{CanLogout: true}, nil
} else if err2 != nil {
h.G().Log.CDebugf(ctx, "CanLogout: cannot check if current device is revoked: %s", err2.Error())
}

return keybase1.CanLogoutRes{
CanLogout: false,
Reason: fmt.Sprintf("We couldn't ensure that your account has a passphrase: %s", err.Error()),
Expand Down