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

add the user to automatic rights groups #516

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

NotAFile
Copy link
Member

add the team's user type and also a generic all user type

This allows you to give permissions to run certain, usually restricted
commands to anyone or to a certain team

closes #475

add the team's user type and also a generic all user type

This allows you to give permissions to run certain, usually restricted
commands to anyone or to a certain team
@NotAFile NotAFile added this to In progress in Sauerkraut via automation May 19, 2019
@NotAFile NotAFile added this to In Progress in New Features via automation May 19, 2019
Copy link

@samuelallan72 samuelallan72 left a comment

Choose a reason for hiding this comment

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

It would be nice to document the behaviour of this - at least a comment in the example config.toml and details in the sphinx docs.

team_usertype = None

if team is self.protocol.team_1:
team_usertype = "team_1"

Choose a reason for hiding this comment

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

Perhaps we could use team1 and team2 to be consistent with the toml config sections?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good point. Kind of unfortunate that our config and code differ now.

elif team.spectator:
team_usertype = "spectator"

if team_usertype:

Choose a reason for hiding this comment

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

If no team_usertype, shouldn't all the special usertypes still be removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's true. team_usertype will never be None in the forseeable future, so the if is pretty pointless anyway.

@@ -88,8 +91,6 @@ def on_login(self, name: str) -> None:
self.protocol.irc_say('* %s (IP %s, ID %s) entered the game!' %
(self.name, self.address[0], self.player_id))
if self.user_types is None:

Choose a reason for hiding this comment

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

Change to if not self.user_types: since it might be an empty set rather than None.

Copy link
Member Author

@NotAFile NotAFile May 21, 2019

Choose a reason for hiding this comment

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

Hm, you're right, we have a problem here. But it's a different one. on_join is called multiple times for the lifetime of a connection, once per match. We probably don't want to login the users as admin every time when a new match starts. But we need the user to be joined to be able to execute on_user_login as it might send chat messages.

Easiest thing is probably to defer initialization of the sets to on_join after all.

@samuelallan72
Copy link

samuelallan72 commented May 21, 2019

Something else to consider: server config shouldn't allow giving passwords for built-in roles such as team_1, and users should not be able to /login team_1

relevant code:

@command()
def login(connection, password):
"""
Log in if you're staff or a trusted member of this server
/login <password>
You will be kicked if a wrong password is given 3 times in a row
"""
if connection not in connection.protocol.players:
raise KeyError()
for user_type, passwords in connection.protocol.passwords.items():
if password in passwords:
if user_type in connection.user_types:
return "You're already logged in as %s" % user_type
return connection.on_user_login(user_type, True)
if connection.login_retries is None:
connection.login_retries = connection.protocol.login_retries - 1
else:
connection.login_retries -= 1
if not connection.login_retries:
connection.kick('Ran out of login attempts')
return
return 'Invalid password - you have %s tries left' % (
connection.login_retries)

@NotAFile
Copy link
Member Author

NotAFile commented May 21, 2019

server config shouldn't allow giving passwords for built-in roles such as team_1, and users should not be able to /login team_1

Why not? I don't see any harm in this technically being possible. It should work fine, but it'd be pretty pointless... I don't think we need to protect our users from being able to do something pointless?

@samuelallan72
Copy link

Why not? I don't see any harm in this technically being possible. It should work fine, but it'd be pretty pointless... I don't think we need to protect our users from being able to do something pointless?

I guess. I was thinking just a check that rejected logging in with a message about it being an automatic/dynamic role.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
New Features
  
In Progress
Sauerkraut
  
In progress
Development

Successfully merging this pull request may close these issues.

pseudo-roles for all, team_1, team_2
2 participants