- Notifications
You must be signed in to change notification settings - Fork927
fix: include dormant users in template acl query#14461
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
4035f9a to0c9e3ffCompareThe issue is that if you add a user and then immediately go to give thempermissions, you can add them but they will not show up in the UI. Theyalso do not show up in the audit log entry.
0c9e3ff todf46098Compare| Role: codersdk.TemplateRoleUse, | ||
| }) | ||
| }) | ||
| // Test that we do not return suspended users. | ||
| t.Run("FilterSuspendedUsers", func(t *testing.T) { |
code-asherAug 27, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should we should include suspended users as well? Since the UI is kind of lying about the permissions if a suspended user is there, whether they were added after being suspended or became suspended after the fact. It looks like we explicitly wanted to filter out suspended users so I wanted to hold off to validate if that makes sense first.
Reasoning being:
- You can add a suspended user, but then it will just not show up which is confusing (and it does not record in the audit log).
- It could cause someone to think a user has no permissions, then if they unsuspend the user "suddenly" they have permissions again.
- Someone might know a suspended user had permissions, but when they go to check they cannot see the user and cannot remove the permissions.
But, I am not completely sure about the workflow/use case around suspension so these concerns might be invalid.
Alternatively we could prevent adding suspended users in the first place, and remove all their permissions when they become suspended. More work, but depending on what suspension is meant to be maybe it makes more sense to do it that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The UX story around who to show is unclear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Opened#14486 so we can figure this out at some point
code-asher commentedAug 29, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@Emyrk sorry to keep requesting you on everything haha, I think Jon is out for a bit and I know you have some context on rbac and permissions and such so thought you might have the right insight <3 |
Makes sense if you add a user, they are dormant, but should be available for perms 👍 |
ef7fcf3 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
The issue is that if you add a user and then immediately go to give them
permissions, you can add them but they will not show up in the UI. They
also do not show up in the audit log entry.