WebGUI::Group->new didn't check group validity

docs/changelog/7.x.x.txt

@@ -3,6 +3,7 @@
  - fix: SQLForm file upload broken
  - fix: Newsletter can contain duplicate threads
  - fix: Captcha unreadable when using Image::Magick
+ - fix: WebGUI::Group->new doesn't check group validity
 
 7.4.7
  - fix: misspelled i18n in webgui password recovery

lib/WebGUI/Asset/Post/Thread.pm

@@ -778,9 +778,11 @@ Negates the subscribe method.
 =cut
 
 sub unsubscribe {
-	my $self = shift;
-  my $group = WebGUI::Group->new($self->session,$self->get("subscriptionGroupId"));
-  $group->deleteUsers([$self->session->user->userId]);
+    my $self = shift;
+    my $group = WebGUI::Group->new($self->session,$self->get("subscriptionGroupId"));
+    return
+        if !$group;
+    $group->deleteUsers([$self->session->user->userId]);
 }
 
 

lib/WebGUI/Asset/Wobject/Collaboration.pm

@@ -1138,7 +1138,9 @@ sub processPropertiesFromFormPost {
 sub purge {
 	my $self = shift;
 	my $group = WebGUI::Group->new($self->session, $self->get("subscriptionGroupId"));
-	$group->delete;
+	if ($group) {
+        $group->delete;
+    }
 	if ($self->get("getMailCronId")) {
 		my $cron = WebGUI::Workflow::Cron->new($self->session, $self->get("getMailCronId"));
 		$cron->delete if defined $cron;
@@ -1262,8 +1264,16 @@ Subscribes a user to this collaboration system.
 
 sub subscribe {
 	my $self = shift;
-	my $group = WebGUI::Group->new($self->session,$self->get("subscriptionGroupId"));
-	$group->addUsers([$self->session->user->userId]);
+    my $group;
+    my $subscriptionGroup = $self->get('subscriptionGroupId');
+    if ($subscriptionGroup) {
+	    $group = WebGUI::Group->new($self->session,$subscriptionGroup);
+    }
+    if (!$group) {
+        $self->createSubscriptionGroup;
+	    $group = WebGUI::Group->new($self->session,$self->get('subscriptionGroupId'));
+    }
+    $group->addUsers([$self->session->user->userId]);
 }
 
 #-------------------------------------------------------------------
@@ -1277,7 +1287,9 @@ Unsubscribes a user from this collaboration system
 sub unsubscribe {
 	my $self = shift;
 	my $group = WebGUI::Group->new($self->session,$self->get("subscriptionGroupId"));
-	$group->deleteUsers([$self->session->user->userId],[$self->get("subscriptionGroupId")]);
+	return
+        unless $group;
+    $group->deleteUsers([$self->session->user->userId],[$self->get("subscriptionGroupId")]);
 }
 
 

lib/WebGUI/Group.pm

@@ -981,15 +981,18 @@ If you specified "new" for groupId, you can use this property to specify an id y
 =cut
 
 sub new {
-        my ($class, %group);
-        tie %group, 'Tie::CPHash';
-        $class = shift;
-	my $self = {};
-	$self->{_session} = shift;
-	$self->{_groupId} = shift;
-	my $override = shift;
-	my $cached = $self->{_session}->stow->get("groupObj");
+    my ($class, %group);
+    tie %group, 'Tie::CPHash';
+   	my $self = {};
+    
+    $class              = shift;
+    $self->{_session}   = shift;
+	$self->{_groupId}   = shift;
+	my $override        = shift;
+
+    my $cached = $self->{_session}->stow->get("groupObj");
 	return $cached->{$self->{_groupId}} if ($cached->{$self->{_groupId}});
+
 	bless $self, $class;
         if ($self->{_groupId} eq "new") {
 		$self->_create($override);
@@ -997,6 +1000,18 @@ sub new {
 	elsif ($self->{_groupId} eq "") {
 		$self->{_group} = $self->_defaults();
 	}
+    else {
+        # Check if the groupId is valid. If not return undef
+        my ($groupExists) = $self->{_session}->db->quickArray('select groupId from groups where groupId=?', [
+            $self->{_groupId},
+        ]);
+        unless ($groupExists) {
+            $self->{_session}->errorHandler->warn('WebGUI::Group->new called with a non-existant groupId:'
+                .'['.$self->{_groupId}.']');
+            return undef;
+        }
+    }
+
 	$cached->{$self->{_groupId}} = $self;
 	$self->{_session}->stow->set("groupObj", $cached);
 	return $self;

lib/WebGUI/Operation/Group.pm

@@ -82,6 +82,8 @@ sub canEditGroup {
     return 1 if canEditAll($session, $user);
 
 	my $group = WebGUI::Group->new($session,$groupId);
+    return
+        unless $group;
     return $user->isInGroup( $session->setting->get("groupIdAdminGroupAdmin") )  
         && $group->userIsAdmin( $user->userId )
         ;
@@ -464,7 +466,7 @@ sub www_editGroupSave {
     return $session->privilege->adminOnly
         unless canEditGroup($session, $gid);
     my $g = WebGUI::Group->new($session, $gid);
-    # We don't want them to use an existing name.  If needed, we'll ad a number to the name to keep it unique.
+    # We don't want them to use an existing name.  If needed, we'll add a number to the name to keep it unique.
     my $groupName = $session->form->process("groupName");
     while (my $existingGroupId = WebGUI::Group->find($session, $groupName)->getId) {
         last
@@ -499,50 +501,52 @@ sub www_editGroupSave {
 
 #-------------------------------------------------------------------
 sub www_editGrouping {
-	my $session = shift;
-	return $session->privilege->adminOnly() unless (canEditGroup($session,$session->form->process("gid")));
-	my $i18n = WebGUI::International->new($session);
-	my $f = WebGUI::HTMLForm->new($session);
-	$f->submit;
-        $f->hidden(
-		-name => "op",
-		-value => "editGroupingSave"
-	);
-        $f->hidden(
-		-name => "uid",
-		-value => $session->form->process("uid")
-	);
-        $f->hidden(
-		-name => "gid",
-		-value => $session->form->process("gid")
-	);
-	my $u = WebGUI::User->new($session,$session->form->process("uid"));
-	my $g = WebGUI::Group->new($session,$session->form->process("gid"));
-        $f->readOnly(
-		-value => $u->username,
-		-label => $i18n->get(50),
-		-hoverHelp => $i18n->get('50 description'),
-        );
-        $f->readOnly(
-		-value => $g->name,
-		-label => $i18n->get(84),
-		-hoverHelp => $i18n->get('84 description'),
-        );
-	my $group = WebGUI::Group->new($session,$session->form->process("gid"));
-	$f->date(
-		-name => "expireDate",
-		-label => $i18n->get(369),
-		-hoverHelp => $i18n->get('369 description'),
-		-value => $group->userGroupExpireDate($session->form->process("uid")),
-	);
-	$f->yesNo(
-		-name=>"groupAdmin",
-		-label=>$i18n->get(977),
-		-hoverHelp=>$i18n->get('977 description'),
-		-value=>$group->userIsAdmin($session->form->process("uid"))
-		);
-	$f->submit;
-        return _submenu($session,$f->print,'370');
+    my $session = shift;
+    my $uid = $session->form->process('uid');
+    my $gid = $session->form->process('gid');
+    return $session->privilege->adminOnly()
+        unless canEditGroup($session, $gid);
+    my $i18n = WebGUI::International->new($session);
+    my $f = WebGUI::HTMLForm->new($session);
+    $f->submit;
+    $f->hidden(
+        -name => "op",
+        -value => "editGroupingSave"
+    );
+    $f->hidden(
+        -name => "uid",
+        -value => $uid,
+    );
+    $f->hidden(
+        -name => "gid",
+        -value => $gid,
+    );
+    my $u = WebGUI::User->new($session,$uid);
+    my $g = WebGUI::Group->new($session,$gid);
+    $f->readOnly(
+        -value => $u->username,
+        -label => $i18n->get(50),
+        -hoverHelp => $i18n->get('50 description'),
+    );
+    $f->readOnly(
+        -value => $g && $g->name,
+        -label => $i18n->get(84),
+        -hoverHelp => $i18n->get('84 description'),
+    );
+    $f->date(
+        -name => "expireDate",
+        -label => $i18n->get(369),
+        -hoverHelp => $i18n->get('369 description'),
+        -value => $g && $g->userGroupExpireDate($uid),
+    );
+    $f->yesNo(
+        -name=>"groupAdmin",
+        -label=>$i18n->get(977),
+        -hoverHelp=>$i18n->get('977 description'),
+        -value=> $g && $g->userIsAdmin($uid),
+    );
+    $f->submit;
+    return _submenu($session,$f->print,'370');
 }
 
 #-------------------------------------------------------------------

lib/WebGUI/User.pm

@@ -297,6 +297,8 @@ sub isInGroup {
    return $isInGroup->{$uid}{$gid} if exists $isInGroup->{$uid}{$gid};
    ### Lookup the actual groupings.
    my $group = WebGUI::Group->new($self->session,$gid);
+   # Cope with non-existant groups. Default to the admin group if the groupId is invalid.
+   $group = WebGUI::Group->new($self->session, 3) unless $group;
    ### Check for groups of groups.
    my $users = $group->getAllUsers();
    foreach my $user (@{$users}) {