rpc: reject RegisterSession when creating an account for existing user (#76)

* rpc: reject RegisterSession when creating an account for existing user

* add more explanation to the RegisterSession flow

* small comment grammar fixes

Author: patrislav

data/account.go

@@ -128,6 +128,25 @@ func (t *AccountTable) Get(ctx context.Context, projectID uint64, identity proto
 	return &acct, true, nil
 }
 
+func (t *AccountTable) ExistsByUserID(ctx context.Context, userID string) (bool, error) {
+	input := &dynamodb.QueryInput{
+		TableName:              &t.tableARN,
+		IndexName:              &t.indices.ByUserID,
+		KeyConditionExpression: aws.String("UserID = :userID"),
+		ExpressionAttributeValues: map[string]types.AttributeValue{
+			":userID": &types.AttributeValueMemberS{Value: userID},
+		},
+		Limit: aws.Int32(1),
+	}
+
+	out, err := t.db.Query(ctx, input)
+	if err != nil {
+		return false, fmt.Errorf("Query: %w", err)
+	}
+
+	return len(out.Items) > 0, nil
+}
+
 // ListByUserID returns all Accounts of a given user.
 //
 // TODO: implement pagination.

rpc/sessions.go

@@ -118,7 +118,27 @@ func (s *RPC) RegisterSession(
 		return nil, nil, proto.ErrWebrpcInternalError.WithCausef("failed to retrieve account: %w", err)
 	}
 
+	// If there's no account for this identity then we know it's used for the first time. Prepare the account to be
+	// created at the end of the process.
 	if !accountFound {
+		// The user ID is deterministic and derived from the first session ever used by the user.
+		userID := fmt.Sprintf("%d|%s", tntData.ProjectID, sessionHash)
+
+		// If the user already has an account (of a different identity), we need to reject this intent. Otherwise, it
+		// would result in an accidental account federation as a new identity is connected to an existing user through
+		// unintended method.
+		userExists, err := s.Accounts.ExistsByUserID(ctx, userID)
+		if err != nil {
+			return nil, nil, proto.ErrWebrpcInternalError.WithCausef("failed to check if user exists: %w", err)
+		}
+		if userExists {
+			return nil, nil, proto.ErrWebrpcBadRequest.WithCausef("user already exists")
+		}
+
+		// Warn the user if another account already exists with the same email address. This allows them to go back and
+		// sign in using the other identity and then use account federation to add this one.
+		// Otherwise, this would result in a creation of a new user and thus a separate wallet and that's very unlikely
+		// to be the user's intent.
 		if !intentTyped.Data.ForceCreateAccount && ident.Email != "" {
 			accs, err := s.Accounts.ListByEmail(ctx, tntData.ProjectID, ident.Email)
 			if err != nil {
@@ -135,7 +155,7 @@ func (s *RPC) RegisterSession(
 
 		accData := &proto.AccountData{
 			ProjectID: tntData.ProjectID,
-			UserID:    fmt.Sprintf("%d|%s", tntData.ProjectID, sessionHash),
+			UserID:    userID,
 			Identity:  ident.String(),
 			CreatedAt: time.Now(),
 		}
@@ -144,6 +164,7 @@ func (s *RPC) RegisterSession(
 			return nil, nil, proto.ErrWebrpcInternalError.WithCausef("encrypting account data: %w", err)
 		}
 
+		// This account is inserted to the DB later once the WaaS API returns successfully.
 		account = &data.Account{
 			ProjectID:          tntData.ProjectID,
 			Identity:           data.Identity(ident),
@@ -157,11 +178,17 @@ func (s *RPC) RegisterSession(
 		}
 	}
 
+	// This calls the Sequence WaaS API. No changes to the DB were done yet up to this point, and we can only execute
+	// them if the call is successful.
+	// Note that if we return an error *after* this and *before* the DB is updated, we risk having data desync between
+	// the enclave and the guard. This is a dangerous state to be in, so this call is expected -- and assumed -- to be
+	// idempotent. Retrying it with the same input is safe.
 	res, err := s.Wallets.RegisterSession(waasapi.Context(ctx), account.UserID, waasapi.ConvertToAPIIntent(intent))
 	if err != nil {
 		return nil, nil, proto.ErrWebrpcInternalError.WithCausef("registering session with WaaS API: %w", err)
 	}
 
+	// Insert an account if it's new OR update it with a fresh email if it differs from what we have in the DB.
 	if !accountFound || (ident.Email != "" && account.Email != ident.Email) {
 		account.Email = ident.Email
 		if err := s.Accounts.Put(ctx, account); err != nil {