-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
base: master
Are you sure you want to change the base?
Conversation
I have also written an improved AuthorizationListener at https://github.com/oliverhausler/netty-socketio/tree/1.7.13-improved-authorization
Data can then be retrieved as `client.get("key"); untested, undocumented |
result = configuration.getAuthorizationListener().isAuthorized(data); | ||
redirectUrl = configuration.getAuthorizationListener().isRedirected(data); | ||
} catch (Exception ignore) { | ||
} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.