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

RBAC deleteUser does not work correctly and is not tested #118

Closed
Sefriol opened this issue Feb 19, 2020 · 9 comments
Closed

RBAC deleteUser does not work correctly and is not tested #118

Sefriol opened this issue Feb 19, 2020 · 9 comments
Assignees
Labels
bug Something isn't working released

Comments

@Sefriol
Copy link
Contributor

Sefriol commented Feb 19, 2020

Coveralls
Source

I think deleteUser should follow similar idea as deleteRole:

public async deleteUser(user: string): Promise<boolean> {
    const res1 = await this.removeFilteredGroupingPolicy(0, user);
    const res2 = await this.removeFilteredPolicy(0, user);
    return res1 || res2;
}

Since we want to remove assigned grouping policy and policies assigned to that user. Now when using mongoose-adapter, it will just remove user grouping policies, leaving normal policies still into the database. With code above, I was able to fix this.

@Sefriol
Copy link
Contributor Author

Sefriol commented Feb 19, 2020

@nodece
Copy link
Member

nodece commented Feb 19, 2020

@hsluoyz what do you think?

@Sefriol
Copy link
Contributor Author

Sefriol commented Feb 19, 2020

I'll create a pull request with RBAC tests and required changes.

@hsluoyz
Copy link
Member

hsluoyz commented Feb 19, 2020

@nodece @Sefriol can you check how Golang's Casbin handle it? I want all Casbin implementations to follow the same behavior on this.

@Sefriol
Copy link
Contributor Author

Sefriol commented Feb 19, 2020

#119

@Sefriol
Copy link
Contributor Author

Sefriol commented Feb 19, 2020

@hsluoyz Is the function properly tested in Golang? Since implementation looks the same, so issue should be the same. Unless the implementation of removeFilteredGroupingPolicy() is different in golang.

However, in golang tests it does not seem like user permission tests are correct.

In line 107 I think alice should have no rights for data1 after she was deleted.

@hsluoyz
Copy link
Member

hsluoyz commented Feb 20, 2020

Hi @Sefriol , I think you are right. Both Golang and Node.js Casbin have wrong implementation. Can you make a PR for Golang too to fix the code and test?

@hsluoyz hsluoyz self-assigned this Feb 20, 2020
@hsluoyz hsluoyz added the bug Something isn't working label Feb 20, 2020
@Sefriol
Copy link
Contributor Author

Sefriol commented Feb 20, 2020

I'll update the PR later on today. Golang does not seem to be that complicated either, but it has been awhile since I worked on Golang-project.

Sefriol added a commit to Sefriol/casbin that referenced this issue Feb 20, 2020
…cies

Previously it just removed Grouping policies. Now it shares same implementation as DeleteRole
casbin/node-casbin#118
nodece pushed a commit that referenced this issue Feb 20, 2020
## [4.1.1](v4.1.0...v4.1.1) (2020-02-20)

### Bug Fixes

* **enforcer.ts:** fix deleteUser and improve deleteRole description ([1e6af16](1e6af16)), closes [#118](#118)
@nodece
Copy link
Member

nodece commented Feb 20, 2020

🎉 This issue has been resolved in version 4.1.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working released
Projects
None yet
Development

No branches or pull requests

3 participants