412710 - add ability to edit existing tree

Status: RESOLVED WONTFIX

(Webtools Graveyard :: Tinderbox, defect)

Description

[Robert Helmer [:rhelmer]]

We can add a tree via admintree.cgi, but there is no way (as far as I can tell) to edit settings an existing tree, without modifying the files directly.

For tinderbox.mozilla.org, if anyone with sheriff access messes up tree creation then we need to escalate to IT, and I'd rather spare them :)

Comment 1

[cls]

I think that we would want a dependency upon bug 322418 as you don't want just anyone tweaking the tree settings.  On the other hand, we do allow anyone to create a tree.  If you don't care about anyone being able to change settings, it may be easiest to just add a 'delete tree' option for this case. :-)

Comment 2

[timeless]

confused.

http://tinderbox.mozilla.org/showbuilds.cgi has an entire section devoted to editing trees, it includes links like this:

http://tinderbox.mozilla.org/admintree.cgi?tree=Mozilla-l10n-sv-SE

Comment 3

[cls]

The current admintree page only lets you edit the tree display and trim logs.  There's no way to edit the tree configuration (ie, the contents of treedata.pl) except manually.

Comment 4

[Reed Loden [:reed]]

Attachment: patch - v1

Things this patch does:
* Adds an "Edit a tinderbox's configuration" section to admintree.cgi.
* Modifies create_tree() in doadmin.cgi to support both creating and modifying tree configs.
* Fixes regular expression to pass Perl warnings being thrown during testing.
* Changes some whitespace.

Remaining issue to think about:
* How best to deal with XSS attempts via tree config variables. We don't do any input checking or output encoding, but that's not really new to anything on admintree.cgi except the tree name, so I'm not sure I really care that much, considering you need the password to change the variables anyway. Thoughts?

It's live on http://tinderbox-stage.mozilla.org/ if you want to play.

Comment 5

[cls]

Comment on attachment 320657 [details] [diff] [review]
patch - v1

The code looks fine but I don't like that the internal setup of the server is available for just anyone to see, much less have a chance of tweaking.  That information should only be available to system level admins.  It should be hidden behind another access mechanism, like how params are/were hidden in bugzilla.

Comment 6

[Mike Taylor [:bear]]

Comment on attachment 320657 [details] [diff] [review]
patch - v1

looks simple enough but it just hangs all the internal variables out for viewing.  +1 to cls' suggestion

Comment 7

[Reed Loden [:reed]]

(In reply to comment #6)
> (From update of attachment 320657 [details] [diff] [review])
> looks simple enough but it just hangs all the internal variables out for
> viewing.  +1 to cls' suggestion

True, but people can go to Bonsai and see the internal variables themselves... :)

However, would fixing bug 398050 suffice for you and cls?

Comment 8

[Reed Loden [:reed]]

Attachment: patch - v2

Now that bug 398050 has landed, this was much easier. Does this work for you all?

Comment 9

[Reed Loden [:reed]]

Checking in admintree.cgi;
/cvsroot/mozilla/webtools/tinderbox/admintree.cgi,v  <--  admintree.cgi
new revision: 1.39; previous revision: 1.38
done
Checking in doadmin.cgi;
/cvsroot/mozilla/webtools/tinderbox/doadmin.cgi,v  <--  doadmin.cgi
new revision: 1.37; previous revision: 1.36
done

Comment 10

[cls]

Comment on attachment 322886 [details] [diff] [review]
patch - v2

Why are you alternating between using '$treedata->{field}' and '$::global_treedata->{tree}->{field}' ?

Comment 11

[Reed Loden [:reed]]

(In reply to comment #10)
> (From update of attachment 322886 [details] [diff] [review])
> Why are you alternating between using '$treedata->{field}' and
> '$::global_treedata->{tree}->{field}' ?  

$treedata doesn't contain everything I need (see tb_load_data() in tbglobals.pl). I'm more than happy to add what I need to tb_load_data() and just use $treedata. That sound good to you?

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Tinderbox isn't maintained anymore. Closing.