Closed
Bug 186093
Opened 21 years ago
Closed 19 years ago
Move CanSeeBug to User.pm and make User.pm usable by templates
Categories
(Bugzilla :: User Interface, enhancement, P2)
Tracking
()
RESOLVED
FIXED
Bugzilla 2.20
People
(Reporter: bugreport, Assigned: bugreport)
References
Details
Attachments
(1 file, 8 obsolete files)
16.20 KB,
patch
|
kiko
:
review+
|
Details | Diff | Splinter Review |
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)
Assignee | ||
Updated•21 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → Bugzilla 2.18
Comment 1•21 years ago
|
||
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.
Assignee | ||
Comment 2•21 years ago
|
||
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 %]
Assignee | ||
Updated•20 years ago
|
Target Milestone: Bugzilla 2.18 → Bugzilla 2.20
Assignee | ||
Comment 3•19 years ago
|
||
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.
Assignee | ||
Comment 4•19 years ago
|
||
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.
Assignee | ||
Updated•19 years ago
|
Attachment #109741 -
Attachment is obsolete: true
Assignee | ||
Comment 5•19 years ago
|
||
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•19 years ago
|
||
(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?
Assignee | ||
Comment 7•19 years ago
|
||
This needs a careful inspection and test, but it should be complete
Attachment #154619 -
Attachment is obsolete: true
Assignee | ||
Updated•19 years ago
|
Attachment #154837 -
Attachment is obsolete: true
Assignee | ||
Comment 8•19 years ago
|
||
Assignee | ||
Updated•19 years ago
|
Summary: Add functions to permit templates to determine security classifications of bugs → Move CanSeeBug to User.pm and make User.pm usable by templates
Assignee | ||
Updated•19 years ago
|
Attachment #154839 -
Flags: review?
Assignee | ||
Updated•19 years ago
|
Attachment #154839 -
Flags: review?
Assignee | ||
Comment 9•19 years ago
|
||
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
Assignee | ||
Updated•19 years ago
|
Attachment #154858 -
Flags: review?
Assignee | ||
Updated•19 years ago
|
Attachment #154858 -
Attachment is patch: false
Attachment #154858 -
Flags: review?
Assignee | ||
Comment 10•19 years ago
|
||
Assignee | ||
Updated•19 years ago
|
Attachment #154858 -
Attachment is obsolete: true
Assignee | ||
Updated•19 years ago
|
Attachment #154859 -
Flags: review?
Assignee | ||
Comment 11•19 years ago
|
||
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>
Assignee | ||
Comment 12•19 years ago
|
||
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?
Assignee | ||
Comment 13•19 years ago
|
||
Attachment #154859 -
Attachment is obsolete: true
Assignee | ||
Updated•19 years ago
|
Attachment #154866 -
Flags: review?(kiko)
Assignee | ||
Updated•19 years ago
|
Attachment #154866 -
Flags: review?
Comment 14•19 years ago
|
||
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•19 years ago
|
||
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•19 years ago
|
||
mine was on Bug.pm, so can_see_bug is overkill....
Assignee | ||
Updated•19 years ago
|
Attachment #154866 -
Flags: review?(kiko)
Attachment #154866 -
Flags: review?
Assignee | ||
Comment 17•19 years ago
|
||
Attachment #154866 -
Attachment is obsolete: true
Assignee | ||
Updated•19 years ago
|
Attachment #155079 -
Flags: review?(kiko)
Comment 18•19 years ago
|
||
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.
Assignee | ||
Updated•19 years ago
|
Attachment #155079 -
Attachment is obsolete: true
Attachment #155079 -
Flags: review?(kiko)
Assignee | ||
Comment 19•19 years ago
|
||
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.
Assignee | ||
Updated•19 years ago
|
Attachment #155086 -
Flags: review?(kiko)
Comment 20•19 years ago
|
||
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+
Assignee | ||
Updated•19 years ago
|
Flags: approval?
Updated•19 years ago
|
Flags: approval? → approval+
Assignee | ||
Comment 21•19 years ago
|
||
checked in
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
QA Contact: matty_is_a_geek → default-qa
You need to log in
before you can comment on or make changes to this bug.
Description
•