[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:
Duplicate
is duplicated by ZBX-2224 incorrect screen xml imported Closed
is duplicated by ZBX-6367 screen items attempted to be updated ... Closed
is duplicated by ZBX-6368 rowspan & colspan should not be manda... Closed

 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.
b) then "$dbScreenItems = API::getApi()->select('screens_items',..." for some reason returns items for a whole bunch of ids.
c) then "$screens = API::getApi()->select('screens', array(..." for some reason returns vsize as 2, although it was 3 up to this moment.

maybe unrelated things that surprise me...

1) why does creating a new screen go through the update methods/functions ?
2) when validating screen, why do we insert some data in the db, then select it only to validate ? is that sane ?

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 ZBX-6367.

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

  • validation before screen item creation
  • deleting screen items which become outside of the screen
  • changing screen item coordinates don't change screen items ids (it was absolutely crazy)
  • screen item span validate become the same as for create and update
  • rowspan and colspan parameters become optional

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():

  • Screen should not be created one-by-one, but in bulk;
  • There's no need to use replaceItems() since the new screens don't have any items.

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:

  • The following request successfully creates a new item, although the cell is already taken.
{
    "jsonrpc": "2.0",
    "method": "screenitem.create",
    "params": {
        "screenid": 20,
        "resourcetype": 7,
        "resourceid": 0
    },
    "id": 9,
    "auth": "c4674d432c610d4fed9d5c13c5f859d8"
}
  • CScreenItem::checkDuplicateResourceInCell() should not be called for each screen item, but for all screen itms at once;
  • Lets change the error message to "Screen %screenName% cell X - %x% Y - %y% is already taken." It's more precise.

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():

  • When loading resourcetype for an updated screen element, it's easier to use CZBXAPI::extendFromObjects() instead of
    			if (isset($screenItem['resourcetype'])) {
    				$resourceType = $screenItem['resourcetype'];
    			}
    			else {
    				$resourceType = $update ? $dbScreenItems[$screenItem['screenitemid']]['resourcetype'] : null;
    			}
    
  • $resourceType cannot be null on line 431
  • Condition "if (!(!isset($screenItem['resourceid']) && $update)) {" seems incorrect. It will never be executed if resourceid is set.
  • don't use empty()
  • when saving a host time clock item without an item, the message says "No host ID provided for screen element." instead of "No item ID ..."
  • In line 508 using zbx_empty() was correct, since 0 is a valid URL.

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

Generated at Thu Mar 28 22:19:10 EET 2024 using Jira 9.12.4#9120004-sha1:625303b708afdb767e17cb2838290c41888e9ff0.