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

ACL rules do not to apply auth->user model, allowing users to make themselves admins #39

Closed
mkrecek234 opened this issue Apr 20, 2020 · 19 comments · Fixed by #115
Closed
Assignees
Labels

Comments

@mkrecek234
Copy link
Contributor

Tried this demo and from my understanding the user "user" should not be allowed to change his own user role according to the ACL entry that Role model is not editable. In fact, he can edit his own rule.

@DarkSide666
Copy link
Member

Can you please provide more info who you setup your ACL?
Also what records do you have in data models?

@DarkSide666 DarkSide666 self-assigned this Apr 20, 2020
@mkrecek234
Copy link
Contributor Author

mkrecek234 commented Apr 20, 2020

I just took the demo as it is with a User Role configured for class \atk4\login\Model\Role with "All Editable" set to no. I thought that this means, that a user belonging to "user role" is not allowed to edit the role "User role" (eg rename it). But in fact a user belonging to "user role" can rename the "user role"

@mkrecek234
Copy link
Contributor Author

Could you see the issue in the demo?

@DarkSide666
Copy link
Member

I will have to try to reproduce that and see if it works. It should, but ... :)

@mkrecek234
Copy link
Contributor Author

mkrecek234 commented May 11, 2020

Any news on this? I retried again: the user is able to change his user group, even though there is the standard rule, that role "User" does not have editable rights.
image

@DarkSide666
Copy link
Member

DarkSide666 commented May 11, 2020

Just an idea - maybe it's using cached (from session) user model and no permissions are set in it.

@mkrecek234
Copy link
Contributor Author

mkrecek234 commented Jun 10, 2020

Cache does not play a role - it also does not work in a private fresh browser window. So ACL as far as I can see is not working. You can easily check just with the atk4/login demo.

DarkSide666 pushed a commit that referenced this issue Jun 19, 2020
* Refactoring to ATK 2.1

* Further refactoring to 2.1, however ACL rules are still not applied, so issue #39 persists.

Co-authored-by: Michael Krecek <krecek@dte-systems.de>
@acicovic
Copy link
Collaborator

Given that In #44 it is stated that this issue is not resolved, @DarkSide666 were you able to reproduce this and do you know if anyone is working on this?

Thanks!

@acicovic
Copy link
Collaborator

Tagging this as major, as it is necessary for many multi-user environments.

@DarkSide666
Copy link
Member

DarkSide666 commented Oct 26, 2020

I will have to fix this sometime very soon if that's the actual issue.
Probably will not try to fix this for atk 2.1 or 2.2 branches. But definitely for atk 2.3 and upcoming branches. I need that myself too.

@mkrecek234
Copy link
Contributor Author

Thank you - FYI back 7 months ago (around that time) it still worked (1.7 ui/data) It then stopped working silently. Hope we can fix this first for 2.3 - however as 2.3 also has still some issues (which 2.2 hasn't) it would be great to have it for 2.2. At least we then have one working set of repos to work on

@DarkSide666
Copy link
Member

I'm planning that this working set will be based on 2.3, but probably it can be backported for 2.2 too afterwards.

@acicovic
Copy link
Collaborator

acicovic commented Jan 9, 2021

@mkrecek234 I see that #66 contains some code related to ACL. If you happen to test it, let us know if things got better.

Thanks.

@mkrecek234
Copy link
Contributor Author

mkrecek234 commented Oct 18, 2021

Hi there, I tested now the latest release for 3.0 and could find what the issue really is with ACL:
For other models, ACL is working properly, BUT:

  • For the \Atk4\Login\User model it is NOT working!

This creates a very strange situation: Each user can just go their preference menu and change the role from user to admin and gain full access rights.

To check, please follow these steps in the included demo:

  1. Go to RoleAdmin and add another rule with model set to Atk4\Login\Model\User, save then.
  2. Edit this rule now and deactivate "All Editable" and list only these editable fields: "name,email,password" and save.
  3. Logout.
  4. Log back in as user / user and click on upper right "User" > Preferences.
  5. You can now edit the Role and change it to "Admin", even though that model should have been restricted by your above rule.

@mvorisek

@mkrecek234 mkrecek234 changed the title ACL rules do not seem to apply ACL rules do not to apply auth->user model, allowing users to make themselves admins Oct 18, 2021
@mvorisek
Copy link
Member

The issue is here: https://github.com/atk4/login/blob/3.0.0/demos/acl-clients.php#L20

ACL needs to be initialized currently manually, if added into demos/admin-users.php, it works.

Of course, this is bad as ACL should be opt-in not opt-out, eg:

  1. we must reject access to every model by default
  2. update default ACLs rules to allow full access for admin and restricted rule for user
  3. with this strict opt-in approach, check if some ACL rule for unlogged/guest user is needed too
  4. then move the App::initAcl call to App::init as developer can forget to add it manually otherwise

@mvorisek mvorisek removed the MAJOR label Oct 19, 2021
@mkrecek234
Copy link
Contributor Author

@mvorisek Confirmed. If you add $this->initAcl(); before this line https://github.com/atk4/login/blob/3.0.0/demos/_includes/App.php#L44
it works as it should 👍 Thank you

@mkrecek234
Copy link
Contributor Author

mkrecek234 commented Oct 26, 2021

Most recent 3.0 and dev-develop do work now. Please note that the model HAS to be called Atk4... - if you call it \Atk4... the ACL rules will not apply. That might be improved for better usability.
Secondly, if you want to restrict Atk4\Login\User fields reflected in the User Preference menu, currently you have to create the Auth object with check => false. Otherwise it will add the User Preference menu before initACL was called, and then users are able to give themselves an admin role.

@mvorisek mvorisek reopened this Oct 26, 2021
@mvorisek
Copy link
Member

this is crutical to work always on opt-out basis and the class names should be compared using reflection, IRRC if class is extended, it does not work currently too

@mvorisek
Copy link
Member

mvorisek commented Dec 5, 2021

matching of extended classes fixed in #84 (now you can specify parent class or interface in model rule field)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants