Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define wsBeforeUpgrade via WsConfig #2221

Open
LoonyRules opened this issue Apr 7, 2024 · 2 comments
Open

Define wsBeforeUpgrade via WsConfig #2221

LoonyRules opened this issue Apr 7, 2024 · 2 comments

Comments

@LoonyRules
Copy link
Contributor

Describe the feature
Add a onBeforeUpgrade to the WsConfig for when a Ws endpoint is registered:

javalin.ws("/ws", wsConfig -> {
  wsConfig.onBeforeUpgrade(ctx -> {
    if (!ctx.isAuthorized()) { // Pseudo method.
      throw UnauthorizedResponse();
    }
  });,
  
  wsConfig.onConnect(ctx -> {
    // Only invoked if `onBeforeUpgrade` succeeds.
  });
});

This is the same as doing:

javalin.wsBeforeUpgrade("/ws", ctx -> {
  if (!ctx.isAuthorized()) { // Pseudo method.
    throw UnauthorizedResponse();
  }
});
javalin.ws("/ws", wsConfig -> {
  wsConfig.onConnect(ctx -> {
    // Only invoked if `onBeforeUpgrade` succeeds.
  });
});

but allows you to:

  1. Define the path once.
  2. Easily define an 'access manager' per websocket path.
  3. Stops you from accidentally doing javalin.wsBeforeUpgrade("/wws", ctx -> { ... }); and javalin.ws("/ws", wsConfig -> { ... }); and not realising that the before upgrade isn't ever going to match a path, so it results in a 404 error, and now /ws is unauthorized when the application boots. (stumped me for about 30 seconds, then realised I was being dumb).

Additional context
I think it is extremely important to continue supporting the wsBeforeUpgrade(ctx -> { ... }); and wsBeforeUpgrade(path, ctx -> { ... }); methods. The WsConfig introduction is essentially the same thing under the hood.
Add any other context about the feature request here

@tipsy
Copy link
Member

tipsy commented Apr 8, 2024

@LoonyRules I think it is extremely important to continue supporting the wsBeforeUpgrade(ctx -> { ... }); and wsBeforeUpgrade(path, ctx -> { ... }); methods. The WsConfig introduction is essentially the same thing under the hood.

There seems to be significant overlap here, why do you think it's important to keep both?

(You could eliminate the path issue by using the path(...) method? 🤔)

@LoonyRules
Copy link
Contributor Author

Ease of use is the only thing I can think of. If you want all web sockets upgrades to be authenticated, then you could just define 1 wsBeforeUpgrade(ctx -> { ... }) handler instead of 1-per-ws, something that you might not necessarily have access to (at the WsConfig level) if you registered the wsBeforeUpgrade in a framework rather than at the application impl level, where the .ws("/ws", wsConfig -> { ... }); is defined.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants