Skip to content

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 29, 2024

Conversation

code-asher
Copy link
Member

@code-asher code-asher commented Aug 27, 2024

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.

@code-asher code-asher changed the title fix: include suspended and dormant users in acl query fix: include suspended and dormant users in template acl query Aug 27, 2024
@code-asher code-asher force-pushed the asher/template-acl-query-fix branch 2 times, most recently from 4035f9a to 0c9e3ff Compare August 27, 2024 22:57
@code-asher code-asher changed the title fix: include suspended and dormant users in template acl query fix: include dormant users in template acl query Aug 27, 2024
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.
@code-asher code-asher force-pushed the asher/template-acl-query-fix branch from 0c9e3ff to df46098 Compare August 27, 2024 23:00
Role: codersdk.TemplateRoleUse,
})
})

// Test that we do not return suspended users.
t.Run("FilterSuspendedUsers", func(t *testing.T) {
Copy link
Member Author

@code-asher code-asher Aug 27, 2024

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:

  1. 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).
  2. It could cause someone to think a user has no permissions, then if they unsuspend the user "suddenly" they have permissions again.
  3. 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.

Copy link
Member

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.

Copy link
Member Author

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 code-asher marked this pull request as ready for review August 27, 2024 23:23
@code-asher code-asher requested review from sreya and Emyrk August 27, 2024 23:24
@code-asher
Copy link
Member Author

code-asher commented Aug 29, 2024

@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

@Emyrk
Copy link
Member

Emyrk commented Aug 29, 2024

Makes sense if you add a user, they are dormant, but should be available for perms 👍

@code-asher code-asher merged commit ef7fcf3 into main Aug 29, 2024
48 checks passed
@code-asher code-asher deleted the asher/template-acl-query-fix branch August 29, 2024 21:48
@github-actions github-actions bot locked and limited conversation to collaborators Aug 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants