Thread Rating:
  • 0 Votes - 0 Average
  • 1
  • 2
  • 3
  • 4
  • 5
NewPoints' Buy Forum Access module

Please note that this is pretty much a negative criticism post, rather than a balanced review as mentioned in this thread. Also be aware that stuff posted here may be highly subjective.
Please feel free to criticise this post, however.

Plugin Reviewed: Buy Forum Access
Plugin Version: 1.0 (last updated 25th August 2010)
Plugin Author: Pirata Nervo
Author Message
ZiNgA BuRgA Offline
Fag
*******
Posts: 3,357
Joined: Jan 2008
Post: #1
NewPoints' Buy Forum Access module
This is a NewPoints addon "review" requested by Pirata Nervo.
I generally only do MyBB plugins, but due to similarity and guessing, I can probably do this one too.  Note that I generally like to test my claims before making them, but I haven't done any testing whatsoever here, so apologies if I make any wrong assumptions.


I don't really see huge issues, but one thing I would mention is that there is a lot of unnecessary code duplication in this plugin.
Pretty much all of the following can be put into a generic checking function, where the other plugin hooks call

PHP Code:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
function newpoints_buyaccess_access_forum()
{
	global $mybb, $db, $lang;
	
	// in this hook, the fid hasn't been verified yet so we'll just check if it's in settings or not - in case it is in settings and it does not exist, it's the admin's fault.
	// Do not use forumdisplay_end because we'd be executing the whole forumdisplay.php file without needing to.
	$fid = (int)$mybb->input['fid'];
	
	$affected_fids = explode(',', $mybb->settings['newpoints_buyaccess_forums']);
	if (!in_array($fid, $affected_fids))
		return;
		
	// If we're a guest, we cant' browse this forum because we can't pay for it
	if (!$mybb->user['uid'])
		error_no_permission();
	
	newpoints_lang_load('newpoints_buyaccess');
	
	if ($mybb->settings['newpoints_buyaccess_time'] > 0)
	{
		$time_check = 'AND `date` > ('.TIME_NOW.' - '.$mybb->settings['newpoints_buyaccess_time'].')';
	}
	else 
		$time_check = '';
	
	// check if we've bought access to this forum
	$query = $db->simple_select('newpoints_buyaccess', '*', '`uid`=\''.$mybb->user['uid'].'\' AND `fid`=\''.$fid.'\' '.$time_check);
	$access = $db->fetch_array($query);
	if (!$access)
	{
		// we need to pay
		
		if ($mybb->request_method == "post" && $mybb->input['action'] == 'do_pay')
		{
			// insert data into database
			$insert_array = array(
				'fid' => intval($fid),
				'uid' => intval($mybb->user['uid']),
				'date' => TIME_NOW
			);
			$db->insert_query('newpoints_buyaccess', $insert_array);
			
			// get money from user
			newpoints_addpoints($mybb->user['uid'], -floatval($mybb->settings['newpoints_buyaccess_fee']));
			
			// log
			newpoints_log('buyaccess', $lang->sprintf($lang->newpoints_buyaccess_log, $fid, $mybb->settings['newpoints_buyaccess_fee']));
		}
		else {
			// Show page telling the user to pay
			
			// do we have enough points?
			if (floatval($mybb->user['newpoints']) < floatval($mybb->settings['newpoints_buyaccess_fee']))
			{
				error($lang->sprintf($lang->newpoints_buyaccess_no_points, newpoints_format_points(floatval($mybb->settings['newpoints_buyaccess_fee']))));
			}
			else {
				if (!$time_check)
					$time_check = $lang->newpoints_buyaccess_unlimited;
				else
				{
					$secondsaccess = $mybb->settings['newpoints_buyaccess_time'];
					$hoursaccess = floor($secondsaccess / 3600);
					$minsaccess = floor(($secondsaccess / 60) % 60);
					$secsaccess = floor($secondsaccess % 60);
					
					$time_check = $hoursaccess."h ".$minsaccess."m ".$secsaccess."s";
				}
			
				$page = $lang->sprintf($lang->newpoints_buyaccess_notice, newpoints_format_points(floatval($mybb->settings['newpoints_buyaccess_fee'])), $time_check)."<br /><form action=\"forumdisplay.php?fid=".$fid."\" method=\"post\">";
			
				$page .= "<input type=\"hidden\" name=\"action\" value=\"do_pay\">\n";
				
				// Make sure we escape these before outputting them
				$input = array (
					'datecut' => intval($mybb->input['datecut']),
					'order' => htmlspecialchars($mybb->input['order']),
					'sortby' => htmlspecialchars($mybb->input['sortby']),
					'page' => intval($mybb->input['page']),
				);
				
				foreach ($input as $name => $value)
				{
					if ($value == 0 || $value == '')
						continue;
					
					$page .= "<input type=\"hidden\" name=\"".$name."\" value=\"".$value."\">\n";
				}
			
				$page .= "<input type=\"submit\" value=\"".$lang->newpoints_buyaccess_confirm."\">\n";
				
				$page .= "</form>";
			
				error($page, $lang->newpoints_buyaccess_verify_payment);
			}
		}
	}
}



The plugin only blocks showthread, forumdisplay, sendthread, printthread and archive.  Which means it may be possible to leak information from, say, the newreply thread review.  Also other pages such as moderation pages aren't blocked - perhaps moderators are always allowed access, I don't know, but there doesn't appear to be any checking for moderators on the pages which are blocked.


Not a big issue, but a personal opinion:

PHP Code:
1
2
3
4
5
6
7
	$db->write_query("CREATE TABLE `".TABLE_PREFIX."newpoints_buyaccess` (
	  `pid` int(10) UNSIGNED NOT NULL auto_increment,
	  `fid` int(5) unsigned NOT NULL default '0',
	  `uid` bigint(30) unsigned NOT NULL default '0',
	  `date` bigint(30) unsigned NOT NULL default '0',
	  PRIMARY KEY  (`pid`)
		) ENGINE=MyISAM");

I'd personally ditch the pid column and make fid,uid the primary key.  IMO, fid,uid should be unique, and it is always searched that way, thus having an index on fid,uid makes sense.
In other words, I'd have changed the above to the following:

PHP Code:
1
2
3
4
5
6
	$db->write_query("CREATE TABLE `".TABLE_PREFIX."newpoints_buyaccess` (
	  `fid` int(5) unsigned NOT NULL default '0',
	  `uid` bigint(30) unsigned NOT NULL default '0',
	  `date` bigint(30) unsigned NOT NULL default '0',
	  PRIMARY KEY  (`fid`,`uid`)
		) ENGINE=MyISAM");

I guess some people might want to differ, but I'd suggest at least having a UNIQUE key on fid,uid.

Note: the plugin doesn't assume uniqueness on the column, so it is possible to have multiple entries for a single fid,uid if there is a time limit specified (ie user buys access into forum twice).  I don't really see much point in having more than one entry for a particular fid,uid - buying into a forum again should update the timestamp, rather than insert a new entry.  Also, perhaps the system should prune old entries every now and then.
This, of course, does depend on the design idea.  Pruning entries does mean that if the admin decides to remove the time limit later on, people who have bought access may have it pruned by the system.


My Blog
(This post was last modified: 08-26-2010 09:35 AM by ZiNgA BuRgA.)
08-26-2010 09:30 AM
Find all posts by this user Quote this message in a reply
Imran Offline
Member
***
Posts: 204
Joined: Apr 2010
Post: #2
RE: NewPoints' Buy Forum Access module
Nice ! Smile

being the first one to Thank You Wink

[Image: logo.png]

[Image: twitter.png]
08-26-2010 10:18 AM
Visit this user's website Find all posts by this user Quote this message in a reply
Pirata Nervo Offline
Member
***
Posts: 235
Joined: Jan 2008
Post: #3
RE: NewPoints' Buy Forum Access module
Thank you!
I'll take your suggestions into consideration and will release 1.1 as soon as possible Smile
08-26-2010 10:09 PM
Find all posts by this user Quote this message in a reply


Forum Jump: