From 1803a1723c2952723a4ec9ad67e3cd1184fcf137 Mon Sep 17 00:00:00 2001
From: Daniel Friesel <derf@finalrewind.org>
Date: Sun, 13 Jun 2021 08:48:08 +0200
Subject: [PATCH] move user name validation to Users model

---
 lib/Travelynx/Controller/Account.pm | 37 +++++++----------------------
 lib/Travelynx/Model/Users.pm        | 26 ++++++++++++++++++++
 2 files changed, 34 insertions(+), 29 deletions(-)

diff --git a/lib/Travelynx/Controller/Account.pm b/lib/Travelynx/Controller/Account.pm
index 31ba434..9c161e0 100644
--- a/lib/Travelynx/Controller/Account.pm
+++ b/lib/Travelynx/Controller/Account.pm
@@ -85,8 +85,8 @@ sub register {
 		return;
 	}
 
-	if ( not length($user) ) {
-		$self->render( 'register', invalid => 'user_empty' );
+	if ( my $error = $self->users->is_name_invalid( name => $user ) ) {
+		$self->render( 'register', invalid => $error );
 		return;
 	}
 
@@ -95,16 +95,6 @@ sub register {
 		return;
 	}
 
-	if ( $user !~ m{ ^ [0-9a-zA-Z_-]+ $ }x ) {
-		$self->render( 'register', invalid => 'user_format' );
-		return;
-	}
-
-	if ( $self->users->user_name_exists( name => $user ) ) {
-		$self->render( 'register', invalid => 'user_collision' );
-		return;
-	}
-
 	if ( $self->users->mail_is_blacklisted( email => $email ) ) {
 		$self->render( 'register', invalid => 'mail_blacklisted' );
 		return;
@@ -485,13 +475,8 @@ sub change_name {
 			return;
 		}
 
-		if ( not length($new_name) ) {
-			$self->render( 'change_name', invalid => 'user_empty' );
-			return;
-		}
-
-		if ( $new_name !~ m{ ^ [0-9a-zA-Z_-]+ $ }x ) {
-			$self->render( 'change_name', invalid => 'user_format' );
+		if ( my $error = $self->users->is_name_invalid( name => $new_name ) ) {
+			$self->render( 'change_name', invalid => $error );
 			return;
 		}
 
@@ -500,16 +485,10 @@ sub change_name {
 			return;
 		}
 
-       # This call is technically superfluous. The users table has a unique
-       # constraint on the "name" column, so having two users with the same name
-       # is not possible. However, to minimize the number of failed SQL
-       # queries, we first do a select check here and only attempt an update
-       # if it succeeded.
-		if ( $self->users->user_name_exists( name => $new_name ) ) {
-			$self->render( 'change_name', invalid => 'user_collision' );
-			return;
-		}
-
+       # The users table has a unique constraint on the "name" column, so having
+       # two users with the same name is not possible. The race condition
+       # between the user_name_exists check in is_name_invalid and this
+       # change_name call is harmless.
 		my $success = $self->users->change_name(
 			uid  => $self->current_user->{id},
 			name => $new_name
diff --git a/lib/Travelynx/Model/Users.pm b/lib/Travelynx/Model/Users.pm
index 8bd60f0..535b938 100644
--- a/lib/Travelynx/Model/Users.pm
+++ b/lib/Travelynx/Model/Users.pm
@@ -201,6 +201,32 @@ sub change_mail_with_token {
 	return;
 }
 
+sub is_name_invalid {
+	my ( $self, %opt ) = @_;
+	my $db   = $opt{db} // $self->{pg}->db;
+	my $name = $opt{name};
+
+	if ( not length($name) ) {
+		return 'user_empty';
+	}
+
+	if ( $name !~ m{ ^ [0-9a-zA-Z_-]+ $ }x ) {
+		return 'user_format';
+	}
+
+	if (
+		$self->user_name_exists(
+			db   => $db,
+			name => $name
+		)
+	  )
+	{
+		return 'user_collision';
+	}
+
+	return;
+}
+
 sub change_name {
 	my ( $self, %opt ) = @_;
 	my $db  = $opt{db} // $self->{pg}->db;