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.

Sign up for GitHub

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

Add 307 redirect to AuthorizationListener #620

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add 307 redirect to AuthorizationListener #620

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions src/main/java/com/corundumstudio/socketio/AuthorizationListener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,17 @@
public interface AuthorizationListener {

/**
* Checks is client with handshake data is authorized
* Checks if client with handshake data is redirected (307)
*
* @return - the <b>URL</b> if client is redirected or <b>null</b> otherwise
*/
String isRedirected(HandshakeData data);

/**
* Checks if client with handshake data is authorized
*
* @param data - handshake data
* @return - <b>true</b> if client is authorized of <b>false</b> otherwise
* @return - <b>true</b> if client is authorized or <b>false</b> otherwise
*/
boolean isAuthorized(HandshakeData data);

Expand Down
21 changes: 18 additions & 3 deletions src/main/java/com/corundumstudio/socketio/handler/AuthorizeHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,14 +146,29 @@ private boolean authorize(ChannelHandlerContext ctx, Channel channel, String ori
(InetSocketAddress)channel.remoteAddress(),
req.uri(), origin != null && !origin.equalsIgnoreCase("null"));

boolean result = false;
String redirectUrl = null;
try {
result = configuration.getAuthorizationListener().isAuthorized(data);
redirectUrl = configuration.getAuthorizationListener().isRedirected(data);
} catch (Exception ignore) {
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bad practice to swallow Exceptions. Please fix

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not working on this PR branch anymore. I decided to carefully break compatibility in some places to implement more features, see #631 instead, where this is fixed.

Instead returning isRedirected in a separate @OverRide, I decided to move it into the same authorize function and return a response object instead a boolean. This breaks compatibility, but enables us to do a lot more.

I'd suggest you reject this PR and switch to the newer one.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

8fb13b0


if (redirectUrl != null) {
HttpResponse res = new DefaultHttpResponse(HTTP_1_1, HttpResponseStatus.TEMPORARY_REDIRECT);
res.headers().add("Location", redirectUrl);
channel.writeAndFlush(res)
.addListener(ChannelFutureListener.CLOSE);
log.debug("Handshake redirected, query params: {} headers: {}", params, headers);
return false;
}

boolean isAuthorized = false;
try {
isAuthorized = configuration.getAuthorizationListener().isAuthorized(data);
} catch (Exception e) {
log.error("Authorization error", e);
}

if (!result) {
if (!isAuthorized) {
HttpResponse res = new DefaultHttpResponse(HTTP_1_1, HttpResponseStatus.UNAUTHORIZED);
channel.writeAndFlush(res)
.addListener(ChannelFutureListener.CLOSE);
Expand Down
5 changes: 5 additions & 0 deletions src/main/java/com/corundumstudio/socketio/handler/SuccessAuthorizationListener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,11 @@

public class SuccessAuthorizationListener implements AuthorizationListener {

@Override
public String isRedirected(HandshakeData data) {
return null;
}

@Override
public boolean isAuthorized(HandshakeData data) {
return true;
Expand Down