vaadin / flow

Vaadin Flow is a Java framework binding Vaadin web components to Java. This is part of Vaadin 10+.
Apache License 2.0
601 stars 165 forks source link

PiT 24.4: Regression in alpha19 NoClassDefFoundError and redirection to line-awesome.min.css on login #19094

Closed manolo closed 4 months ago

manolo commented 5 months ago

Description of the bug

There is an exception error when running the project in my app

java.lang.NoClassDefFoundError: org/jsoup/safety/Safelist
    at com.vaadin.flow.router.AbstractRouteNotFoundError.setRouteNotFoundErrorParameter(AbstractRouteNotFoundError.java:80) ~[flow-server-24.4.0.alpha26.jar:24.4.0.alpha26]
    at com.vaadin.flow.router.RouteNotFoundError.setErrorParameter(RouteNotFoundError.java:36) ~[flow-server-24.4.0.alpha26.jar:24.4.0.alpha26]
    at com.vaadin.flow.router.internal.ErrorStateRenderer.notifyNavigationTarget(ErrorStateRenderer.java:121) ~[flow-server-24.4.0.alpha26.jar:24.4.0.alpha26]
    at com.vaadin.flow.router.internal.AbstractNavigationStateRenderer.notifyNavigationTarget(AbstractNavigationStateRenderer.java:616) ~[flow-server-24.4.0.alpha26.jar:24.4.0.alpha26]
    at com.vaadin.flow.router.internal.AbstractNavigationStateRenderer.sendBeforeEnterEvent(AbstractNavigationStateRenderer.java:593) ~[flow-server-24.4.0.alpha26.jar:24.4.0.alpha26]
    at com.vaadin.flow.router.internal.AbstractNavigationStateRenderer.sendBeforeEnterEventAndPopulateChain(AbstractNavigationStateRenderer.java:494) ~[flow-server-24.4.0.alpha26.jar:24.4.0.alpha26]
    at com.vaadin.flow.router.internal.AbstractNavigationStateRenderer.createChainIfEmptyAndExecuteBeforeEnterNavigation(AbstractNavigationStateRenderer.java:466) ~[flow-server-24.4.0.alpha26.jar:24.4.0.alpha26]
    at com.vaadin.flow.router.internal.AbstractNavigationStateRenderer.handle(AbstractNavigationStateRenderer.java:211) ~[flow-server-24.4.0.alpha26.jar:24.4.0.alpha26]
    at com.vaadin.flow.router.internal.ErrorStateRenderer.handle(ErrorStateRenderer.java:106) ~[flow-server-24.4.0.alpha26.jar:24.4.0.alpha26]
    at com.vaadin.flow.component.UI.handleErrorNavigation(UI.java:2015) ~[flow-server-24.4.0.alpha26.jar:24.4.0.alpha26]
    at com.vaadin.flow.component.UI.renderViewForRoute(UI.java:1912) ~[flow-server-24.4.0.alpha26.jar:24.4.0.alpha26]
    at com.vaadin.flow.component.UI.browserNavigate(UI.java:1795) ~[flow-server-24.4.0.alpha26.jar:24.4.0.alpha26]
    at com.vaadin.flow.component.ComponentEventBus.fireEventForListener(ComponentEventBus.java:239) ~[flow-server-24.4.0.alpha26.jar:24.4.0.alpha26]
    at com.vaadin.flow.component.ComponentEventBus.handleDomEvent(ComponentEventBus.java:488) ~[flow-server-24.4.0.alpha26.jar:24.4.0.alpha26]
    at com.vaadin.flow.component.ComponentEventBus.lambda$addDomTrigger$dd1b7957$1(ComponentEventBus.java:298) ~[flow-server-24.4.0.alpha26.jar:24.4.0.alpha26]
    at com.vaadin.flow.internal.nodefeature.ElementListenerMap.lambda$fireEvent$2(ElementListenerMap.java:447) ~[flow-server-24.4.0.alpha26.jar:24.4.0.alpha26]
    at java.base/java.util.ArrayList.forEach(ArrayList.java:1511) ~[na:na]
    at com.vaadin.flow.internal.nodefeature.ElementListenerMap.fireEvent(ElementListenerMap.java:447) ~[flow-server-24.4.0.alpha26.jar:24.4.0.alpha26]

After removing webdriver manager dependency, class not found has gone, but a redirection to a css resource happens

Screenshot 2024-04-08 at 08 30 19

Expected behavior

no exception, and redirection does not happen

Minimal reproducible example

run attached project

./mvnw -B
navigate to localhost:8080

k8s-demo-app.zip

UPDATED: App without the webdriver dependency

k8s-demo-app.zip

Versions

mshabarov commented 5 months ago

The above exception is caused by webdrivermanager being fetching an old jsoup version, where the Safelist class doesn't exist:

        <dependency>
            <groupId>io.github.bonigarcia</groupId>
            <artifactId>webdrivermanager</artifactId>
            <version>4.4.0</version>
            <scope>test</scope>
        </dependency>

webdrivermanager isn't actually needed in Vaadin, so I removed it and got rid of this exception.

Then I got another problem. When I logged in, the application immediately redirected me to http://localhost:8080/line-awesome/dist/line-awesome/css/line-awesome.min.css (application sent the redirect response after sending a login POST request /login). This obviously showed "Couldn't found a route..." page.

Then I tried to set up the line-awesome import in another way in the theme.json file:

{
  "lumoImports":["typography","color","spacing","badge","utility"],
  "importCss": [
    "line-awesome/dist/line-awesome/css/line-awesome.min.css"
  ]
}

and that solved the issue for me.

This weird redirect seems to happen after this patch. We shouldn't rely on theme.json way of importing external resource, because it's basically the same as having an import in the CSS file and shouldn't be considered as something unusual.

Thus, a real bug consists in having unexpected redirect to http://localhost:8080/line-awesome/dist/line-awesome/css/line-awesome.min.css after login.

NOTE: if I navigate to localhost:8080/hello or /about after login (even after I get redirected to a lineawesome url), the page is shown correctly.

I have no idea at the moment who triggers the redirect to line-awesome, but maybe we should redo the review of https://github.com/vaadin/flow/pull/19021.

knoobie commented 5 months ago

Thus, a real bug consists in having unexpected redirect to http://localhost:8080/line-awesome/dist/line-awesome/css/line-awesome.min.css after login.

This sounds like a static resource resolution problem, or? At least I won't expect them there.. if spring security is involved.. that path could have been cached before the login as unauthorized and you are therefore redirected to that "authorized" resource "you" (your browser) tried to load before login.. similar problems could be observed in earlier (v14) versions with e.g. favicon or other resources.

I have no idea at the moment who triggers the redirect to line-awesome

It's probably VaadinSavedRequestAwareAuthenticationSuccessHandler

mcollovati commented 5 months ago

Look at this comment about using NPM package assets into theme CSS: https://github.com/vaadin/flow/pull/19021#discussion_r1537940956 We should probably rewrite all relative URLs prepending them with VAADIN/themes/

mcollovati commented 5 months ago

And this issue also seems related to asset resolution in CSS

knoobie commented 5 months ago

We should probably rewrite all relative URLs prepending them with VAADIN/themes/

I'm not sure if that's a good idea.. let's say I add background image via css and a background.jpg to the images folder - which is a public folder within spring.. spring won't handle it anymore and you would look for it at the wrong location / look it up yourself

Edit: another example I've seen in the wild: resources / images that are used within the theme folder but unresolvable without a Reverse Proxy / cdn in Front - for example you wanna create a sellable application with different logos and so on.. because of the missing possibility to change themes at runtime; you would just add /cdn-images/bg.jpg to the css and let the Reverse Proxy in front of your Vaadin application handle that path and serve different images, depending on the installation / customer on premise

mcollovati commented 5 months ago

Good point. Another option may be to use an alias like the 'Frontend' we already have. So the asset URL could be something like 'Theme/path'

knoobie commented 5 months ago

I think I've seen that suggestion with an alias / protocol somewhere in the past.. Reminds me of the context:// prefix the JavaScript annotation has as well

ZheSun88 commented 5 months ago

about the webdriver issue, upgrade it to its latest seems fix the NoClassDefFoundError issue. (if that dependency is needed)

or we can just remove that dependency totally (PR for this change has been merged).

manolo commented 5 months ago

Now the error does no happen, but the redirection to the resource mentioned by @mshabarov is there making the PiT test fail like it's shown in the screenshot. So keeping this issue opened. Screenshot 2024-04-08 at 08 30 19

mshabarov commented 5 months ago

Could we apply a workaround to the demo app and put the line-awesome import into the theme.json file, as I shown above? The issue stays opened until we fix this redirect.

manolo commented 5 months ago

@mshabarov It's not a big deal to keep the app failing in the PiT output until we fix the problem, after the fix it should go green

mcollovati commented 5 months ago

After some investigation, I would say that using importCss in theme.json is not a workaround, but the correct and documented way to use CSS from NPM packages. The redirect in this case is expected because http://localhost:8080/line-awesome/dist/line-awesome/css/line-awesome.min.css is not a request Flow will handle since it cannot find the resource locally (node_modules might not exist if using a dev bundle) and it is also not a theme asset. Since Flow cannot make valid assumptions on the nature of the resource, the request cannot be automatically excluded from Spring Security checks.

importCss works because it is handled by a Vite plugin, so it can access node_module and verify for the file existence.

I propose to fix the demo project to use the CSS as documented, and close this issue.

manolo commented 4 months ago

@mcollovati I'm a bit confused do we need still to fix the demo or the https://github.com/vaadin/flow/pull/19140 fixes indirectly this issue ?

mcollovati commented 4 months ago

19140 fixes other CSS related issues, but the demo need to be fixed as well because the correct way to use CSS from NPM packages is to declare theme in theme.json with the importCss entry.

Trying to use it directly with an @import directive in CSS will not work at all.