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

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

wants to merge 1 commit into from

Conversation

oliverhausler
Copy link

Use case:

For building a server cluster, we wanted to 307 redirect to another machine before authentication occurs. I added an @Override isRedirected() to AuthorizationListener for this. I tried to make the change without breaking compatibility.

If it interests you, feel free to merge. If there's a better way to achieve this, please let me know.

I based the change on 1.7.13 because it's the latest stable version.

@mrniko mrniko added this to the 1.7.17 milestone Nov 25, 2018
@oliverhausler
Copy link
Author

I have also written an improved AuthorizationListener at https://github.com/oliverhausler/netty-socketio/tree/1.7.13-improved-authorization

public class CustomAuthorizationListener implements AuthorizationListener {

    @Override
    public AuthorizationResponse authorize(HandshakeData data) {
        return AuthorizationResponse.connect("key", "value"); // adds initial data to the store

        // Alternatives:
        // return AuthorizationResponse.disconnect(); // disconnects with 401 Unauthorized
        // return AuthorizationResponse.redirect("https://") // returns a redirect URL to the client
    }

}

Data can then be retrieved as `client.get("key");

untested, undocumented

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.

@mrniko mrniko modified the milestones: 1.7.17, 1.7.18 Sep 4, 2019
@mrniko mrniko modified the milestones: 1.7.18, 1.7.19 Aug 4, 2020
@mrniko mrniko modified the milestones: 1.7.19, 1.7.23 Oct 12, 2022
@mrniko mrniko modified the milestones: 1.7.23, 1.7.24 Apr 14, 2023
@mrniko mrniko modified the milestones: 1.7.24, 2.0.1 May 14, 2023
@mrniko mrniko modified the milestones: 2.0.1, 2.0.3 Jul 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants