[ZBX-7338] incorrect span validation in screen import Created: 2013 Nov 12 Updated: 2017 May 30 Resolved: 2013 Dec 06 |
|
Status: | Closed |
Project: | ZABBIX BUGS AND ISSUES |
Component/s: | API (A) |
Affects Version/s: | 2.2.0 |
Fix Version/s: | 2.2.2rc1, 2.3.0 |
Type: | Incident report | Priority: | Blocker |
Reporter: | richlv | Assignee: | Unassigned |
Resolution: | Fixed | Votes: | 1 |
Labels: | xml | ||
Remaining Estimate: | Not Specified | ||
Time Spent: | Not Specified | ||
Original Estimate: | Not Specified |
Issue Links: |
|
Description |
have a fresh 2.2.0 install, export zabbix server template, try to import it : Row span of screen element located at X - 0 and Y - 2 is too big. edit : actually, i might have had an older version of this template present, which would mean that it might have already had that screen with dimensions of 2x2 maybe we validate xml screen elements against screen data in the db, not xml itself. or something |
Comments |
Comment by richlv [ 2013 Nov 12 ] |
in CScreenItem.php validateCreate() the following seems to happen, although i might have gotten it all wrong... a) "$screenIds = zbx_objectValues($screenItems, 'screenid');" in my case returns only one id - 4. maybe unrelated things that surprise me... 1) why does creating a new screen go through the update methods/functions ? edit : there might have been an existing template with same name in another group, thus update and db access might be justified. actual problem might be inability to change screen dimensions when updating it during the import |
Comment by Pavels Jelisejevs (Inactive) [ 2013 Nov 13 ] |
Looks like a duplicate of |
Comment by richlv [ 2013 Nov 14 ] |
note that span can also be set to 0 in the xml, and then the screen imports properly... not sure whether we want to handle that now |
Comment by Eduards Samersovs (Inactive) [ 2013 Nov 15 ] |
Resolved
Fixed in development branch svn://svn.zabbix.com/branches/dev/ZBX-7338 |
Comment by Pavels Jelisejevs (Inactive) [ 2013 Nov 20 ] |
(1) Minor correction in r40373. Eduards OK, CLOSED |
Comment by Pavels Jelisejevs (Inactive) [ 2013 Nov 20 ] |
(2) In CScreen::create():
Eduards RESOLVED r.40479 jelisejev I've made a minor code improvement in r40785. Since DB::insert() preserves the keys in the ID array, we don't need a separate counter. Please review. Eduards OK, CLOSED |
Comment by Pavels Jelisejevs (Inactive) [ 2013 Nov 20 ] |
(3) In CScreen::replaceItems() most of the code could be replaced with DB::replace(). Eduards As discussed, we need validate input before write to db, so DB::replace() is not proper solution. jelisejev CLOSED. |
Comment by Pavels Jelisejevs (Inactive) [ 2013 Nov 20 ] |
(4) Deleting screen items which become outside of the screen must also be implemented for template screens. I suggest to implement it in a separate method to avoid cope duplicates. Eduards RESOLVED r.40496 jelisejev CLOSED. |
Comment by Pavels Jelisejevs (Inactive) [ 2013 Nov 20 ] |
(5) Screen item resourceid parameter should not be required. There's a mistake in the API docs. Eduards RESOLVED r.40522 jelisejev CLOSED. |
Comment by Pavels Jelisejevs (Inactive) [ 2013 Nov 20 ] |
(6) Some problems with checking whether the cell is already taken:
{ "jsonrpc": "2.0", "method": "screenitem.create", "params": { "screenid": 20, "resourcetype": 7, "resourceid": 0 }, "id": 9, "auth": "c4674d432c610d4fed9d5c13c5f859d8" }
Eduards RESOLVED r.40479 jelisejev CLOSED. |
Comment by Pavels Jelisejevs (Inactive) [ 2013 Nov 20 ] |
(7) The following request is executed even when the row and column spans are to big: { "jsonrpc": "2.0", "method": "screenitem.create", "params": { "screenid": 20, "resourcetype": 7, "resourceid": 0, "rowspan": 1000, "colspan": 1000 }, "id": 2, "auth": "2c5a20d45c2a808a5a9581bc26f96ca9" } There are a lot of bugs caused by omitting certain parameters. To avoid them you mustn't use isset the check* methods, you always need to make sure that all of the required parameters are present: either from the database, or from default values. Eduards RESOLVED r.40479 jelisejev CLOSED. |
Comment by Pavels Jelisejevs (Inactive) [ 2013 Nov 20 ] |
(8) When validating the coordinates of the screen item, the error message should reference a specific screen item and screen. Something like: The Y coordinate of screen element located at at X - %x% and Y - %y% of screen "%screenName%" is too big. Similarly for X. Eduards RESOLVED r.40479 jelisejev CLOSED. |
Comment by Pavels Jelisejevs (Inactive) [ 2013 Nov 22 ] |
(9) Trying to save an screen element without a graph triggers an "Incorrect graph ID "0" provided for screen element.", when it should say "No graph ID provided for screen element." or, even better, "Field "Graph name" is mandatory.". Same for map and screen elements. Eduards RESOLVED r.40520 jelisejev Some corrections for CScreenItem::checkInput():
Eduards RESOLVED r.40765 jelisejev After you use extendFromObjects() on $screenItems, screen item resourceid cannot be null, so the check in line 430 is not required. Eduards This check is require in create, as discussed jelisejev CLOSED. |
Comment by Pavels Jelisejevs (Inactive) [ 2013 Nov 22 ] |
(10) It's possible to create a plain text and simple graph element without an item. Eduards RESOLVED r.40520 jelisejev CLOSED. |
Comment by Pavels Jelisejevs (Inactive) [ 2013 Nov 22 ] |
(11) Using the API it's possible to create a trigger overview element with a host group that I have no permissions to. Eduards RESOLVED r.40520 jelisejev This problem is not resolved. Eduards RESOLVED r.40765 jelisejev CLOSED. |
Comment by Pavels Jelisejevs (Inactive) [ 2013 Nov 22 ] |
(12) Some CScreenItem::checkInput() coding style issues
Eduards RESOLVED r.40479 jelisejev CLOSED. |
Comment by Pavels Jelisejevs (Inactive) [ 2013 Nov 22 ] |
(13) screenitem.update doesn't validate resourceid if resourcetype is not given: { "jsonrpc": "2.0", "method": "screenitem.update", "params": { "screenitemid": 84, "resourceid": "3333333" }, "id": 4, "auth": "e9d858b5b7fd64e128926a59d8e40e9c" } Similar problem as (7). Eduards RESOLVED r.40520 jelisejev Now this request triggers a different error: "The X coordinate of screen element located at X - 0 and Y - 0 of screen \"\" is too big." Eduards RESOLVED r.40765 jelisejev CLOSED. |
Comment by Pavels Jelisejevs (Inactive) [ 2013 Nov 22 ] |
(14) screenitem.updatebyposition:
Eduards RESOLVED r.40522 jelisejev The $found variable is now unused and must be removed. Eduards RESOLVED r.40765 jelisejev CLOSED. |
Comment by Pavels Jelisejevs (Inactive) [ 2013 Nov 22 ] |
(15) According to the new guidelines, we don't assign variables in if statements (graphs.inc.php:288) if ($dbGraph = DBfetch(DBselect('SELECT g.* FROM graphs g WHERE g.graphid='.zbx_dbstr($graphId)))) { return $dbGraph; } Eduards RESOLVED r.40479 jelisejev CLOSED. |
Comment by Pavels Jelisejevs (Inactive) [ 2013 Nov 22 ] |
(16) In CScreen::update() please use API::getApi()->select instead of screenitem.get. The same for CScreenItem::validateCreate() and validateUpdate() (permission checks won't work there, it's a separate issue). Eduards RESOLVED r.40522 jelisejev This issue is not resolved. Eduards RESOLVED r.40765 jelisejev CLOSED. |
Comment by Pavels Jelisejevs (Inactive) [ 2013 Dec 04 ] |
(17) Using the API it's possible to change a screen item from a graph to a map with the following request: { "jsonrpc": "2.0", "method": "screenitem.update", "params": { "screenitemid": 13, "resourcetype": 2 }, "id": 24, "auth": "0f06ca623de2fbc956a104c5f40b77ba" } Eduards RESOLVED r.40765 jelisejev CLOSED. |
Comment by Pavels Jelisejevs (Inactive) [ 2013 Dec 06 ] |
(18) Please remove the "with_templated" option the way we discussed. Eduards RESOLVED r.40788 jelisejev CLOSED. |
Comment by Pavels Jelisejevs (Inactive) [ 2013 Dec 09 ] |
TESTED. |
Comment by Eduards Samersovs (Inactive) [ 2013 Dec 09 ] |
Fixed in versions 2.3.0 (trunk) r.40830, 2.2.2rc1 r.40827 |