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

fix secure profile with proxy online mode #9183

Closed
wants to merge 4 commits into from
Closed

fix secure profile with proxy online mode #9183

wants to merge 4 commits into from

Conversation

NonSwag
Copy link
Contributor

@NonSwag NonSwag commented May 4, 2023

This change fixes the problem with the new unverified chat toast if the server is in offline-mode behind an online-mode proxy.

@NonSwag NonSwag requested a review from a team as a code owner May 4, 2023 17:50
@NonSwag
Copy link
Contributor Author

NonSwag commented May 7, 2023

Someone wants to review this ?

Copy link
Contributor

@lynxplay lynxplay left a comment

Choose a reason for hiding this comment

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

This, unless I am missing something, can just use io.papermc.paper.configuration.GlobalConfiguration.Proxies#isProxyOnlineMode to avoid this code duplication.

@NonSwag
Copy link
Contributor Author

NonSwag commented May 7, 2023

Thanks for the tip, done!

@NonSwag NonSwag requested a review from lynxplay May 7, 2023 15:14
Copy link
Member

@Warriorrrr Warriorrrr left a comment

Choose a reason for hiding this comment

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

Shouldn't this be enforceSecureProfile && (onlineMode || proxyOnlineMode), right now if the enforce-secure-profile option is disabled in the properties but proxy online mode is used it'll be enforced, or is this intended?

@NonSwag
Copy link
Contributor Author

NonSwag commented May 7, 2023

missed that sorry

@NonSwag NonSwag requested a review from Warriorrrr May 7, 2023 20:01
@MiniDigger
Copy link
Member

please merge your patches by amending your original commit instead of creating a new one

@NonSwag
Copy link
Contributor Author

NonSwag commented May 9, 2023

I don't know how to merge commits after they are already pushed
I've deleted the first file, so the merge would only consist out of 17e7bb7

@NonSwag
Copy link
Contributor Author

NonSwag commented May 10, 2023

Anything else that I can improve, or does someone want to accept/close the PR?

@electronicboy
Copy link
Member

electronicboy commented May 10, 2023

Has this been properly tested on velocity and bungee/waterfall, especially around stuff like server switches? I'd imagine velocity is fine, not sure about the latter, however

@NonSwag
Copy link
Contributor Author

NonSwag commented May 11, 2023

Yes, I've tested all of them.
Velocity is working as it should (it just forwards the value the requested server provides).
BungeeCord works even without that (I guess they just say true all the time).
Waterfall does (seemingly) not change the behavior of BungeeCord.
I can't see any (negative) effects from that code change on BungeeCord or Waterfall.

@electronicboy
Copy link
Member

electronicboy commented May 11, 2023 via email

@NonSwag
Copy link
Contributor Author

NonSwag commented May 11, 2023

I've checked everything related to that option.
It changes just the ServerData which is only sent to the user and the chat signing.
image

@NonSwag
Copy link
Contributor Author

NonSwag commented May 12, 2023

Anything else that I can improve, or does someone want to accept/close the PR?

what about now ?

@NonSwag
Copy link
Contributor Author

NonSwag commented May 15, 2023

Hello?

@Warriorrrr
Copy link
Member

The patch as it is now won't apply since you deleted the first patch that it depended on, you need to undo the deletion of the first patch and then amend the second one into it like mini said.

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.

5 participants