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 Cookie API #1313

Open
wants to merge 3 commits into
base: dev/3.0.0
Choose a base branch
from
Open

Add Cookie API #1313

wants to merge 3 commits into from

Conversation

Luccboy
Copy link
Contributor

@Luccboy Luccboy commented May 5, 2024

I tried adding the missing cookie API. I tested it and it seems to work fine. I would appreciate any feedback.

@pop4959
Copy link
Member

pop4959 commented May 5, 2024

This looks overall pretty good, but my biggest concern here is that there is no handling over cookie packets sent or retrieved by backend servers. I think I would like this better if it moved away from CompletableFuture and instead fired events similar to what is done for PluginMessageEvent so you can identify the source, destination, whether to forward it, and have the ability to modify the data before sending to/from the backend.

@electronicboy
Copy link
Member

sunday, bad eye; Using a map like that would also create dozens of issues if multiple things tried to request that data

@Luccboy
Copy link
Contributor Author

Luccboy commented May 5, 2024

Agreed, I will try to implement your feedback. Thanks!

@kashike kashike added the type: feature New feature or request label May 5, 2024
@Luccboy Luccboy marked this pull request as draft May 5, 2024 22:03
@Luccboy
Copy link
Contributor Author

Luccboy commented May 7, 2024

Hi, I'm currently working on this and I'm unsure how to proceed. Do you want to have an event for each cookie packet or is one event for the CookieResponse packet sufficient? Also, to determine the destination of a CookieResponse packet, I would need to somehow keep track of the cookies requested by the proxy, right? - what would be the best way to do this, maybe a queue?

@pop4959
Copy link
Member

pop4959 commented May 8, 2024

Maybe it helps to think about the data flow here, we have:
StoreCookiePacket which is server -> client, and in-between the proxy may want to block or modify the result.
CookieRequestPacket which is also server -> client, and in-between the proxy may want to block.
CookieResponsePacket which is client -> server, and again in-between the proxy may want to block or modify.

If you look at PluginMessageEvent you basically have the same idea of keyable custom payloads between the client and server, although there the message interaction is two way, server <-> client. It would not be necessary to distinguish between the source and destination if there are separate events here since the client -> server and server -> client flow is implied. You'd be able to figure everything out based on the player the events are being handled for.

Correct me if I'm wrong, but I don't think there is any reason for the proxy to store anything here, since proxy plugins will be able to listen to the events fired here and store any intermediate results they need before it gets forwarded, or cancel the event if it doesn't want them forwarded.

@Luccboy
Copy link
Contributor Author

Luccboy commented May 8, 2024

Thanks for the clarification, I agree that if we let plugins decide whether to forward the packet or not, we don't need to store anything on the proxy.

@Luccboy
Copy link
Contributor Author

Luccboy commented May 10, 2024

Short update: The only thing missing in my opinion is how to forward the CookieResponsePacket to the backend server in the login phase, because the backend server to which the client should be connected has not yet been determined. But I also noticed, that when a backend server requests a cookie in login phase, the client is already in config phase. So it should not be possible to receive a CookieResponsePacket from a client in login phase caused by a cookie request from a backend server in login phase.

@Heliumdioxid
Copy link

That sounds good to me: If the server can only send a CookieRequestPacket once the client is in config phase, the CookieResponsePacket doesn't have to be forwarded if it is sent in login phase (as it was necessarily requested by Velocity).

@Luccboy Luccboy marked this pull request as ready for review May 12, 2024 13:48
@GrayR0ot
Copy link

Amazing PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants