fix: address review comments

This commit is contained in:
James Houlahan 2020-04-21 07:17:50 +00:00
parent febdf98349
commit 1457005f86
31 changed files with 88 additions and 104 deletions

View File

@ -56,7 +56,6 @@ import (
"github.com/ProtonMail/proton-bridge/pkg/args"
"github.com/ProtonMail/proton-bridge/pkg/config"
"github.com/ProtonMail/proton-bridge/pkg/listener"
"github.com/ProtonMail/proton-bridge/pkg/logs"
"github.com/ProtonMail/proton-bridge/pkg/pmapi"
"github.com/ProtonMail/proton-bridge/pkg/updates"
"github.com/allan-simon/go-singleinstance"
@ -87,7 +86,7 @@ var (
longVersion = Version + " (" + Revision + ")" //nolint[gochecknoglobals]
buildVersion = longVersion + " " + BuildTime //nolint[gochecknoglobals]
log = logs.GetLogEntry("main") //nolint[gochecknoglobals]
log = logrus.WithField("pkg", "main") //nolint[gochecknoglobals]
// How many crashes in a row.
numberOfCrashes = 0 //nolint[gochecknoglobals]

View File

@ -31,12 +31,12 @@ import (
"github.com/ProtonMail/proton-bridge/internal/preferences"
"github.com/ProtonMail/proton-bridge/pkg/config"
"github.com/ProtonMail/proton-bridge/pkg/listener"
"github.com/ProtonMail/proton-bridge/pkg/logs"
"github.com/ProtonMail/proton-bridge/pkg/ports"
"github.com/sirupsen/logrus"
)
var (
log = logs.GetLogEntry("api") //nolint[gochecknoglobals]
log = logrus.WithField("pkg", "api") //nolint[gochecknoglobals]
)
type apiServer struct {

View File

@ -29,7 +29,6 @@ import (
"github.com/ProtonMail/proton-bridge/internal/preferences"
"github.com/ProtonMail/proton-bridge/internal/store"
"github.com/ProtonMail/proton-bridge/pkg/listener"
"github.com/ProtonMail/proton-bridge/pkg/logs"
"github.com/ProtonMail/proton-bridge/pkg/pmapi"
"github.com/hashicorp/go-multierror"
"github.com/pkg/errors"
@ -37,8 +36,8 @@ import (
)
var (
log = logs.GetLogEntry("bridge") //nolint[gochecknoglobals]
isApplicationOutdated = false //nolint[gochecknoglobals]
log = logrus.WithField("pkg", "bridge") //nolint[gochecknoglobals]
isApplicationOutdated = false //nolint[gochecknoglobals]
)
// Bridge is a struct handling users.
@ -65,7 +64,8 @@ type Bridge struct {
lock sync.RWMutex
cancel chan struct{}
// stopAll can be closed to stop all goroutines from looping (watchBridgeOutdated, watchAPIAuths, heartbeat etc).
stopAll chan struct{}
userAgentClientName string
userAgentClientVersion string
@ -94,7 +94,7 @@ func New(
storeCache: store.NewCache(config.GetIMAPCachePath()),
idleUpdates: make(chan interface{}),
lock: sync.RWMutex{},
cancel: make(chan struct{}),
stopAll: make(chan struct{}),
}
// Allow DoH before starting bridge if the user has previously set this setting.
@ -146,7 +146,7 @@ func (b *Bridge) heartbeat() {
b.pref.Set(preferences.NextHeartbeatKey, strconv.FormatInt(nextTime.Unix(), 10))
}
case <-b.cancel:
case <-b.stopAll:
return
}
}
@ -191,7 +191,7 @@ func (b *Bridge) watchBridgeOutdated() {
isApplicationOutdated = true
b.closeAllConnections()
case <-b.cancel:
case <-b.stopAll:
return
}
}
@ -218,7 +218,7 @@ func (b *Bridge) watchAPIAuths() {
Error("User logout failed while watching API auths")
}
case <-b.cancel:
case <-b.stopAll:
return
}
}
@ -568,7 +568,7 @@ func (b *Bridge) CheckConnection() error {
// StopWatchers stops all bridge goroutines.
func (b *Bridge) StopWatchers() {
close(b.cancel)
close(b.stopAll)
}
func (b *Bridge) updateCurrentUserAgent() {

View File

@ -0,0 +1,6 @@
package bridge
// IsAuthorized returns whether the user has received an Auth from the API yet.
func (u *User) IsAuthorized() bool {
return u.isAuthorized
}

View File

@ -27,14 +27,13 @@ import (
"fmt"
"strings"
"github.com/ProtonMail/proton-bridge/pkg/logs"
"github.com/sirupsen/logrus"
)
const sep = "\x00"
var (
log = logs.GetLogEntry("bridge") //nolint[gochecknoglobals]
log = logrus.WithField("pkg", "bridge") //nolint[gochecknoglobals]
ErrWrongFormat = errors.New("backend/creds: malformed password")
)

View File

@ -164,7 +164,7 @@ func (u *User) SetIMAPIdleUpdateChannel() {
// it tries to connect it.
func (u *User) authorizeIfNecessary(emitEvent bool) (err error) {
// If user is connected and has an auth channel, then perfect, nothing to do here.
if u.creds.IsConnected() && u.HasAPIAuth() {
if u.creds.IsConnected() && u.isAuthorized {
// The keyring unlock is triggered here to resolve state where apiClient
// is authenticated (we have auth token) but it was not possible to download
// and unlock the keys (internet not reachable).
@ -585,7 +585,3 @@ func (u *User) CloseConnection(address string) {
func (u *User) GetStore() *store.Store {
return u.store
}
func (u *User) HasAPIAuth() bool {
return u.isAuthorized
}

View File

@ -24,13 +24,13 @@ import (
"github.com/ProtonMail/proton-bridge/internal/preferences"
"github.com/ProtonMail/proton-bridge/pkg/config"
"github.com/ProtonMail/proton-bridge/pkg/listener"
"github.com/ProtonMail/proton-bridge/pkg/logs"
"github.com/abiosoft/ishell"
"github.com/sirupsen/logrus"
)
var (
log = logs.GetLogEntry("frontend/cli") //nolint[gochecknoglobals]
log = logrus.WithField("pkg", "frontend/cli") //nolint[gochecknoglobals]
)
type frontendCLI struct {

View File

@ -26,11 +26,11 @@ import (
"github.com/ProtonMail/proton-bridge/internal/frontend/types"
"github.com/ProtonMail/proton-bridge/pkg/config"
"github.com/ProtonMail/proton-bridge/pkg/listener"
"github.com/ProtonMail/proton-bridge/pkg/logs"
"github.com/sirupsen/logrus"
)
var (
log = logs.GetLogEntry("frontend") // nolint[unused]
log = logrus.WithField("pkg", "frontend") // nolint[unused]
)
// Frontend is an interface to be implemented by each frontend type (cli, gui, html).

View File

@ -43,9 +43,9 @@ import (
"github.com/ProtonMail/proton-bridge/internal/frontend/types"
"github.com/ProtonMail/proton-bridge/internal/preferences"
"github.com/ProtonMail/proton-bridge/pkg/config"
"github.com/ProtonMail/proton-bridge/pkg/logs"
"github.com/ProtonMail/proton-bridge/pkg/ports"
"github.com/ProtonMail/proton-bridge/pkg/useragent"
"github.com/sirupsen/logrus"
//"github.com/ProtonMail/proton-bridge/pkg/keychain"
"github.com/ProtonMail/proton-bridge/pkg/listener"
@ -59,7 +59,7 @@ import (
"github.com/therecipe/qt/widgets"
)
var log = logs.GetLogEntry("frontend-qt")
var log = logrus.WithField("pkg", "frontend-qt")
var accountMutex = &sync.Mutex{}
// API between Bridge and Qt.

View File

@ -26,10 +26,10 @@ import (
"github.com/ProtonMail/proton-bridge/internal/frontend/types"
"github.com/ProtonMail/proton-bridge/pkg/config"
"github.com/ProtonMail/proton-bridge/pkg/listener"
"github.com/ProtonMail/proton-bridge/pkg/logs"
"github.com/sirupsen/logrus"
)
var log = logs.GetLogEntry("frontend-nogui") //nolint[gochecknoglobals]
var log = logrus.WithField("pkg", "frontend-nogui") //nolint[gochecknoglobals]
type FrontendHeadless struct{}

View File

@ -17,7 +17,7 @@
package imap
import "github.com/ProtonMail/proton-bridge/pkg/logs"
import "github.com/sirupsen/logrus"
const (
fetchMessagesWorkers = 5 // In how many workers to fetch message (group list on IMAP).
@ -31,5 +31,5 @@ const (
)
var (
log = logs.GetLogEntry("imap") //nolint[gochecknoglobals]
log = logrus.WithField("pkg", "imap") //nolint[gochecknoglobals]
)

View File

@ -18,8 +18,8 @@
// Package smtp provides SMTP server of the Bridge.
package smtp
import "github.com/ProtonMail/proton-bridge/pkg/logs"
import "github.com/sirupsen/logrus"
var (
log = logs.GetLogEntry("smtp") //nolint[gochecknoglobals]
log = logrus.WithField("pkg", "smtp") //nolint[gochecknoglobals]
)

View File

@ -247,7 +247,7 @@ func (loop *eventLoop) processNextEvent() (more bool, err error) { // nolint[fun
}
if event == nil {
return
return false, errors.New("received empty event")
}
l = l.WithField("newEventID", event.EventID)

View File

@ -19,7 +19,6 @@ package store
import (
"encoding/json"
"errors"
"fmt"
"github.com/ProtonMail/proton-bridge/pkg/pmapi"
@ -56,7 +55,7 @@ func (store *Store) TestGetStoreFilePath() string {
func (store *Store) TestDumpDB(tb assert.TestingT) {
if store == nil || store.db == nil {
fmt.Printf(">>>>>>>> NIL STORE / DB <<<<<\n\n")
assert.NoError(tb, errors.New("store or database is nil"))
assert.Fail(tb, "store or database is nil")
return
}

View File

@ -23,12 +23,12 @@ import (
"path/filepath"
"github.com/ProtonMail/go-appdir"
"github.com/ProtonMail/proton-bridge/pkg/logs"
"github.com/hashicorp/go-multierror"
"github.com/sirupsen/logrus"
)
var (
log = logs.GetLogEntry("config") //nolint[gochecknoglobals]
log = logrus.WithField("pkg", "config") //nolint[gochecknoglobals]
)
type appDirProvider interface {

View File

@ -23,8 +23,8 @@ import (
"strings"
"sync"
"github.com/ProtonMail/proton-bridge/pkg/logs"
"github.com/docker/docker-credential-helpers/credentials"
"github.com/sirupsen/logrus"
)
const (
@ -32,7 +32,7 @@ const (
)
var (
log = logs.GetLogEntry("bridgeUtils/keychain") //nolint[gochecknoglobals]
log = logrus.WithField("pkg", "bridgeUtils/keychain") //nolint[gochecknoglobals]
ErrWrongKeychainURL = errors.New("wrong keychain base URL")
ErrMacKeychainRebuild = errors.New("keychain error -25293")

View File

@ -21,10 +21,10 @@ import (
"sync"
"time"
"github.com/ProtonMail/proton-bridge/pkg/logs"
"github.com/sirupsen/logrus"
)
var log = logs.GetLogEntry("bridgeUtils/listener") //nolint[gochecknoglobals]
var log = logrus.WithField("pkg", "bridgeUtils/listener") //nolint[gochecknoglobals]
// Listener has a list of channels watching for updates.
type Listener interface {

View File

@ -1,10 +0,0 @@
package logs
import "github.com/sirupsen/logrus"
// GetLogEntry returns logrus.Entry with PID and `packageName`.
func GetLogEntry(packageName string) *logrus.Entry {
return logrus.WithFields(logrus.Fields{
"pkg": packageName,
})
}

View File

@ -218,7 +218,7 @@ func (c *client) sendAuth(auth *Auth) {
}
go func(auth ClientAuth) {
c.cm.GetClientAuthChannel() <- auth
c.cm.clientAuths <- auth
}(ClientAuth{
UserID: c.userID,
Auth: auth,
@ -425,7 +425,7 @@ func (c *client) Unlock(password string) (kr *pmcrypto.KeyRing, err error) {
func (c *client) AuthRefresh(uidAndRefreshToken string) (auth *Auth, err error) {
// If we don't yet have a saved access token, save this one in case the refresh fails!
// That way we can try again later (see handleUnauthorizedStatus).
c.cm.SetTokenIfUnset(c.userID, uidAndRefreshToken)
c.cm.setTokenIfUnset(c.userID, uidAndRefreshToken)
split := strings.Split(uidAndRefreshToken, ":")
if len(split) != 2 {

View File

@ -32,8 +32,8 @@ type ClientManager struct {
expiredTokens chan string
expirationsLocker sync.Locker
bridgeAuths chan ClientAuth
clientAuths chan ClientAuth
clientAuths chan ClientAuth // auths received by clients from the API are received here and handled.
forwardedAuths chan ClientAuth // once auths are handled, they are forwarded on this channel.
host, scheme string
hostLocker sync.RWMutex
@ -82,12 +82,12 @@ func NewClientManager(config *ClientConfig) (cm *ClientManager) {
expiredTokens: make(chan string),
expirationsLocker: &sync.Mutex{},
host: RootURL,
host: rootURL,
scheme: rootScheme,
hostLocker: sync.RWMutex{},
bridgeAuths: make(chan ClientAuth),
clientAuths: make(chan ClientAuth),
forwardedAuths: make(chan ClientAuth),
clientAuths: make(chan ClientAuth),
proxyProvider: newProxyProvider(dohProviders, proxyQuery),
proxyUseDuration: proxyUseDuration,
@ -211,7 +211,7 @@ func (cm *ClientManager) DisallowProxy() {
defer cm.hostLocker.Unlock()
cm.allowProxy = false
cm.host = RootURL
cm.host = rootURL
}
// IsProxyEnabled returns whether we are currently proxying requests.
@ -219,7 +219,7 @@ func (cm *ClientManager) IsProxyEnabled() bool {
cm.hostLocker.RLock()
defer cm.hostLocker.RUnlock()
return cm.host != RootURL
return cm.host != rootURL
}
// switchToReachableServer switches to using a reachable server (either proxy or standard API).
@ -236,12 +236,12 @@ func (cm *ClientManager) switchToReachableServer() (proxy string, err error) {
logrus.WithField("proxy", proxy).Info("Switching to a proxy")
// If the host is currently the RootURL, it's the first time we are enabling a proxy.
// If the host is currently the rootURL, it's the first time we are enabling a proxy.
// This means we want to disable it again in 24 hours.
if cm.host == RootURL {
if cm.host == rootURL {
go func() {
<-time.After(cm.proxyUseDuration)
cm.host = RootURL
cm.host = rootURL
}()
}
@ -260,12 +260,7 @@ func (cm *ClientManager) GetToken(userID string) string {
// GetAuthUpdateChannel returns a channel on which client auths can be received.
func (cm *ClientManager) GetAuthUpdateChannel() chan ClientAuth {
return cm.bridgeAuths
}
// GetClientAuthChannel returns a channel on which clients should send auths.
func (cm *ClientManager) GetClientAuthChannel() chan ClientAuth {
return cm.clientAuths
return cm.forwardedAuths
}
// Errors for possible connection issues
@ -330,19 +325,19 @@ func checkConnection(client *http.Client, url string, errorChannel chan error) {
errorChannel <- nil
}
// forwardClientAuths handles all incoming auths from clients before forwarding them on the bridge auth channel.
// forwardClientAuths handles all incoming auths from clients before forwarding them on the forwarded auths channel.
func (cm *ClientManager) forwardClientAuths() {
for auth := range cm.clientAuths {
logrus.Debug("ClientManager received auth from client")
cm.handleClientAuth(auth)
logrus.Debug("ClientManager is forwarding auth to bridge")
cm.bridgeAuths <- auth
logrus.Debug("ClientManager is forwarding auth")
cm.forwardedAuths <- auth
}
}
// SetTokenIfUnset sets the token for the given userID if it wasn't already set.
// setTokenIfUnset sets the token for the given userID if it wasn't already set.
// The set token does not expire.
func (cm *ClientManager) SetTokenIfUnset(userID, token string) {
func (cm *ClientManager) setTokenIfUnset(userID, token string) {
cm.tokensLocker.Lock()
defer cm.tokensLocker.Unlock()

View File

@ -22,13 +22,13 @@ import (
"runtime"
)
// RootURL is the API root URL.
// rootURL is the API root URL.
//
// This can be changed using build flags: pmapi_local for "localhost/api", pmapi_dev or pmapi_prod.
// Default is pmapi_prod.
//
// It must not contain the protocol! The protocol should be in rootScheme.
var RootURL = "api.protonmail.ch" //nolint[gochecknoglobals]
var rootURL = "api.protonmail.ch" //nolint[gochecknoglobals]
var rootScheme = "https" //nolint[gochecknoglobals]
// CurrentUserAgent is the default User-Agent for go-pmapi lib. This can be changed to program

View File

@ -20,6 +20,6 @@
package pmapi
func init() {
RootURL = "dev.protonmail.com/api"
rootURL = "dev.protonmail.com/api"
rootScheme = "https"
}

View File

@ -27,7 +27,7 @@ import (
func init() {
// Use port above 1000 which doesn't need root access to start anything on it.
// Now the port is rounded pi. :-)
RootURL = "127.0.0.1:3142/api"
rootURL = "127.0.0.1:3142/api"
rootScheme = "http"
// TLS certificate is self-signed

View File

@ -330,7 +330,7 @@ func (p *DialerWithPinning) dial(network, address string) (conn net.Conn, err er
// If we are not dialing the standard API then we should skip cert verification checks.
var tlsConfig *tls.Config = nil
if address != RootURL {
if address != rootURL {
tlsConfig = &tls.Config{InsecureSkipVerify: true} // nolint[gosec]
}

View File

@ -21,3 +21,8 @@ package pmapi
func (s *Auth) DANGEROUSLYSetUID(uid string) {
s.uid = uid
}
// GetClientAuthChannel returns a channel on which clients should send auths.
func (cm *ClientManager) GetClientAuthChannel() chan ClientAuth {
return cm.clientAuths
}

View File

@ -88,9 +88,9 @@ func (p *proxyProvider) findReachableServer() (proxy string, err error) {
logrus.WithError(err).Warn("Failed to refresh proxy cache, cache may be out of date")
}
// We want to switch back to the RootURL if possible.
if p.canReach(RootURL) {
proxyResult <- RootURL
// We want to switch back to the rootURL if possible.
if p.canReach(rootURL) {
proxyResult <- rootURL
return
}

View File

@ -198,7 +198,7 @@ func TestProxyProvider_UseProxy_RevertAfterTime(t *testing.T) {
require.Equal(t, proxy.URL, cm.getHost())
time.Sleep(2 * time.Second)
require.Equal(t, RootURL, cm.getHost())
require.Equal(t, rootURL, cm.getHost())
}
func TestProxyProvider_UseProxy_RevertIfProxyStopsWorkingAndOriginalAPIIsReachable(t *testing.T) {
@ -227,8 +227,8 @@ func TestProxyProvider_UseProxy_RevertIfProxyStopsWorkingAndOriginalAPIIsReachab
// We should now find the original API URL if it is working again.
url, err = cm.switchToReachableServer()
require.NoError(t, err)
require.Equal(t, RootURL, url)
require.Equal(t, RootURL, cm.getHost())
require.Equal(t, rootURL, url)
require.Equal(t, rootURL, cm.getHost())
}
func TestProxyProvider_UseProxy_FindSecondAlternativeIfFirstFailsAndAPIIsStillBlocked(t *testing.T) {
@ -298,14 +298,14 @@ func TestProxyProvider_DoHLookup_FindProxyFirstProviderUnreachable(t *testing.T)
}
// testAPIURLBackup is used to hold the globalOriginalURL because we clear it for test purposes and need to restore it.
var testAPIURLBackup = RootURL
var testAPIURLBackup = rootURL
// blockAPI prevents tests from reaching the standard API, forcing them to find a proxy.
func blockAPI() {
RootURL = ""
rootURL = ""
}
// unblockAPI allow tests to reach the standard API again.
func unblockAPI() {
RootURL = testAPIURLBackup
rootURL = testAPIURLBackup
}

View File

@ -27,8 +27,8 @@ import (
"runtime"
"strings"
"github.com/ProtonMail/proton-bridge/pkg/logs"
"github.com/kardianos/osext"
"github.com/sirupsen/logrus"
)
const (
@ -44,7 +44,7 @@ var (
)
var (
log = logs.GetLogEntry("bridgeUtils/updates") //nolint[gochecknoglobals]
log = logrus.WithField("pkg", "bridgeUtils/updates") //nolint[gochecknoglobals]
installFileSuffix = map[string]string{ //nolint[gochecknoglobals]
"darwin": ".dmg",

View File

@ -35,8 +35,8 @@ func BridgeChecksFeatureContext(s *godog.Suite) {
s.Step(`^"([^"]*)" does not have loaded store$`, userDoesNotHaveLoadedStore)
s.Step(`^"([^"]*)" has running event loop$`, userHasRunningEventLoop)
s.Step(`^"([^"]*)" does not have running event loop$`, userDoesNotHaveRunningEventLoop)
s.Step(`^"([^"]*)" does not have API auth$`, doesNotHaveAPIAuth)
s.Step(`^"([^"]*)" has API auth$`, hasAPIAuth)
s.Step(`^"([^"]*)" does not have API auth$`, isNotAuthorized)
s.Step(`^"([^"]*)" has API auth$`, isAuthorized)
}
func bridgeResponseIs(expectedResponse string) error {
@ -91,9 +91,7 @@ func userIsConnected(bddUserID string) error {
if err != nil {
return internalError(err, "getting user %s", account.Username())
}
a.Eventually(ctx.GetTestingT(), func() bool {
return bridgeUser.IsConnected()
}, 5*time.Second, 10*time.Millisecond)
a.Eventually(ctx.GetTestingT(), bridgeUser.IsConnected, 5*time.Second, 10*time.Millisecond)
a.NotEmpty(t, bridgeUser.GetPrimaryAddress())
a.NotEmpty(t, bridgeUser.GetStoreAddresses())
return ctx.GetTestingError()
@ -175,7 +173,7 @@ func userDoesNotHaveRunningEventLoop(bddUserID string) error {
return ctx.GetTestingError()
}
func hasAPIAuth(accountName string) error {
func isAuthorized(accountName string) error {
account := ctx.GetTestAccount(accountName)
if account == nil {
return godog.ErrPending
@ -184,14 +182,11 @@ func hasAPIAuth(accountName string) error {
if err != nil {
return internalError(err, "getting user %s", account.Username())
}
a.Eventually(ctx.GetTestingT(),
bridgeUser.HasAPIAuth,
5*time.Second, 10*time.Millisecond,
)
a.Eventually(ctx.GetTestingT(), bridgeUser.IsAuthorized, 5*time.Second, 10*time.Millisecond)
return ctx.GetTestingError()
}
func doesNotHaveAPIAuth(accountName string) error {
func isNotAuthorized(accountName string) error {
account := ctx.GetTestAccount(accountName)
if account == nil {
return godog.ErrPending
@ -200,6 +195,6 @@ func doesNotHaveAPIAuth(accountName string) error {
if err != nil {
return internalError(err, "getting user %s", account.Username())
}
a.False(ctx.GetTestingT(), bridgeUser.HasAPIAuth())
a.Eventually(ctx.GetTestingT(), func() bool { return !bridgeUser.IsAuthorized() }, 5*time.Second, 10*time.Millisecond)
return ctx.GetTestingError()
}

View File

@ -65,7 +65,7 @@ func (api *FakePMAPI) Auth(username, password string, authInfo *pmapi.AuthInfo)
auth := &pmapi.Auth{
TwoFA: user.get2FAInfo(),
RefreshToken: session.refreshToken,
ExpiresIn: 86400,
ExpiresIn: 86400, // seconds
}
auth.DANGEROUSLYSetUID(session.uid)

View File

@ -28,7 +28,7 @@ func (api *FakePMAPI) GetEvent(eventID string) (*pmapi.Event, error) {
// Request for empty ID returns the latest event.
if eventID == "" {
if len(api.events) == 0 {
return &pmapi.Event{EventID: ""}, nil
return &pmapi.Event{EventID: "first-event-id"}, nil
}
return api.events[len(api.events)-1], nil
}