Bug 33344 - wrong referenced policy is displayed
wrong referenced policy is displayed
Status: CLOSED FIXED
Product: UCS
Classification: Unclassified
Component: UMC - Policies
UCS 4.1
Other Linux
: P5 normal (vote)
: UCS 4.1-1-errata
Assigned To: Jürn Brodersen
Florian Best
:
: 33367 34681 (view as bug list)
Depends on:
Blocks: 35315 38674
  Show dependency treegraph
 
Reported: 2013-11-13 09:47 CET by Florian Best
Modified: 2016-05-04 18:10 CEST (History)
7 users (show)

See Also:
What kind of report is it?: ---
What type of bug is this?: ---
Who will be affected by this bug?: ---
How will those affected feel about the bug?: ---
User Pain:
Enterprise Customer affected?:
School Customer affected?:
ISV affected?:
Waiting Support:
Flags outvoted (downgraded) after PO Review:
Ticket number:
Bug group (optional): External feedback, Usability
Max CVSS v3 score:


Attachments
Proposed patch (32.43 KB, patch)
2016-04-06 17:23 CEST, Jürn Brodersen
Details | Diff
proposed patch 2 (6.25 KB, patch)
2016-04-08 16:20 CEST, Jürn Brodersen
Details | Diff
patch for po files (27.60 KB, patch)
2016-04-08 16:22 CEST, Jürn Brodersen
Details | Diff
proposed patch 3 (use of udm/get) (6.47 KB, patch)
2016-04-20 17:53 CEST, Jürn Brodersen
Details | Diff
patch (6.80 KB, patch)
2016-04-21 08:46 CEST, Florian Best
Details | Diff
define widget in the backend (6.33 KB, patch)
2016-04-26 11:33 CEST, Jürn Brodersen
Details | Diff
Proposed patch (6.64 KB, patch)
2016-04-26 15:32 CEST, Jürn Brodersen
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Florian Best univentionstaff 2013-11-13 09:47:07 CET
When opening more than one policy the referenced policy is always displayed as the same.

To reproduce:
* create 2 UCR policies
* reference them to different objects (e.g. to LDAP-base and cn=computers)
* open both policies one after the other
* on the "Referencing objects" tab the referenced object from the first opened policy is displayed
Comment 1 Florian Best univentionstaff 2013-11-13 10:45:04 CET
It seems that this is a error in the backend. The information is stored as static class member on the LDAP_Search syntax.
Comment 2 Florian Best univentionstaff 2013-11-13 10:52:51 CET
It seems also that the wrong polcies are also displayed in the policy tab of any udm object.
Comment 3 Alexander Kläser univentionstaff 2013-11-14 10:38:29 CET
*** Bug 33367 has been marked as a duplicate of this bug. ***
Comment 4 Dirk Wiesenthal univentionstaff 2014-04-29 12:21:27 CEST
*** Bug 34681 has been marked as a duplicate of this bug. ***
Comment 5 Dirk Wiesenthal univentionstaff 2014-04-29 12:23:25 CEST
As noted in Bug #34681: The frontend cache is most likely the cause of this bug. It caches the properties for each module. Among them the LDAP_Search syntax, which now has its ldap_dn "hardcoded" for all instances of this UDM module
Comment 6 Alexander Kläser univentionstaff 2014-04-29 12:55:27 CEST
*** Bug 33367 has been marked as a duplicate of this bug. ***
Comment 7 Jan Christoph Ebersbach univentionstaff 2014-06-23 09:48:23 CEST
I stumble upon this issue quite often in projects where I want to review and modify policies.  Reviewing and debugging policies becomes extremely hard through this issue.
Comment 8 Florian Best univentionstaff 2015-05-29 16:39:45 CEST
Also: open a policy, change object to use the policy, open policy again → reference is not shown (due to frontend cache).
Comment 9 Alexander Kläser univentionstaff 2015-07-29 18:09:55 CEST
(In reply to Dirk Wiesenthal from comment #5)
> As noted in Bug #34681: The frontend cache is most likely the cause of this
> bug. It caches the properties for each module. Among them the LDAP_Search
> syntax, which now has its ldap_dn "hardcoded" for all instances of this UDM
> module

Yes, it seems to be the frontend cache.

I would really like to see the referenced objects being send along with a udm/get request of the actual policy. The current implementation via pseudo syntax leads to awkward workarounds both on the frontend or the backend side. A module's list of properties and a module's layout should be static values that can be easily cached. In current implementations, the DN always needs to be send for a list of module properties.

Are there any problems that such a change could lead to?
Comment 10 Michael Grandjean univentionstaff 2016-01-04 12:39:25 CET
> Reviewing and debugging policies becomes extremely hard through this issue.

+1

Additionally, it's very confusing for customers who try to solve policy issues in their own and don't know about this bug.

Setting Version to 4.1, because it's still broken there.
Comment 11 Michael Grandjean univentionstaff 2016-01-22 21:06:16 CET
Noticed by a customer, mentioned on summit.
Comment 12 Jürn Brodersen univentionstaff 2016-04-06 17:23:35 CEST
Created attachment 7582 [details]
Proposed patch
Comment 13 Florian Best univentionstaff 2016-04-06 21:32:06 CEST
(In reply to Jürn Brodersen from comment #12)
> Created attachment 7582 [details]
> Proposed patch
Can you explain a little bit why you moved logic from the backend into the frontend?
At least the ldap-filter is broken in some cases as the value of the policy DN is not escaped properly.
I'll have a closer look tomorrow.
Comment 14 Alexander Kläser univentionstaff 2016-04-07 11:00:07 CEST
(In reply to Florian Best from comment #13)
> (In reply to Jürn Brodersen from comment #12)
> > Created attachment 7582 [details]
> > Proposed patch
> Can you explain a little bit why you moved logic from the backend into the
> frontend?

See comment #9:
> [...] The current implementation via pseudo
> syntax leads to awkward workarounds both on the frontend or the backend
> side. A module's list of properties and a module's layout should be static
> values that can be easily cached. In current implementations, the DN always
> needs to be send for a list of module properties.

Jürn actually just moved the logic from the backend to the frontend. In this way the policy property/layout information can still be cached, yet the search for referenced objects of the specific policy can be triggered for each policy separately.

> At least the ldap-filter is broken in some cases as the value of the policy
> DN is not escaped properly.
> I'll have a closer look tomorrow.

In fact, the current fix does not introduce any regression as the filter had not been escaped before. However, I agree that it would make sense to include an additional UMC command (e.g., "udm/policy/referenced_objects") which expects an DN and returns all referenced objects. The escaping could then be done easily in the backend.
Comment 15 Jürn Brodersen univentionstaff 2016-04-08 16:20:58 CEST
Created attachment 7587 [details]
proposed patch 2

I added a new umc command that returns all objects that are referenced by a given policy and use that command instead of building the LDAP filter in js.
For escaping the input "LDAPSearchSanitizer" is used.
Comment 16 Jürn Brodersen univentionstaff 2016-04-08 16:22:05 CEST
Created attachment 7588 [details]
patch for po files
Comment 17 Florian Best univentionstaff 2016-04-11 07:23:59 CEST
(In reply to Jürn Brodersen from comment #15)
> Created attachment 7587 [details]
> proposed patch 2
> 
> I added a new umc command that returns all objects that are referenced by a
> given policy and use that command instead of building the LDAP filter in js.
> For escaping the input "LDAPSearchSanitizer" is used.
This patch looks very nice!
I have an idea for improvement:
Wouldn't it be better to add these values as key "$references$" into the udm/get call? Then the object representation would contain these values directly and one wouldn't need an extra UMC-call. Also it would be a little bit more generic in the naming as it is not necessarily bound to policies. Maybe one day we want to display also the network of a computer object in that way, etc.
Comment 18 Jürn Brodersen univentionstaff 2016-04-20 17:53:11 CEST
Created attachment 7606 [details]
proposed patch 3 (use of udm/get)

This patch uses the existing udm/get command and adds a $references$ key to it. As discussed with Florian (comment 17)

Also the reverencing objects tab is not shown for new policies anymore.
Comment 19 Florian Best univentionstaff 2016-04-21 08:46:25 CEST
Created attachment 7608 [details]
patch

(In reply to Jürn Brodersen from comment #18)
> Created attachment 7606 [details]
> proposed patch 3 (use of udm/get)
> 
> This patch uses the existing udm/get command and adds a $references$ key to
> it. As discussed with Florian (comment 17)
> 
> Also the reverencing objects tab is not shown for new policies anymore.
Yes, that looks nicer. Can you have a look at my attached patch? It does it a more extensible manner. But there is a javascript exception when I try to this._addSubTab() it.
Comment 20 Jürn Brodersen univentionstaff 2016-04-26 11:33:22 CEST
Created attachment 7619 [details]
define widget in the backend

Adding the referencing objects page from the frontend was problematic because it was not easily possible to decide early enough if the tab should be shown or not.

As discussed I put the logic back in to the backend and added a widget (based on the LinkList widget) to the umc package for showing referencing objects.
Comment 21 Florian Best univentionstaff 2016-04-26 13:18:57 CEST
(In reply to Jürn Brodersen from comment #20)
> Created attachment 7619 [details]
> define widget in the backend
> 
> Adding the referencing objects page from the frontend was problematic
> because it was not easily possible to decide early enough if the tab should
> be shown or not.
> 
> As discussed I put the logic back in to the backend and added a widget
> (based on the LinkList widget) to the umc package for showing referencing
> objects.
That looks nice!
The _setValueAttr() function in the new ReferencingObjects widget should replace all current childs. (e.g. if set('value', ...) is called twice it would be duplicated).
Comment 22 Alexander Kläser univentionstaff 2016-04-26 15:00:38 CEST
(In reply to Jürn Brodersen from comment #20)
> Created attachment 7619 [details]
> define widget in the backend
> 
> Adding the referencing objects page from the frontend was problematic
> because it was not easily possible to decide early enough if the tab should
> be shown or not.
> 
> As discussed I put the logic back in to the backend and added a widget
> (based on the LinkList widget) to the umc package for showing referencing
> objects.

Nice :) ! You need call to this._set('value', value) at the end of _setValueAttr(). This is needed for the internal watch-mechanism to work properly. _set() will call registered observers for a value and will update this.value, as well:

https://dojotoolkit.org/documentation/tutorials/1.8/understanding_widgetbase/
Comment 23 Jürn Brodersen univentionstaff 2016-04-26 15:32:21 CEST
Created attachment 7623 [details]
Proposed patch

New Patch
Comment 24 Jürn Brodersen univentionstaff 2016-04-26 16:50:13 CEST
UCS4.1-1:

r68915: show the right referenced objects for policies
r68916: YAML
Package: univention-management-console-module-udm
Version: 6.0.11-16.646.201604261629
Comment 25 Florian Best univentionstaff 2016-04-29 11:25:14 CEST
### jshint udm/ReferencingObjects.js ###
Extra comma. (udm/ReferencingObjects.js:40:34)
> "umc/widgets/ContainerWidget",

Extra comma. (udm/ReferencingObjects.js:42:75)
> return declare("umc.modules.udm.ReferencingObjects", [ ContainerWidget, ], {

Unnecessary semicolon. (udm/ReferencingObjects.js:64:14)
> };

'on' is defined but never used. (udm/ReferencingObjects.js:41:37)
> ], function(declare, lang, array, on, topic, Button, tools, app, ContainerWidget) {
Comment 26 Jürn Brodersen univentionstaff 2016-04-29 12:02:37 CEST
UCS4.1-1:

r69017: code cleanup
r69018: YAML
Package: univention-management-console-module-udm
Version: 6.0.11-17.647.201604291153
Comment 27 Florian Best univentionstaff 2016-04-29 15:12:18 CEST
Ok, the changes are now very nice.
YAML: adjusted in svn r68987
Comment 28 Janek Walkenhorst univentionstaff 2016-05-04 18:10:30 CEST
<http://errata.software-univention.de/ucs/4.1/167.html>