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

Readd root/admin user detection #6593

Merged
merged 11 commits into from
Oct 2, 2021

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

patches/server/0806-Add-root-admin-user-detection.patch Outdated Show resolved Hide resolved
@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