fix the annoying infinite handshake bug (tested) (#69)

internal/federation/federator.go

@@ -21,6 +21,7 @@ package federation
 import (
 	"net/http"
 	"net/url"
+	"sync"
 
 	"github.com/go-fed/activity/pub"
 	"github.com/sirupsen/logrus"
@@ -54,6 +55,8 @@ type Federator interface {
 	//
 	// If username is an empty string, our instance user's credentials will be used instead.
 	GetTransportForUser(username string) (transport.Transport, error)
+	// Handshaking returns true if the given username is currently in the process of dereferencing the remoteAccountID.
+	Handshaking(username string, remoteAccountID *url.URL) bool
 	pub.CommonBehavior
 	pub.FederatingProtocol
 }
@@ -67,6 +70,8 @@ type federator struct {
 	transportController transport.Controller
 	actor               pub.FederatingActor
 	log                 *logrus.Logger
+	handshakes          map[string][]*url.URL
+	handshakeSync       *sync.Mutex // mutex to lock/unlock when checking or updating the handshakes map
 }
 
 // NewFederator returns a new federator
@@ -81,6 +86,7 @@ func NewFederator(db db.DB, federatingDB federatingdb.DB, transportController tr
 		typeConverter:       typeConverter,
 		transportController: transportController,
 		log:                 log,
+		handshakeSync:       &sync.Mutex{},
 	}
 	actor := newFederatingActor(f, f, federatingDB, clock)
 	f.actor = actor

internal/federation/handshake.go

@@ -0,0 +1,80 @@
+package federation
+
+import "net/url"
+
+func (f *federator) Handshaking(username string, remoteAccountID *url.URL) bool {
+	f.handshakeSync.Lock()
+	defer f.handshakeSync.Unlock()
+
+	if f.handshakes == nil {
+		// handshakes isn't even initialized yet so we can't be handshaking with anyone
+		return false
+	}
+
+	remoteIDs, ok := f.handshakes[username];
+	if !ok {
+		// user isn't handshaking with anyone, bail
+		return false
+	}
+
+	for _, id := range remoteIDs {
+		if id.String() == remoteAccountID.String() {
+			// we are currently handshaking with the remote account, yep
+			return true
+		}
+	}
+
+	// didn't find it which means we're not handshaking
+	return false
+}
+
+func (f *federator) startHandshake(username string, remoteAccountID *url.URL) {
+	f.handshakeSync.Lock()
+	defer f.handshakeSync.Unlock()
+
+	// lazily initialize handshakes
+	if f.handshakes == nil {
+		f.handshakes = make(map[string][]*url.URL)
+	}
+
+	remoteIDs, ok := f.handshakes[username]
+	if !ok {
+		// there was nothing in there yet, so just add this entry and return
+		f.handshakes[username] = []*url.URL{remoteAccountID}
+		return
+	}
+
+	// add the remote ID to the slice
+	remoteIDs = append(remoteIDs, remoteAccountID)
+	f.handshakes[username] = remoteIDs
+}
+
+func (f *federator) stopHandshake(username string, remoteAccountID *url.URL) {
+	f.handshakeSync.Lock()
+	defer f.handshakeSync.Unlock()
+
+	if f.handshakes == nil {
+		return
+	}
+
+	remoteIDs, ok := f.handshakes[username]
+	if !ok {
+		// there was nothing in there yet anyway so just bail
+		return
+	}
+
+	newRemoteIDs := []*url.URL{}
+	for _, id := range remoteIDs {
+		if id.String() != remoteAccountID.String() {
+			newRemoteIDs = append(newRemoteIDs, id)
+		}
+	}
+
+	if len(newRemoteIDs) == 0 {
+		// there are no handshakes so just remove this user entry from the map and save a few bytes
+		delete(f.handshakes, username)
+	} else {
+		// there are still other handshakes ongoing
+		f.handshakes[username] = newRemoteIDs
+	}
+}

internal/federation/util.go

@@ -213,6 +213,8 @@ func (f *federator) AuthenticateFederatedRequest(username string, r *http.Reques
 }
 
 func (f *federator) DereferenceRemoteAccount(username string, remoteAccountID *url.URL) (typeutils.Accountable, error) {
+	f.startHandshake(username, remoteAccountID)
+	defer f.stopHandshake(username, remoteAccountID)
 
 	transport, err := f.GetTransportForUser(username)
 	if err != nil {

internal/processing/federation.go

@@ -26,7 +26,6 @@ import (
 
 	"github.com/go-fed/activity/streams"
 	"github.com/go-fed/activity/streams/vocab"
-	"github.com/sirupsen/logrus"
 	apimodel "github.com/superseriousbusiness/gotosocial/internal/api/model"
 	"github.com/superseriousbusiness/gotosocial/internal/db"
 	"github.com/superseriousbusiness/gotosocial/internal/gtserror"
@@ -35,23 +34,16 @@ import (
 	"github.com/superseriousbusiness/gotosocial/internal/util"
 )
 
-// authenticateAndDereferenceFediRequest authenticates the HTTP signature of an incoming federation request, using the given
+// dereferenceFediRequest authenticates the HTTP signature of an incoming federation request, using the given
 // username to perform the validation. It will *also* dereference the originator of the request and return it as a gtsmodel account
 // for further processing. NOTE that this function will have the side effect of putting the dereferenced account into the database,
 // and passing it into the processor through a channel for further asynchronous processing.
-func (p *processor) authenticateAndDereferenceFediRequest(username string, r *http.Request) (*gtsmodel.Account, error) {
-
-	// first authenticate
-	requestingAccountURI, err := p.federator.AuthenticateFederatedRequest(username, r)
-	if err != nil {
-		return nil, fmt.Errorf("couldn't authenticate request for username %s: %s", username, err)
-	}
-
+func (p *processor) dereferenceFediRequest(username string, requestingAccountURI *url.URL) (*gtsmodel.Account, error) {
 	// OK now we can do the dereferencing part
 	// we might already have an entry for this account so check that first
 	requestingAccount := &gtsmodel.Account{}
 
-	err = p.db.GetWhere([]db.Where{{Key: "uri", Value: requestingAccountURI.String()}}, requestingAccount)
+	err := p.db.GetWhere([]db.Where{{Key: "uri", Value: requestingAccountURI.String()}}, requestingAccount)
 	if err == nil {
 		// we do have it yay, return it
 		return requestingAccount, nil
@@ -98,12 +90,6 @@ func (p *processor) authenticateAndDereferenceFediRequest(username string, r *ht
 }
 
 func (p *processor) GetFediUser(requestedUsername string, request *http.Request) (interface{}, gtserror.WithCode) {
-	l := p.log.WithFields(logrus.Fields{
-		"func":              "GetFediUser",
-		"requestedUsername": requestedUsername,
-		"requestURL":        request.URL.String(),
-	})
-
 	// get the account the request is referring to
 	requestedAccount := &gtsmodel.Account{}
 	if err := p.db.GetLocalAccountByUsername(requestedUsername, requestedAccount); err != nil {
@@ -113,16 +99,21 @@ func (p *processor) GetFediUser(requestedUsername string, request *http.Request)
 	var requestedPerson vocab.ActivityStreamsPerson
 	var err error
 	if util.IsPublicKeyPath(request.URL) {
-		l.Debug("serving from public key path")
 		// if it's a public key path, we don't need to authenticate but we'll only serve the bare minimum user profile needed for the public key
 		requestedPerson, err = p.tc.AccountToASMinimal(requestedAccount)
 		if err != nil {
 			return nil, gtserror.NewErrorInternalError(err)
 		}
 	} else if util.IsUserPath(request.URL) {
-		l.Debug("serving from user path")
 		// if it's a user path, we want to fully authenticate the request before we serve any data, and then we can serve a more complete profile
-		requestingAccount, err := p.authenticateAndDereferenceFediRequest(requestedUsername, request)
+		requestingAccountURI, err := p.federator.AuthenticateFederatedRequest(requestedUsername, request)
+		if err != nil {
+			return nil, gtserror.NewErrorNotAuthorized(err)
+		}
+
+		// if we're already handshaking/dereferencing a remote account, we can skip the dereferencing part
+		if !p.federator.Handshaking(requestedUsername, requestingAccountURI) {
+			requestingAccount, err := p.dereferenceFediRequest(requestedUsername, requestingAccountURI)
 			if err != nil {
 				return nil, gtserror.NewErrorNotAuthorized(err)
 			}
@@ -135,6 +126,8 @@ func (p *processor) GetFediUser(requestedUsername string, request *http.Request)
 			if blocked {
 				return nil, gtserror.NewErrorNotAuthorized(fmt.Errorf("block exists between accounts %s and %s", requestedAccount.ID, requestingAccount.ID))
 			}
+		}
+
 		requestedPerson, err = p.tc.AccountToAS(requestedAccount)
 		if err != nil {
 			return nil, gtserror.NewErrorInternalError(err)
@@ -159,7 +152,12 @@ func (p *processor) GetFediFollowers(requestedUsername string, request *http.Req
 	}
 
 	// authenticate the request
-	requestingAccount, err := p.authenticateAndDereferenceFediRequest(requestedUsername, request)
+	requestingAccountURI, err := p.federator.AuthenticateFederatedRequest(requestedUsername, request)
+	if err != nil {
+		return nil, gtserror.NewErrorNotAuthorized(err)
+	}
+
+	requestingAccount, err := p.dereferenceFediRequest(requestedUsername, requestingAccountURI)
 	if err != nil {
 		return nil, gtserror.NewErrorNotAuthorized(err)
 	}
@@ -199,7 +197,12 @@ func (p *processor) GetFediFollowing(requestedUsername string, request *http.Req
 	}
 
 	// authenticate the request
-	requestingAccount, err := p.authenticateAndDereferenceFediRequest(requestedUsername, request)
+	requestingAccountURI, err := p.federator.AuthenticateFederatedRequest(requestedUsername, request)
+	if err != nil {
+		return nil, gtserror.NewErrorNotAuthorized(err)
+	}
+
+	requestingAccount, err := p.dereferenceFediRequest(requestedUsername, requestingAccountURI)
 	if err != nil {
 		return nil, gtserror.NewErrorNotAuthorized(err)
 	}
@@ -239,7 +242,12 @@ func (p *processor) GetFediStatus(requestedUsername string, requestedStatusID st
 	}
 
 	// authenticate the request
-	requestingAccount, err := p.authenticateAndDereferenceFediRequest(requestedUsername, request)
+	requestingAccountURI, err := p.federator.AuthenticateFederatedRequest(requestedUsername, request)
+	if err != nil {
+		return nil, gtserror.NewErrorNotAuthorized(err)
+	}
+
+	requestingAccount, err := p.dereferenceFediRequest(requestedUsername, requestingAccountURI)
 	if err != nil {
 		return nil, gtserror.NewErrorNotAuthorized(err)
 	}