Wicket
  1. Wicket
  2. WICKET-3471

WicketTester checkUsability is called before the request has started

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.4.15
    • Fix Version/s: 1.4.18, 1.5-RC5
    • Component/s: wicket
    • Labels:
      None

      Description

      For example, when clicking a link, the checkUsability method is called with the link before RequestCycle.onBeginRequest has been called. This causes tests to fail when, for example, the link has a security check that requires the request cycle. I've created a quickstart that demonstrates the problem.

      1. quickstart.tar.bz2
        5 kB
        Emond Papegaaij
      2. WICKET-3471.patch
        4 kB
        Martijn Dashorst
      3. WICKET-3471.patch
        8 kB
        Pedro Santos

        Activity

        Hide
        Martin Grigorov added a comment -

        Made the method protected so it can be suppressed when the user app really needs this.

        Show
        Martin Grigorov added a comment - Made the method protected so it can be suppressed when the user app really needs this.
        Hide
        Pedro Santos added a comment -

        AjaxLink has an AjaxEventBehavior that do not override Behavior#canCallListenerInterface, so invoking the behavior when its component is not enabled is not allowed, this is the default policy. TestBehaviorDisabledComponentAllowed override the canCallListenerInterface to allow the use case at 'testBehaviorOnDisabledComponentAllowed()'

        Show
        Pedro Santos added a comment - AjaxLink has an AjaxEventBehavior that do not override Behavior#canCallListenerInterface, so invoking the behavior when its component is not enabled is not allowed, this is the default policy. TestBehaviorDisabledComponentAllowed override the canCallListenerInterface to allow the use case at 'testBehaviorOnDisabledComponentAllowed()'
        Hide
        Martin Grigorov added a comment -

        Hi,

        I have a question for the patch.
        Test 'testBehaviorOnDisabledComponentAllowed()' allows execution of Ajax behavior on disabled component. The next test 'testClickOnDisableAjaxLinkNotAllowed()' checks that clicking on disabled AjaxLink is not allowed. AjaxLink is just a Link with AjaxEventBehavior("onclick").
        It looks to me that both methods contradict to each other. What I'm missing ?

        Show
        Martin Grigorov added a comment - Hi, I have a question for the patch. Test 'testBehaviorOnDisabledComponentAllowed()' allows execution of Ajax behavior on disabled component. The next test 'testClickOnDisableAjaxLinkNotAllowed()' checks that clicking on disabled AjaxLink is not allowed. AjaxLink is just a Link with AjaxEventBehavior("onclick"). It looks to me that both methods contradict to each other. What I'm missing ?
        Hide
        Martijn Dashorst added a comment -

        Just as a reminder to myself: need to revert r140505 in our svn repository when this issue is resolved.

        Show
        Martijn Dashorst added a comment - Just as a reminder to myself: need to revert r140505 in our svn repository when this issue is resolved.
        Hide
        Pedro Santos added a comment - - edited

        Hi Martjin, iirc it was add because the core was failing in to refuse execute behaviors in not enabled/disable behaviors. But it is fixed now and I'm 1+ to bring this check responsibility back to core request processing. Initial investigations shows that we can remove the checkUsability from clickLink and executeAjaxEvent methods. Sending the patch with tests.

        Show
        Pedro Santos added a comment - - edited Hi Martjin, iirc it was add because the core was failing in to refuse execute behaviors in not enabled/disable behaviors. But it is fixed now and I'm 1+ to bring this check responsibility back to core request processing. Initial investigations shows that we can remove the checkUsability from clickLink and executeAjaxEvent methods. Sending the patch with tests.
        Hide
        Martijn Dashorst added a comment -

        The big question in my opinion is why doesn't Wicket throw an exception from inside core when an invisible/disabled component receives an event during wickettester processing? Why is that checkUsability necessary?

        Show
        Martijn Dashorst added a comment - The big question in my opinion is why doesn't Wicket throw an exception from inside core when an invisible/disabled component receives an event during wickettester processing? Why is that checkUsability necessary?
        Hide
        Martijn Dashorst added a comment -

        A possible way to address this is to proxy the requestcycle and perform the check after onBeginRequest. Problem is that the component to check is not always known in the code paths...

        Looking at this it seems wickettester is a big pain in the behind: a big giant tangled mess.

        Show
        Martijn Dashorst added a comment - A possible way to address this is to proxy the requestcycle and perform the check after onBeginRequest. Problem is that the component to check is not always known in the code paths... Looking at this it seems wickettester is a big pain in the behind: a big giant tangled mess.
        Hide
        Martijn Dashorst added a comment -

        This fixes our issues, discovers a hidden bug in our test cases, and checks for usability (not-hidden, nor disabled) in even more places

        Show
        Martijn Dashorst added a comment - This fixes our issues, discovers a hidden bug in our test cases, and checks for usability (not-hidden, nor disabled) in even more places
        Hide
        Martin Grigorov added a comment -

        I really don't see a way to move checkUsability() inside the request cycle. There is no information about the component/behavior at that point (it gets resolved at later point when it is too late to make the check and actually is inside Wicket core core, not WicketTester).

        As I said earlier I see two solutions:
        1) make it 'protected' so you can override it to do what you need if needed
        2) remove it completely

        I prefer 1).
        checkUsability() may not be perfect and doesn't cover all cases but seems to be good enough for most of them.

        Other opinions ?

        Show
        Martin Grigorov added a comment - I really don't see a way to move checkUsability() inside the request cycle. There is no information about the component/behavior at that point (it gets resolved at later point when it is too late to make the check and actually is inside Wicket core core, not WicketTester). As I said earlier I see two solutions: 1) make it 'protected' so you can override it to do what you need if needed 2) remove it completely I prefer 1). checkUsability() may not be perfect and doesn't cover all cases but seems to be good enough for most of them. Other opinions ?
        Hide
        Emond Papegaaij added a comment -

        Of course, we do not use the check I used for the quickstart, it only serves to demonstrate the problem. In our application we set up a context in onBeginRequest and tear it down in onEndRequest. This context is required for some of the security checks to be performed. For example, this context contains the organisation the current user belongs to and one of the security checks verifies that the link targets a page that is part of a module that is active for that organisation. This security check currently fails, because the organisation has not yet been set on the context.

        Show
        Emond Papegaaij added a comment - Of course, we do not use the check I used for the quickstart, it only serves to demonstrate the problem. In our application we set up a context in onBeginRequest and tear it down in onEndRequest. This context is required for some of the security checks to be performed. For example, this context contains the organisation the current user belongs to and one of the security checks verifies that the link targets a page that is part of a module that is active for that organisation. This security check currently fails, because the organisation has not yet been set on the context.
        Hide
        Martin Grigorov added a comment -

        This usecase is a bit strange to me - a component is enabled only when there is a request.
        checkUsability() is there to prevent usage of (e.g. clicking on) disabled/invisible components. In real environment the browser cannot fire event on such component.
        I am not sure your use case can be supported in WicketTester. We either need to remove the whole check or make this method protected so you can override it locally to support your need.

        Show
        Martin Grigorov added a comment - This usecase is a bit strange to me - a component is enabled only when there is a request. checkUsability() is there to prevent usage of (e.g. clicking on) disabled/invisible components. In real environment the browser cannot fire event on such component. I am not sure your use case can be supported in WicketTester. We either need to remove the whole check or make this method protected so you can override it locally to support your need.

          People

          • Assignee:
            Martin Grigorov
            Reporter:
            Emond Papegaaij
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development