From 754fb79cddf3972a464be9ef64da7559cd02be24 Mon Sep 17 00:00:00 2001
From: Shivaram Lingamneni <slingamn@cs.stanford.edu>
Date: Wed, 7 Oct 2020 08:54:46 -0400
Subject: [PATCH] review fixes

---
 irc/accounts.go          | 33 +++++++++++----------------------
 irc/client_lookup_set.go |  7 ++++---
 2 files changed, 15 insertions(+), 25 deletions(-)

diff --git a/irc/accounts.go b/irc/accounts.go
index 6a2da92b..94e51513 100644
--- a/irc/accounts.go
+++ b/irc/accounts.go
@@ -403,28 +403,17 @@ func (am *AccountManager) Register(client *Client, account string, callbackNames
 		return errLimitExceeded
 	}
 
-	// if nick reservation is enabled, you can only register your current nickname
-	// as an account; this prevents "land-grab" situations where someone else
-	// registers your nick out from under you and then NS GHOSTs you
-	// n.b. client is nil during a SAREGISTER
-	// n.b. if ForceGuestFormat, then there's no concern, because you can't
-	// register a guest nickname anyway, and the actual registration system
-	// will prevent any double-register
-	if client != nil {
-		if client.registered {
-			if config.Accounts.NickReservation.Enabled &&
-				!config.Accounts.NickReservation.ForceGuestFormat &&
-				client.NickCasefolded() != casefoldedAccount {
-				return errAccountMustHoldNick
-			}
-		} else {
-			// XXX this is a REGISTER command from a client who hasn't completed the
-			// initial handshake ("connection registration"). Do SetNick with dryRun=true,
-			// testing whether they are able to claim the nick
-			_, nickAcquireError, _ := am.server.clients.SetNick(client, nil, account, true)
-			if !(nickAcquireError == nil || nickAcquireError == errNoop) {
-				return errAccountMustHoldNick
-			}
+	// if nick reservation is enabled, don't let people reserve nicknames
+	// that they would not be eligible to take, e.g.,
+	// 1. a nickname that someone else is currently holding
+	// 2. a nickname confusable with an existing reserved nickname
+	// this has a lot of weird edge cases because of force-guest-format
+	// and the possibility of registering a nickname on an "unregistered connection"
+	// (i.e., pre-handshake).
+	if client != nil && config.Accounts.NickReservation.Enabled {
+		_, nickAcquireError, _ := am.server.clients.SetNick(client, nil, account, true)
+		if !(nickAcquireError == nil || nickAcquireError == errNoop) {
+			return errAccountMustHoldNick
 		}
 	}
 
diff --git a/irc/client_lookup_set.go b/irc/client_lookup_set.go
index d7bdaed8..519a3bb5 100644
--- a/irc/client_lookup_set.go
+++ b/irc/client_lookup_set.go
@@ -142,7 +142,7 @@ func (clients *ClientManager) SetNick(client *Client, session *Session, newNick
 			return "", errNickMissing, false
 		}
 
-		if account == "" && config.Accounts.NickReservation.ForceGuestFormat {
+		if account == "" && config.Accounts.NickReservation.ForceGuestFormat && !dryRun {
 			newCfNick, err = CasefoldName(newNick)
 			if err != nil {
 				return "", errNicknameInvalid, false
@@ -199,9 +199,10 @@ func (clients *ClientManager) SetNick(client *Client, session *Session, newNick
 
 	currentClient := clients.byNick[newCfNick]
 	// the client may just be changing case
-	if currentClient != nil && currentClient != client && session != nil {
+	if currentClient != nil && currentClient != client {
 		// these conditions forbid reattaching to an existing session:
-		if registered || !bouncerAllowed || account == "" || account != currentClient.Account() {
+		if registered || !bouncerAllowed || account == "" || account != currentClient.Account() ||
+			dryRun || session == nil {
 			return "", errNicknameInUse, false
 		}
 		// check TLS modes