Merge pull request #56 from phpbb/ticket/security-250

[ticket/security-250] Check form key when approving group membership

3 files changed, 94 insertions, 12 deletions

diff --git a/phpBB/includes/ucp/ucp_groups.php b/phpBB/includes/ucp/ucp_groups.php
index 24b94126b0..cf6e049748 100644
--- a/phpBB/includes/ucp/ucp_groups.php
+++ b/phpBB/includes/ucp/ucp_groups.php
@@ -875,6 +875,11 @@ class ucp_groups
 							trigger_error($user->lang['NO_GROUP'] . $return_page);
 						}
 
+						if (!check_form_key('ucp_groups'))
+						{
+							trigger_error($user->lang('FORM_INVALID') . $return_page);
+						}
+
 						if (!($row = group_memberships($group_id, $user->data['user_id'])))
 						{
 							trigger_error($user->lang['NOT_MEMBER_OF_GROUP'] . $return_page);
diff --git a/tests/functional/ucp_groups_test.php b/tests/functional/ucp_groups_test.php
index cd18a0fcae..445c124158 100644
--- a/tests/functional/ucp_groups_test.php
+++ b/tests/functional/ucp_groups_test.php
@@ -54,4 +54,72 @@ class phpbb_functional_ucp_groups_test extends phpbb_functional_common_groups_te
 		$this->assertContains($this->lang('GROUP_UPDATED'), $crawler->text());
 		$this->assertEquals($teampage_settings, $this->get_teampage_settings());
 	}
+
+	public function test_create_request_group()
+	{
+		$this->login();
+		$this->admin_login();
+		$this->add_lang('acp/groups');
+
+		$crawler = self::request('GET', 'adm/index.php?i=acp_groups&mode=manage&sid=' . $this->sid);
+		$form = $crawler->selectButton($this->lang('SUBMIT'))->form();
+		$crawler = self::submit($form, array('group_name' => 'request-group'));
+
+		$form = $crawler->selectButton($this->lang('SUBMIT'))->form();
+		$crawler = self::submit($form, array('group_name' => 'request-group'));
+
+		$this->assertContainsLang('GROUP_CREATED', $crawler->filter('#main')->text());
+
+		$group_id = $this->get_group_id('request-group');
+
+		// Make admin group leader
+		$crawler = self::request('GET', 'adm/index.php?i=acp_groups&mode=manage&action=list&g=' . $group_id . '&sid=' . $this->sid);
+		$form = $crawler->filter('input[name=addusers]')->selectButton($this->lang('SUBMIT'))->form();
+		$crawler = self::submit($form, [
+			'leader'	=> 1,
+			'usernames'	=> 'admin',
+		]);
+
+		$this->assertContainsLang('GROUP_MODS_ADDED', $crawler->filter('#main')->text());
+	}
+
+	/**
+	 * @depends test_create_request_group
+	 */
+	public function test_request_group_membership()
+	{
+		$this->create_user('request-group-user');
+		$this->login('request-group-user');
+		$this->add_lang('groups');
+
+		$group_id = $this->get_group_id('request-group');
+
+		$crawler = self::request('GET', 'ucp.php?i=ucp_groups&mode=membership&sid=' . $this->sid);
+		$form = $crawler->selectButton($this->lang('SUBMIT'))->form();
+		$crawler = self::submit($form, ['selected' => $group_id, 'action' => 'join']);
+		$this->assertContainsLang('GROUP_JOIN_PENDING_CONFIRM', $crawler->text());
+
+		$form = $crawler->selectButton($this->lang('YES'))->form();
+		$crawler = self::submit($form);
+		$this->assertContainsLang('GROUP_JOINED_PENDING', $crawler->text());
+	}
+
+	/**
+	 * @depends test_request_group_membership
+	 */
+	public function test_approve_group_membership()
+	{
+		$this->login();
+		$this->add_lang('acp/groups');
+
+		$group_id = $this->get_group_id('request-group');
+		$crawler = self::request('GET', 'ucp.php?i=ucp_groups&mode=manage&action=list&g=' . $group_id . '&sid=' . $this->sid);
+		$form = $crawler->filter('input[name=update]')->selectButton($this->lang('SUBMIT'))->form();
+		$crawler = self::submit($form, [
+			'mark'	=> [$crawler->filter('input[name="mark[]"]')->first()->attr('value')],
+			'action'	=> 'approve',
+		]);
+
+		$this->assertContainsLang('USERS_APPROVED', $crawler->text());
+	}
 }
diff --git a/tests/test_framework/phpbb_functional_test_case.php b/tests/test_framework/phpbb_functional_test_case.php
index 2659cf6e73..f1b30f0fed 100644
--- a/tests/test_framework/phpbb_functional_test_case.php
+++ b/tests/test_framework/phpbb_functional_test_case.php
@@ -623,6 +623,25 @@ class phpbb_functional_test_case extends phpbb_test_case
 		return user_add($user_row);
 	}
 
+	/**
+	 * Get group ID
+	 *
+	 * @param string $group_name Group name
+	 * @return int Group id of specified group name
+	 */
+	protected function get_group_id($group_name)
+	{
+		$db = $this->get_db();
+		$sql = 'SELECT group_id
+			FROM ' . GROUPS_TABLE . "
+			WHERE group_name = '" . $db->sql_escape($group_name) . "'";
+		$result = $db->sql_query($sql);
+		$group_id = (int) $db->sql_fetchfield('group_id');
+		$db->sql_freeresult($result);
+
+		return $group_id;
+	}
+
 	protected function remove_user_group($group_name, $usernames)
 	{
 		global $db, $cache, $auth, $config, $phpbb_dispatcher, $phpbb_log, $phpbb_container, $phpbb_root_path, $phpEx;
@@ -655,12 +674,7 @@ class phpbb_functional_test_case extends phpbb_test_case
 			require_once(__DIR__ . '/../../phpBB/includes/functions_user.php');
 		}
 
-		$sql = 'SELECT group_id
-			FROM ' . GROUPS_TABLE . "
-			WHERE group_name = '" . $db->sql_escape($group_name) . "'";
-		$result = $db->sql_query($sql);
-		$group_id = (int) $db->sql_fetchfield('group_id');
-		$db->sql_freeresult($result);
+		$group_id = $this->get_group_id($group_name);
 
 		return group_user_del($group_id, false, $usernames, $group_name);
 	}
@@ -700,12 +714,7 @@ class phpbb_functional_test_case extends phpbb_test_case
 			require_once(__DIR__ . '/../../phpBB/includes/functions_user.php');
 		}
 
-		$sql = 'SELECT group_id
-			FROM ' . GROUPS_TABLE . "
-			WHERE group_name = '" . $db->sql_escape($group_name) . "'";
-		$result = $db->sql_query($sql);
-		$group_id = (int) $db->sql_fetchfield('group_id');
-		$db->sql_freeresult($result);
+		$group_id = $this->get_group_id($group_name);
 
 		return group_user_add($group_id, false, $usernames, $group_name, $default, $leader);
 	}