NewPoints' Buy Forum Access module

[ZiNgA BuRgA]

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.