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

Refactor network groups out of network #9921

Open
wants to merge 25 commits into
base: develop
from

Conversation

@ZehMatt
Copy link
Contributor

ZehMatt commented Aug 22, 2019

Another PR to clean up more code in the Network area, this will probably take a bit to complete.

@ZehMatt ZehMatt force-pushed the ZehMatt:refactor/network-groups branch from df576c6 to 57216ec Dec 11, 2019
@ZehMatt ZehMatt force-pushed the ZehMatt:refactor/network-groups branch 3 times, most recently from 15f5600 to 96c85e1 Dec 27, 2019
@ZehMatt ZehMatt force-pushed the ZehMatt:refactor/network-groups branch from 96c85e1 to 1a6e755 Dec 27, 2019
@ZehMatt ZehMatt marked this pull request as ready for review Dec 27, 2019
@ZehMatt

This comment has been minimized.

Copy link
Contributor Author

ZehMatt commented Dec 27, 2019

Some things I should mention what this PR does:

  • Introduces a "Host" group which the server operates on, users can not modify or assign other users to this group.
  • Fixes an actual bug that didn't allow the host to change the group of users because the player id is always -1 when not networked first.
  • Keeps compatibility of the current groups.json

Also the "Admin" group is currently immutable which I assume has been only the case because the host was enforced to use the admin group, now that this is no longer the case should we allow users to modify the admin group?

@ZehMatt ZehMatt force-pushed the ZehMatt:refactor/network-groups branch from 1a6e755 to d5e3408 Dec 31, 2019
@ZehMatt ZehMatt force-pushed the ZehMatt:refactor/network-groups branch from 9b9f6a1 to 8d78f05 Feb 2, 2020
@ZehMatt ZehMatt requested a review from duncanspumpkin Feb 2, 2020
@Broxzier

This comment has been minimized.

Copy link
Member

Broxzier commented Feb 8, 2020

Also the "Admin" group is currently immutable which I assume has been only the case because the host was enforced to use the admin group, now that this is no longer the case should we allow users to modify the admin group?

Yes. I think starting off with a few default groups like "Admin", "User, and "Spectator" are good, but give full control about them to the user.

Copy link
Contributor

duncanspumpkin left a comment

This looks fine. We had a discussion about using a std::vector for the _groups on gitter but its fine to do it this way as well.

Its not the greatest to use unique ptr .get() but at least every instance of this doesn't take any ownership.

ZehMatt added 10 commits Aug 19, 2019
…bility for host group
ZehMatt and others added 14 commits Dec 27, 2019
@ZehMatt ZehMatt force-pushed the ZehMatt:refactor/network-groups branch from 8d78f05 to b960453 Feb 14, 2020
@ZehMatt

This comment has been minimized.

Copy link
Contributor Author

ZehMatt commented Feb 14, 2020

I've modified the way it loads and saves the configuration to use names which is much easier for the initial setup and doesn't require to back track files.

The new format for groups.json:

{
    "default_group": "Spectator",
    "groups": [
        {
            "name": "Admin",
            "permissions": [
                "PERMISSION_CHAT",
                "PERMISSION_TERRAFORM",
                "PERMISSION_SET_WATER_LEVEL",
                "PERMISSION_TOGGLE_PAUSE",
                "PERMISSION_CREATE_RIDE",
                "PERMISSION_REMOVE_RIDE",
                "PERMISSION_BUILD_RIDE",
                "PERMISSION_RIDE_PROPERTIES",
                "PERMISSION_SCENERY",
                "PERMISSION_PATH",
                "PERMISSION_CLEAR_LANDSCAPE",
                "PERMISSION_GUEST",
                "PERMISSION_STAFF",
                "PERMISSION_PARK_PROPERTIES",
                "PERMISSION_PARK_FUNDING",
                "PERMISSION_KICK_PLAYER",
                "PERMISSION_MODIFY_GROUPS",
                "PERMISSION_SET_PLAYER_GROUP",
                "PERMISSION_CHEAT",
                "PERMISSION_TOGGLE_SCENERY_CLUSTER",
                "PERMISSION_PASSWORDLESS_LOGIN",
                "PERMISSION_MODIFY_TILE",
                "PERMISSION_EDIT_SCENARIO_OPTIONS"
            ]
        },
        {
            "name": "Spectator",
            "permissions": [
                "PERMISSION_CHAT"
            ]
        },
        {
            "name": "User",
            "permissions": [
                "PERMISSION_CHAT",
                "PERMISSION_TERRAFORM",
                "PERMISSION_SET_WATER_LEVEL",
                "PERMISSION_TOGGLE_PAUSE",
                "PERMISSION_CREATE_RIDE",
                "PERMISSION_REMOVE_RIDE",
                "PERMISSION_BUILD_RIDE",
                "PERMISSION_RIDE_PROPERTIES",
                "PERMISSION_SCENERY",
                "PERMISSION_PATH",
                "PERMISSION_CLEAR_LANDSCAPE",
                "PERMISSION_GUEST",
                "PERMISSION_STAFF",
                "PERMISSION_PARK_PROPERTIES",
                "PERMISSION_PARK_FUNDING",
                "PERMISSION_TOGGLE_SCENERY_CLUSTER"
            ]
        },
        {
            "name": "격리중",
            "permissions": []
        }
    ]
}

The new format for users.json:

[
    {
        "hash": "",
        "name": "Matt",
        "group": "Host",
    },
    {
        "hash": "4d0e90c173766a2535a53eb2f62c82dfa016d680",
        "name": "Matt #2",
        "group": "격리중",
    },
    {
        "hash": "5cadab5de83ffb4ed9e69ba488be8d18378cb1bb",
        "name": "Zeh Matt",
        "group": "Admin",
    },
    {
        "hash": "a433e321e2f164d0cc8ab938353a539b8f918219",
        "name": "Matt #3",
        "group": "User",
    }
]

As for the new rules:
Duplicate names are not allowed, if the configuration file has two records of the same name it will overwrite the previous one.
If a specified group was not found the user will fallback to the default, if the default is improperly configured it will use to the builtin defaults.

Old configuration should not need to be changed as it will still use the id for a one way import to the new format.

I've bumped up the network version and added a changelog entry, from my end its good to go but it would be nice if someone could do a few tests.

@AaronVanGeffen

This comment has been minimized.

Copy link
Member

AaronVanGeffen commented Feb 14, 2020

Old configuration should not need to be changed as it will still use the id for a one way import to the new format.

Thank you for remembering to do this. :)

@IntelOrca

This comment has been minimized.

Copy link
Contributor

IntelOrca commented Feb 14, 2020

If names are unique, why not use that as the key in an object?

{
    "default_group": "Spectator",
    "groups": {
        "Admin": {},
        "Player": {},
        "Spectator": {}
    }
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.