[buddypress-trac] [BuddyPress] #5214: groups_create_group() improvements

[buddypress-trac]

#5214: groups_create_group() improvements
----------------------------------------+------------------------------
 Reporter:  r-a-y                       |       Owner:
     Type:  defect (bug)                |      Status:  new
 Priority:  normal                      |   Milestone:  Awaiting Review
Component:  Groups                      |     Version:  1.0
 Severity:  normal                      |  Resolution:
 Keywords:  has-patch needs-unit-tests  |
----------------------------------------+------------------------------

Comment (by boonebgorges):

 I guess I don't have a particular problem with these changes, but I think
 that we should be careful to attempt some sort of consistency across
 components. Generally speaking, we have three layers of functions for
 things like group creation:

 1. template controllers (eg `groups_action_create_group()`), which call...
 2. procedural functions (eg `groups_create_group()`), which call...
 3. database `save()` methods (eg `BP_Groups_Group::save()`)

 There are lots of things that need to happen in terms of
 validation/sanitization when creating a group, and each of them should
 happen only in one of the places. Off the top of my head:

 a. capabilities checks (should happen in (1), and for the most part we are
 good about this)
 b. getting submitted data out of `$_REQUEST` (1)
 c. sanitization that is directly related to `$_REQUEST` - stuff like
 `urldecode()` and `stripslashes()` (1)
 d. sanitization against SQL injection (3)
 e. sanitization against illegal markup/scripts (kses) (we generally do
 this in filters like `groups_group_name_before_save`, which happen in (3))
 f. providing default/fallback values - stuff like the current time and the
 loggedin user ID
 g. Checking to make sure the submitted data will create a valid group -
 this is stuff like `! empty( $name )`
 h. Checking to make sure that submitted data will properly be submitted
 into the table - stuff like making sure there is a value for `NOT NULL`
 columns in the db (should happen in (3))
 [probably more?]

 So, there are some things that we should be (and are) doing in (1). And
 there are some things that we should be (and are) doing in (3). The
 question for the remaining items is: should they happen in (2) or (3)?

 I think the best principle is something like this: Do everything as late
 as possible without sacrificing the flexibility of the later function. For
 example, we do cap checks in (1) because doing them in (2) would prevent
 automated group creation (where `! current_user_can( 'bp_moderate' )`). On
 the other hand, there's no reason why we should ever allow SQL injection,
 so we do that stuff in `$wpdb->prepare()` in (3).

 r-a-y - I think that your patch 5214.01.patch is actually pretty good on
 these counts. The items you've moved to (3) seem like they are the kinds
 of things that should never be skirted. So, like I said, the patch looks
 fine :) I just want to impose some sanity on the way we make these changes
 in the future, across components, because it's currently somewhat of a
 mess. (Check out the differences between bp-activity and bp-groups on this
 count.) Maybe we could have a document that sketches out our best
 practices with respect to this stuff.

--
Ticket URL: <https://buddypress.trac.wordpress.org/ticket/5214#comment:2>
BuddyPress <http://buddypress.org/>
BuddyPress