Skip to content

Conversation

@NoahvdAa
Copy link
Member

@NoahvdAa NoahvdAa commented Sep 12, 2021

This PR re-adds the root/admin user warning originally introduced in #2432. The original patch was reverted (ecfaff5) because of console spam issues on CentOS. In this PR, a different way of checking for for root/admin privileges is used, which shouldn't cause console spam on certain systems.

Tested on macOS 11.4, Ubuntu 20.04, Centos 8 and Windows Server 2022.

@NoahvdAa NoahvdAa requested review from a team as code owners September 12, 2021 12:07
@molor
Copy link

molor commented Sep 12, 2021

In Windows Server, a single default user named Administrator is used on the every our host, and all user applications/services are run on his behalf (via RDP). We understand all the risks and agree with them, we take all the risks on ourselves. Where is the startup option that allows to bypass this (useless for us — me and my team) check and remove the warning?..

@NoahvdAa NoahvdAa requested review from Proximyst and removed request for a team September 12, 2021 14:26
Copy link
Contributor

@Proximyst Proximyst left a comment

Choose a reason for hiding this comment

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

https://docs.oracle.com/en/java/javase/16/docs/api/jdk.security.auth/com/sun/security/auth/module/package-summary.html
There’s API to detect this properly, apparently. Could you please take a look at this instead?

@NoahvdAa
Copy link
Member Author

Lynx and I came up with the solution used in 489bfa7, which uses Windows Security identifiers. This seems to match command prompt running as administrator correctly. I'm not very familiar with how Windows works, can someone who knows Windows a little better help confirm that this is the best way to check for this?

@lynxplay
Copy link
Contributor

Lynx and I came up with the solution used in 489bfa7, which uses Windows Security identifiers. This seems to match command prompt running as administrator correctly. I'm not very familiar with how Windows works, can someone who knows Windows a little better help confirm that this is the best way to check for this?

Added to this, neither of us are running windows so if someone with access to a windows machine / windows-server machine could validate if this PR works correctly, that would be sweet 👍

@NoahvdAa NoahvdAa requested a review from Proximyst September 13, 2021 05:35
@TheFruxz
Copy link
Contributor

MacOS should be checked too I think

@electronicboy
Copy link
Member

it already does

@NoahvdAa NoahvdAa requested a review from Proximyst September 20, 2021 05:43
@NoahvdAa NoahvdAa requested a review from me4502 September 20, 2021 08:18
Copy link
Contributor

@Proximyst Proximyst left a comment

Choose a reason for hiding this comment

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

Looks good

@me4502 me4502 force-pushed the feature/readd-root-detection branch from b0697bd to 8f9d43f Compare October 2, 2021 09:33
@me4502 me4502 merged commit 45c4f90 into PaperMC:master Oct 2, 2021
@NoahvdAa NoahvdAa deleted the feature/readd-root-detection branch October 2, 2021 09:35
e-im added a commit to e-im/Paper that referenced this pull request Oct 2, 2021
aurorasmiles pushed a commit that referenced this pull request Oct 2, 2021
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.

9 participants