Updates to Allow/White List handling#359
Conversation
If the ALLOW_LIST_USERS or WHITE_LIST_USERS env variables were set, the existing code would append white-list=false to server.properties, which resulted in conflicting variables (allow-list=true , white-list=false). Because the variable is still currently supported, it would result in inconsistent behavior from the server. This commit removes all reference to the white-list variable, as support for it is being deprecated by Microsoft and also includes omission of the WHITE_LIST=false line.
|
Can you read through #336 and make sure your proposed change doesn't introduce a regression? I feel like your proposed change was already attempted and it helped half the people and hindered the other half. If you found the winning combo, then this will be awesome. |
|
... |
I've manually removed the |
|
I can't seem to reproduce the expected behavior of unsetting |
This proposal does the following: If neither ALLOW_LIST_USERS or WHITE_LIST_USERS env variables are set, the allowlist.json is removed and ALLOW_LIST is exported as a false value. If both of variables are set, ALLOW_LIST_USERS takes precedence and WHITE_LIST_USERS is ignored. If only one of these variables are set, it will function as expected. ALLOW_LIST is the only exported variable, which should be sufficient for server.properties. It's also exported outside of the if block.
|
I've updated my original commit to include handling of the I've done testing in a local environment and this commit behaves as you'd expect. Additionally, it should address #336 which I believe was a result of the allowlist.json not being removed or regenerated with env variable changes. |
|
Not sure how to rectify any existing server.properties files out there that have a white-list=false line at the end. I suppose we could reference a skeleton server.properties file and rebuild it on startup. This would require mapping every value to an environmental variable though. Thoughts? |
It's important to solve this. That's the gap I had left in my previous attempts. Maybe I could follow up to your PR with merging #351 Or just |
|
I think we can use sed to remove any existing Also, I've confirmed that my commit is working as expected with removing / generating the allowlist.json file and setting allow-list in server.properties but I am also using absolute paths to these files in my testing ( |
|
|
This will remove any line beginning with "white-list=" in the server.properties file, regardless of if ALLOW_LIST_USERS or WHITE_LIST_USERS env variables are set or not. This should resolve any issues resulting from the addition of this line from previous versions of the docker image where env variables ALLOW_LIST_USERS or WHITE_LIST_USERS were set.
|
I've updated the code to include an implementation of sed that will remove any line that begins with "white-list" from server.properties, regardless of if the ALLOW_LIST_USERS or WHITE_LIST_USERS is set, as it's outside of the block that checks those variables. Users will still be able to use both of these env variables which both ultimately export a Boolean value for ALLOW_LIST, which is confirmed to update server.properties file. With the removal of the white-list entry, conflicting values in server.properties should not be an issue. Please review and let me know your thoughts. |
itzg
left a comment
itzg
left a comment
There was a problem hiding this comment.
One last little tweak and it's good to merge.
bedrock-entry.sh
Outdated
| else | ||
| rm -rf allowlist.json | ||
| ALLOW_LIST=false | ||
| rm allowlist.json |
There was a problem hiding this comment.
Just in case the file isn't present
| rm allowlist.json | |
| rm -f allowlist.json |
bedrock-entry.sh
Outdated
| export WHITE_LIST ALLOW_LIST | ||
| else | ||
| ALLOW_LIST=false | ||
| rm allowlist.json |
|
Should be ready now. Thanks for pointing that out |
2 participants
If the ALLOW_LIST_USERS or WHITE_LIST_USERS env variables were set, the existing code would append white-list=false to server.properties, which resulted in conflicting variables (allow-list=true , white-list=false). Because the variable is still currently supported, it would result in inconsistent behavior from the server.
This commit removes all reference to the white-list variable, as support for it is being deprecated by Microsoft and also includes omission of the WHITE_LIST=false line.