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

Enhance the default permissions logic #929

Merged
1 commit merged into from
May 17, 2022
Merged

Enhance the default permissions logic #929

1 commit merged into from
May 17, 2022

Conversation

omg-xtao
Copy link
Contributor

@omg-xtao omg-xtao commented May 15, 2022

Description

Make default permissions not write to the database and be able to alter them dynamically.

Issues fixed by this PR

Type of changes

  • Bug fix
  • New feature
  • Enhancement
  • Documentation

Checklist:

  • My code follows the style guidelines of this project
  • My pull request is unique and no other pull requests have been opened for these changes
  • I have read the Contributing note and Code of conduct
  • I am responsible for any copyright issues with my code if it occurs in the future.

@ghost
Copy link

ghost commented May 15, 2022

This change makes more sense but I'd like opinions from other people about this before we merge.

@jie65535
Copy link
Contributor

I think many GM need this because the current situation is that it is very troublesome to change the default permissions of old users. For example, a new plug-in has been added with new permissions, which is a nightmare.

@Birdulon
Copy link
Member

This change makes more sense but I'd like opinions from other people about this before we merge.

Perhaps it would be useful to go one step further and have permission group aliases so that admins can define permission groups like user, poweruser, gm, and then add whatever permissions their plugins use to those groups as they please.

@exzork
Copy link
Member

exzork commented May 16, 2022

oh god, if this permission is updated it's mean we have to update the database :(

@omg-xtao
Copy link
Contributor Author

oh god, if this permission is updated it's mean we have to update the database :(

Why? Both new and existing users can have the default permissions.

@exzork
Copy link
Member

exzork commented May 16, 2022

For the default one sure it's, but for custom permission i think no.

@Birdulon
Copy link
Member

oh god, if this permission is updated it's mean we have to update the database :(

Under the current system yes, under this PR's system no.

Current system:

  • User is created, default permissions are added to their account and saved in the db for later.
  • When default permissions are changed, admin has to either:
    • leave old users without these changes, or
    • go through and update all users' individual permissions in the db.

This system:

  • User is created with no attached permissions.
  • Permission checks are performed against not only the individual user, but also against the defaults.
  • When default permissions are changed, they apply to all future permission checks for all users without any db updates.

@ghost ghost merged commit 003e28e into Grasscutters:development May 17, 2022
@omg-xtao omg-xtao deleted the permission branch May 17, 2022 05:14
This pull request was closed.
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.

None yet

4 participants