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

Pass available permissions to getPermissionsForUser #2829

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

fraxachun
Copy link
Contributor

Description

Applications sometimes (in our case most of the time) have two types of users:

  • Admins, which have every permission
  • Users, who have every permission except userPermissions

The problem with the current implementation is that if a permission is added, it also has to be allowlisted in access-control.service.ts. This is often forgotten, leading to unexpected behaviour.

This PR allows to use a Denylist instead of an Allowlist.

Discussion

This might violate the least privilege principle. If we decide against this approach, I think we will need another solution to handle permissions in code (e.g. we start to define them statically again).

Allows to use a Denylist (e.g. everything except User-Permissions)
instead of an Allowlist.
@nsams
Copy link
Member

nsams commented Nov 26, 2024

Can you implement such a Denylist in demo that you think we would use in such applications?

@fraxachun
Copy link
Contributor Author

Can you implement such a Denylist in demo that you think we would use in such applications?

f151ad8

@johnnyomair
Copy link
Collaborator

This might violate the least privilege principle. If we decide against this approach, I think we will need another solution to handle permissions in code (e.g. we start to define them statically again).

@dkarnutsch @max-debug022 what do you think?

@johnnyomair johnnyomair changed the title Pass allowed permissions to getPermissionsForUser Pass available permissions to getPermissionsForUser Nov 28, 2024
@max-debug022
Copy link
Contributor

max-debug022 commented Nov 29, 2024

This might violate the least privilege principle. If we decide against this approach, I think we will need another solution to handle permissions in code (e.g. we start to define them statically again).

@dkarnutsch @max-debug022 what do you think?

I would not recommend allowing a denylist in this matter. I fear it could increase the risk of security vulnerabilities, particularly those related to broken access controls. From a security point of view, it is preferable to wait for users to request additional permissions. As I see it, users are very unlikely to complain if they have more permissions than necessary.

If there is no way around it, I strongly recommend implementing additional safeguards. For example, permissions could be automatically revoked after a certain timeframe (Month, Year, 6 Months, ...) unless explicitly disabled.

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.

4 participants