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

GroupOptions - PermissionCheck not working properly #1324

Closed
Darlelet opened this issue Sep 2, 2017 · 13 comments
Closed

GroupOptions - PermissionCheck not working properly #1324

Darlelet opened this issue Sep 2, 2017 · 13 comments
Labels
Good first issue Marks issues that should be easy to fix/close, useful for new contributors. Type: bug
Milestone

Comments

@Darlelet
Copy link

Darlelet commented Sep 2, 2017

What behaviour is observed:

When option GroupOptions->enablePermissionCheck is enabled through config file, as soon as the user connects his parent group is set to registeredPlayerGroup, using world context.

What behaviour is expected:

The parent group should be set "globally", without using any context, so that this feature is fully compatible with existing permissions plugins.

Steps/models to reproduce:

Using LuckPerms: define a specific parent for an user (lp user Player parent set Group), then display his parents (lp user Player parent info), you should be seeing ~> Parent Groups: Group
Then disconnect and log back in through AuthMe ~> Parent Groups: Group world=worldname, Group

(This could be a potential security issue if you want to demote that user later as the statement could be persistent if the permissions plugin only overwrites "global" parent group)

Plugin list:

As the issue has been narrowed down to AuthMe', the following list seems relevant:

Vault, LuckPerms, AuthMeReloaded

Environment description

Spigot / Paper Spigot; SQLITE driver

AuthMe build number:

5.4-SNAPSHOT-b1760

Error Log:

None

Configuration:

Already exposed

Related issues - #1265, LuckPerms/LuckPerms#294

@sgdc3 sgdc3 added Good first issue Marks issues that should be easy to fix/close, useful for new contributors. Type: enhancement labels Sep 2, 2017
@sgdc3 sgdc3 closed this as completed in 72da27e Sep 2, 2017
@sgdc3 sgdc3 reopened this Sep 2, 2017
@sgdc3
Copy link
Member

sgdc3 commented Sep 2, 2017

@Darlelet could you try last dev build please? ;)

@Darlelet
Copy link
Author

Darlelet commented Sep 5, 2017

@sgdc3 Regarding the (world) context issue this seems to be fixed indeed! 😉

However I noticed two strange behaviors:

  • It happens that the temporary parent is not set at all (and user keeps his base group). I didn't manage to reproduce on purpose, but it happened a few times during the testing phase.

  • If you connect to server, but don't log in, and then disconnect and reconnect immediately (quickly), it also happens that the user looses his group, and by loosing I mean belongs to neither base group nor registeredPlayerGroup. LuckPerms seems to prevent user from being "groupless" (orphan) so I guess it sets back user to "default" parent group when none is assigned (this seems to be the case here).

I think this misbehavior could also be found in earlier dev builds (considering the minor changes you made to fix #1324).

What do you think?

@sgdc3
Copy link
Member

sgdc3 commented Sep 5, 2017

I noticed LuckPerms handles very bad operations on offline players... we should write a proper luckperms integration that directly uses the api, not thru Vault

sgdc3 added a commit that referenced this issue Sep 14, 2017
@sgdc3
Copy link
Member

sgdc3 commented Sep 14, 2017

@Darlelet Could you try with the latest dev build? ;)

@DisTrugator
Copy link

Latest dev build still have this error on PaperSpigot 1.12.2

@sgdc3
Copy link
Member

sgdc3 commented Sep 20, 2017

@DisTrugator Are you using the latest LuckPerms dev build?

@Darlelet
Copy link
Author

Darlelet commented Sep 20, 2017

Sorry for not being the quick-reply guy here 😞, quite busy last few days...

Never mind, just tried out latest Authme build, using LP 3.3.19.

Here's what I noticed:

  • This version definitely seems to fix the previous issue causing a player to lose his group.
    I tried a dozen times, didn't happened, the feature is working as expected. 👍
  • On the other hand this seems to bring in a new (minor hopefully) bug / conflict with luckperms:
    When LP tries to load specific player's permissions during Authme's authenticating phase, this occurs occasionally:
    image
    Here is server's log right before the player gets kicked out ~>
[21:08:07 INFO]: UUID of player XX is xxx
[21:08:07 INFO]: [LuckPerms] [WARN] User xxx - XX doesn't have data pre-loaded. - denying login.

The good thing is that nothing gets lost / buggy tho. Simultaneous operations on the same player simply appear to be unhandled. Who's fault is it to you?

Keep up the good work! 😉

@sgdc3
Copy link
Member

sgdc3 commented Sep 20, 2017

@Darlelet This has been fixed by luckperms in the latest dev builds: https://ci.lucko.me/job/LuckPerms/334/

;)

@sgdc3
Copy link
Member

sgdc3 commented Sep 20, 2017

@Darlelet Check it out and let us know if that fixed your issue

@Darlelet
Copy link
Author

Darlelet commented Sep 20, 2017 via email

@sgdc3
Copy link
Member

sgdc3 commented Sep 30, 2017

@Darlelet Any news?

@Darlelet
Copy link
Author

@sgdc3 LGTM (15+ attempts or so), good job! 🎉
~ Using latest LP (Build 341)

I guess I should close the issue then? ;p

@sgdc3
Copy link
Member

sgdc3 commented Sep 30, 2017

Thank you for the feedback.

@sgdc3 sgdc3 closed this as completed Sep 30, 2017
@ljacqu ljacqu added this to the 5.4 milestone Sep 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good first issue Marks issues that should be easy to fix/close, useful for new contributors. Type: bug
Development

No branches or pull requests

4 participants