Closed Bug 186093 Opened 19 years ago Closed 17 years ago

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

Categories

(Bugzilla :: User Interface, enhancement, P2)

2.17.1
All
Other
enhancement

Tracking

()

RESOLVED FIXED
Bugzilla 2.20

People

(Reporter: bugreport, Assigned: bugreport)

References

Details

Attachments

(1 file, 8 obsolete files)

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)
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → Bugzilla 2.18
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.
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 %]
Target Milestone: Bugzilla 2.18 → Bugzilla 2.20
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("pseudo_allcustomers@nowhere.com") %]
[%# pseudo_anycustomers has the maximum privileges posessed by any customers #%]
[% anycustomers = userfromlogin("pseudo_anycustomers@nowhere.com") %]

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.


Attached patch CanSeeBug for User.pm (obsolete) — Splinter Review
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.
Attachment #109741 - Attachment is obsolete: true
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.

(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?
Depends on: 253588
This needs a careful inspection and test, but it should be complete
Attachment #154619 - Attachment is obsolete: true
Attachment #154837 - Attachment is obsolete: true
Attached patch patch - added template plugin (obsolete) — Splinter Review
Summary: Add functions to permit templates to determine security classifications of bugs → Move CanSeeBug to User.pm and make User.pm usable by templates
Attachment #154839 - Flags: review?
Attachment #154839 - Flags: review?
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.
Attachment #154839 - Attachment is obsolete: true
Attachment #154858 - Flags: review?
Attachment #154858 - Attachment is patch: false
Attachment #154858 - Flags: review?
Attachment #154858 - Attachment is obsolete: true
Attachment #154859 - Flags: review?
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("foo@bar.net") %]
[% 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 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!
Attachment #154859 - Flags: review?
Attached patch v5 (obsolete) — Splinter Review
Attachment #154859 - Attachment is obsolete: true
Attachment #154866 - Flags: review?(kiko)
Attachment #154866 - Flags: review?
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
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.
Blocks: 251669
mine was on Bug.pm, so can_see_bug is overkill....
Attachment #154866 - Flags: review?(kiko)
Attachment #154866 - Flags: review?
Attached patch change naming convention (obsolete) — Splinter Review
Attachment #154866 - Attachment is obsolete: true
Attachment #155079 - Flags: review?(kiko)
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.
Attachment #155079 - Attachment is obsolete: true
Attachment #155079 - Flags: review?(kiko)
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.
Attachment #155086 - Flags: review?(kiko)
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!
Attachment #155086 - Flags: review?(kiko) → review+
Flags: approval?
Flags: approval? → approval+
checked in
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.