From 49360bcbf3da39f9bed2908ae2fea57454c7ba7b Mon Sep 17 00:00:00 2001
From: Julian Rother <julian@cccv.de>
Date: Thu, 9 Dec 2021 03:29:45 +0100
Subject: [PATCH] Enforce alphabet and length constraints for group names

Closes #127
---
 tests/test_user.py                  | 32 ++++++++++++++++++++++++++---
 uffd/user/cli_group.py              |  5 ++++-
 uffd/user/models.py                 |  9 ++++++++
 uffd/user/templates/group/show.html |  5 ++++-
 uffd/user/views_group.py            |  4 +++-
 5 files changed, 49 insertions(+), 6 deletions(-)

diff --git a/tests/test_user.py b/tests/test_user.py
index fbc06a2a..358c2f2f 100644
--- a/tests/test_user.py
+++ b/tests/test_user.py
@@ -506,6 +506,33 @@ class TestGroupViews(UffdTestCase):
 		self.assertEqual(group.description, 'Original description')
 		self.assertEqual(group.unix_gid, gid)
 
+	def test_new_name_too_long(self):
+		r = self.client.post(path=url_for('group.update'),
+			data={'unix_gid': '', 'name': 'a'*33, 'description': 'New description'},
+			follow_redirects=True)
+		dump('group_new_name_too_long', r)
+		self.assertEqual(r.status_code, 400)
+		group = Group.query.filter_by(name='a'*33).one_or_none()
+		self.assertIsNone(group)
+
+	def test_new_name_too_short(self):
+		r = self.client.post(path=url_for('group.update'),
+			data={'unix_gid': '', 'name': '', 'description': 'New description'},
+			follow_redirects=True)
+		dump('group_new_name_too_short', r)
+		self.assertEqual(r.status_code, 400)
+		group = Group.query.filter_by(name='').one_or_none()
+		self.assertIsNone(group)
+
+	def test_new_name_invalid(self):
+		r = self.client.post(path=url_for('group.update'),
+			data={'unix_gid': '', 'name': 'foo bar', 'description': 'New description'},
+			follow_redirects=True)
+		dump('group_new_name_invalid', r)
+		self.assertEqual(r.status_code, 400)
+		group = Group.query.filter_by(name='foo bar').one_or_none()
+		self.assertIsNone(group)
+
 	def test_new_existing_gid(self):
 		gid = self.app.config['GROUP_MAX_GID'] - 1
 		db.session.add(Group(name='newgroup', description='Original description', unix_gid=gid))
@@ -571,9 +598,8 @@ class TestGroupCLI(UffdTestCase):
 	def test_create(self):
 		result = self.app.test_cli_runner().invoke(args=['group', 'create', 'users']) # Duplicate name
 		self.assertEqual(result.exit_code, 1)
-		# See #127 (Enforce alphabet and length constraints for Group.name)
-		#result = self.app.test_cli_runner().invoke(args=['group', 'create', 'new group'])
-		#self.assertEqual(result.exit_code, 1)
+		result = self.app.test_cli_runner().invoke(args=['group', 'create', 'new group'])
+		self.assertEqual(result.exit_code, 1)
 		result = self.app.test_cli_runner().invoke(args=['group', 'create', 'newgroup', '--description', 'A new group'])
 		self.assertEqual(result.exit_code, 0)
 		with self.app.test_request_context():
diff --git a/uffd/user/cli_group.py b/uffd/user/cli_group.py
index 753a3a50..3627f0a0 100644
--- a/uffd/user/cli_group.py
+++ b/uffd/user/cli_group.py
@@ -37,8 +37,11 @@ def show(name):
 @click.option('--description', default='', help='Set description text. Empty per default.')
 def create(name, description):
 	with current_app.test_request_context():
+		group = Group(description=description)
+		if not group.set_name(name):
+			raise click.ClickException('Invalid name')
 		try:
-			db.session.add(Group(name=name, description=description))
+			db.session.add(group)
 			db.session.commit()
 		except IntegrityError as ex:
 			raise click.ClickException(f'Group creation failed: {ex}')
diff --git a/uffd/user/models.py b/uffd/user/models.py
index 51e6793e..4878ba0d 100644
--- a/uffd/user/models.py
+++ b/uffd/user/models.py
@@ -170,3 +170,12 @@ class Group(db.Model):
 	name = Column(String(32), unique=True, nullable=False)
 	description = Column(String(128), nullable=False, default='')
 	members = relationship('User', secondary='user_groups')
+
+	def set_name(self, value):
+		if len(value) > 32 or len(value) < 1:
+			return False
+		for char in value:
+			if not char in string.ascii_lowercase + string.digits + '_-':
+				return False
+		self.name = value
+		return True
diff --git a/uffd/user/templates/group/show.html b/uffd/user/templates/group/show.html
index cc4f7970..698d4836 100644
--- a/uffd/user/templates/group/show.html
+++ b/uffd/user/templates/group/show.html
@@ -24,7 +24,10 @@
 	</div>
 	<div class="form-group col">
 		<label for="group-loginname">{{_("Name")}}</label>
-		<input type="text" class="form-control" id="group-loginname" name="name" value="{{ group.name or '' }}" {{ 'readonly' if group.id }}>
+		<input type="text" class="form-control" id="group-loginname" name="name" minlength=1 maxlength=32 pattern="[a-z0-9_-]*" value="{{ group.name or '' }}" {{ 'readonly' if group.id }}>
+		<small class="form-text text-muted">
+			{{_('At least one and at most 32 lower-case characters, digits, dashes ("-") or underscores ("_"). <b>Cannot be changed later!</b>')|safe}}
+		</small>
 	</div>
 	<div class="form-group col">
 		<label for="group-description">{{_("Description")}}</label>
diff --git a/uffd/user/views_group.py b/uffd/user/views_group.py
index 5e1bdf0c..63d281f4 100644
--- a/uffd/user/views_group.py
+++ b/uffd/user/views_group.py
@@ -38,7 +38,9 @@ def update(id=None):
 		group = Group()
 		if request.form['unix_gid']:
 			group.unix_gid = int(request.form['unix_gid'])
-		group.name = request.form['name']
+		if not group.set_name(request.form['name']):
+			flash(_('Invalid name'))
+			return render_template('group/show.html', group=group), 400
 	else:
 		group = Group.query.get_or_404(id)
 	group.description = request.form['description']
-- 
GitLab