186093 - Move CanSeeBug to User.pm and make User.pm usable by templates

Status: RESOLVED FIXED

(Bugzilla :: User Interface, enhancement, P2)

Description

[Joel Peshkin]

In some site implementations, a UI template (most often the templates for
editing or creating bugs, but possibly even lists) may want to add visual cues
based on the level of security currently applied to a bug.  Bugzilla should
provide the underlying functionality needed to permit templates to do this.

An example of this is a system where a colored stripe is added to the page and
the color varies on the basis of who can see the bug.

For example, I may want to indicate....
GREEN - The bug's product can never be made accessible to customers
BLUE - The product permits this bug to be made accessable to a customer, but it
is not now
YELLOW - The product permits this bug to be made public, but it is not now
ORANGE - Bug is accessible by customers
RED - Bug is public

Naturally, this would be a site-customization, but the following functions need
to be callable from a template in order for it to be capable of doing this....

GetUserIdByLogin($login) - should return a numeric userid for the specified
login. Because this may be called repeatedly by a template, it should keep its
findings in a hash to avoid repeating the lookup if called again.

CanUserViewProduct($product, $userid)

CanUserViewBug($bug, $userid)

Comment 1

[Bradley Baetz (:bbaetz)]

The first one is a Bugzilla::User template plugin
The second is user->canSeeProduct
The thrd is user->CanSeeBug($bug), or $bug->UserCanSee($user)

None of those actually exist yet, mind you, and the first depends on my other
tempate bug, to set up the infrastructure.

Comment 2

[Joel Peshkin]

Attachment: Proof-of-concept Patch ... not ready for prime-time

This proof-of-concept looks up 3 things in
template/en/default/bug/show.html.tmpl
and blurts them out. Specifically, if the bug is currently public, if it could
be made public, and if a user named "pseuso_plainuser" can currently see this
bug.  (pseudo_plainuser can be a disabled account)

This required the following infrastracture changes to make this doable from a
template....

1) Bug.pm needs to include product_id on its list of OK fields.
2) Added a CanSelectProduct(product_id, userid) to globals.pl
3) Cleaned up DBname_to_id to cache its results and call ConfirmGroup. This is 

   something we should do anyway.
4) Passed CanSeeBug, CanSelectProduct, and DBname_to_id to templates.

This enabled the silly code in template/en/default/bug/show.html.tmpl to
perform 3 formatting decisions on the basis of 3 differrent checks using...

[% plainuser = DBname_to_id("pseudo_plainuser") %]
[% isnowpublic = CanSeeBug(bug.bug_id,0) %]
[% plainusercansee = CanSeeBug(bug.bug_id,plainuser) %]
[% couldbepublic = CanSelectProduct(bug.product_id,0) %]
[% IF isnowpublic %]
  ISNOWPUBLIC
[% ELSE %]
  NOT PUBLIC
[% END %]
<hr>
[% IF plainusercansee %]
  PLAIN USER CAN SEE
[% ELSE %]
  PLAIN USER CAN NOT SEE
[% END %]
<hr>
[% IF couldbepublic %]
  COULD BE PUBLIC
[% ELSE %]
  COULD NOT BE PUBLIC
[% END %]

Comment 3

[Joel Peshkin]

Having let some time elapse and let User.pm mature, I think the right way to do
this is as follows....

Pass an additional fucntion to the templates to let the template code create a
new user object from a login_name. (userfromlogin)

Within the User object, add a function (CanSeeBug) that tests the specified bug
id against the user object's permissions and returns true if the user can see
the bug and false if the user cannot see the bug.  

Note: This CanSeeBug cannot replace the one in globals unless we start to create
user objects even for logged out users.  We may want to add the capbility to
initialize a user object for logged-out users. It caches its already-prepared
statement handle so that it can use a placeholder for fast execution when
evaluating templates that call this function once for each bug listed.

Within a template, it is then possible to create user objects for hypothetical
users to establish the level of security for each bug.

At the beginning of a buglist, we would include...

[%# pseudo_allcustomers has the minimum privileges shared by all customers #%]
[% allcustomers = userfromlogin("[e-mail]") %]
[%# pseudo_anycustomers has the maximum privileges posessed by any customers #%]
[% anycustomers = userfromlogin("[e-mail]") %]

As we move through the list...

[% IF allcustomers.CanSeeBug( bug.id ) %]
<img src="public.png">
[% ELSIF anycustomers.CanSeeBug( bug.id ) %]
<img src="shared.png">
[% ELSE %]
<img src="private.png">
[% END %]

So, the process of adding visual cues becomes a reasonably efficient template
programming task.

Comment 4

[Joel Peshkin]

Attachment: CanSeeBug for User.pm

This only implements the CanSeeBug functionality that bbaetz mentioned in the
earlier comment.  The CanSelectProduct() function is simpler.

Ideally, this should replace the global, but we need to have a way to create a
user object for a logged-out user.

Comment 5

[Joel Peshkin]

Here's the missing part...

A new entry point for User.pm new_anonymous should be added to return a working
user object for a logged out user.

Comment 6

[Erik Stambaugh]

(In reply to comment #5)
> Here's the missing part...
> 
> A new entry point for User.pm new_anonymous should be added to return a working
> user object for a logged out user.

Are you sure Bugzilla::User->new_from_login or Bugzilla::User->match won't do
the trick?

Comment 7

[Joel Peshkin]

Attachment: CanSeeBug moved and user available from templates

This needs a careful inspection and test, but it should be complete

Comment 8

[Joel Peshkin]

Attachment: patch - added template plugin

Comment 9

[Joel Peshkin]

Attachment: Patch - also provides GetSelectableProducts in User.pm

It will take a while to phase out the GetSelectable* stuff from globals.pl, but
this makes it available to templates and enables the beginning of that process.

Comment 10

[Joel Peshkin]

Attachment: Patch - remember added file this time

Comment 11

[Joel Peshkin]

For anyone testing this out, here is a snippet of template code that
demonstrates this.  Templates can also call u.CanSeeBug($bugid)

[% USE User %]
[% u = User.new_from_login("[e-mail]") %]
[% IF u %]
  [% u.name FILTER html %]
  <br>
  [% FOREACH g = u.GetSelectableProducts() %]
    [% g FILTER html %] ,
  [% END %]
  <br>
  [% m = u.GetSelectableProducts(1) %]
  [% FOREACH g = m.keys %]
    [% g FILTER html %] [% "=>" FILTER html %] [% m.$g FILTER html %],
  [% END %]
[% ELSE %]
  nope
[% END %]
<br>

Comment 12

[Joel Peshkin]

Comment on attachment 154859 [details] [diff] [review]
Patch - remember added file this time

and messed up the distinction between a function call and a hash. bah!

Comment 13

[Joel Peshkin]

Attachment: v5

Comment 14

[Bradley Baetz (:bbaetz)]

Hmm. I had this in Bug.pm:

sub can_see {
    my ($self, $user) = @_;
    # Order this to avoid unneeded queries where possible, but optimise to the
common case
    # where the user can see the bug
                                                                               
                                   
    # If the bug isn't in any groups, then its OK to see.
    my @ongroups = grep { $_->{ison} } @{$self->groups};
    return 1 unless scalar (@ongroups);
                                                                               
                                   
    # User can see the bug if they're logged in and:
    if ($user) {
        return 1 if
            # They're the assignee, or
            ($self->{assigned_to}->id == $user->id) ||
            # They're the qa contact, or
            (Param('useqacontact') && $self->{qa_contact}->id == $user->id) ||
            # They're the reporter, and the reporter can see this bug, or
            ($self->{reporter_accessible} && $self->{reporter}->id == $user->id) ||
            # They're on the cc list, and the cclist can see this bug
            ($self->{cclist_accessible} && grep $user->login, @{$self->cc});
        # If they're in all the groups, then its OK too
        foreach my $group (@ongroups) {
            return 0 unless $user->in_group($group);
        }
        return 1;
    }
                                                                               
                                   
    return 0;
}

Of course, I can no longer remember whether that worked. It does rely on some
other changes I made to User.pm

Comment 15

[Christian Reis]

I hate to nickpit here, but object/class methods are all-lowercase in User.pm.
There's not one MixedCase method there -- and actually, that's a good way of
telling whether something's a global or not. Couldn't we do the same here? I
love in_group() ... how about can_see() or can_see_bug() if you want to be
specific enough.

Comment 16

[Bradley Baetz (:bbaetz)]

mine was on Bug.pm, so can_see_bug is overkill....

Comment 17

[Joel Peshkin]

Attachment: change naming convention

Comment 18

[Christian Reis]

Comment on attachment 155079 [details] [diff] [review]
change naming convention

>Index: Bugzilla/Bug.pm
>-  $self->{'whoid'} = $user_id;
>+  $self->{'who'} = new Bugzilla::User($user_id);

That's funny. Why were we storing whoid in self if we never used it?

>Index: Bugzilla/User.pm
>+    $self->{sthCanSeeBug} = $sth;

Maybe rename this too, or is this parallel naming thing intentional? (could be,
we have SelectableProducts below?)

>+    return ( (($reporter == $userid) && $reporter_access)
>+           || (($qacontact == $userid) && $userid)

Should we be checking for the useqacontact param here? AFAIK the column isn't
dropped when you turn it off, so the values will still be there...

>+           || (!$missinggroup) );

I have to confess I don't actually understand what you're aiming for with
missinggroup here. I'd really appreciate a comment detailing this bit which
appears non-obvious and a change from CanSeeBug.

Comment 19

[Joel Peshkin]

Attachment: v7 - commented and added param check

For the variables cached within the object, I would rather not have the names
be identical to the method calls.  I have seen a lot of confusion result from
that.  No sense in adding more.

Comment 20

[Christian Reis]

Comment on attachment 155086 [details] [diff] [review]
v7 - commented and added param check

Wish interdiff worked. Looks good, and a nice cleanup. Ping glob to update his
userselect patch when this goes in. Thanks!

Comment 21

[Joel Peshkin]

checked in