Aug 29, 2024 · Aug 27, 2024 · Aug 27, 2024 · Aug 29, 2024 · Aug 29, 2024
diff --git a/coderd/database/modelqueries.go b/coderd/database/modelqueries.go
 WHERE
 users.deleted = false
 AND
 users.status = 'active';
 users.status!= 'suspended';
 `

 var tus []TemplateUser
diff --git a/enterprise/coderd/templates_test.go b/enterprise/coderd/templates_test.go
 require.Len(t, acl.Users, 0, "deleted users should be filtered")
 })

 // Test that we do not filter dormant users.
 t.Run("IncludeDormantUsers", func(t *testing.T) {
 t.Parallel()

 client, user := coderdenttest.New(t, &coderdenttest.Options{LicenseOptions: &coderdenttest.LicenseOptions{
 Features: license.Features{
 codersdk.FeatureTemplateRBAC: 1,
 },
 }})
 anotherClient, _ := coderdtest.CreateAnotherUser(t, client, user.OrganizationID, rbac.RoleTemplateAdmin(), rbac.RoleUserAdmin())

 ctx := testutil.Context(t, testutil.WaitLong)

 // nolint:gocritic // Must use owner to create user.
 user1, err := client.CreateUserWithOrgs(ctx, codersdk.CreateUserRequestWithOrgs{
 Email:           "coder@coder.com",
 Username:        "coder",
 Password:        "SomeStrongPassword!",
 OrganizationIDs: []uuid.UUID{user.OrganizationID},
 })
 require.NoError(t, err)
 require.Equal(t, codersdk.UserStatusDormant, user1.Status)
 version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil)
 template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID)

 err = anotherClient.UpdateTemplateACL(ctx, template.ID, codersdk.UpdateTemplateACL{
 UserPerms: map[string]codersdk.TemplateRole{
 user1.ID.String(): codersdk.TemplateRoleUse,
 },
 })
 require.NoError(t, err)

 acl, err := anotherClient.TemplateACL(ctx, template.ID)
 require.NoError(t, err)
 require.Contains(t, acl.Users, codersdk.TemplateUser{
 User: user1,
 Role: codersdk.TemplateRoleUse,
 })
 })

 // Test that we do not return suspended users.
 t.Run("FilterSuspendedUsers", func(t *testing.T) {
 t.Parallel()
Original file line number	Diff line number	Diff line change
Expand Up		@@ -167,7 +167,7 @@ func (q *sqlQuerier) GetTemplateUserRoles(ctx context.Context, id uuid.UUID) ([]
		WHERE
		users.deleted = false
		AND
		users.status = 'active';
		users.status!= 'suspended';
		`

		var tus []TemplateUser
Expand Down
Original file line number	Diff line number	Diff line change
Expand Up		@@ -1025,6 +1025,46 @@ func TestTemplateACL(t *testing.T) {
		require.Len(t, acl.Users, 0, "deleted users should be filtered")
		})

		// Test that we do not filter dormant users.
		t.Run("IncludeDormantUsers", func(t *testing.T) {
		t.Parallel()

		client, user := coderdenttest.New(t, &coderdenttest.Options{LicenseOptions: &coderdenttest.LicenseOptions{
		Features: license.Features{
		codersdk.FeatureTemplateRBAC: 1,
		},
		}})
		anotherClient, _ := coderdtest.CreateAnotherUser(t, client, user.OrganizationID, rbac.RoleTemplateAdmin(), rbac.RoleUserAdmin())

		ctx := testutil.Context(t, testutil.WaitLong)

		// nolint:gocritic // Must use owner to create user.
		user1, err := client.CreateUserWithOrgs(ctx, codersdk.CreateUserRequestWithOrgs{
		Email: "coder@coder.com",
		Username: "coder",
		Password: "SomeStrongPassword!",
		OrganizationIDs: []uuid.UUID{user.OrganizationID},
		})
		require.NoError(t, err)
		require.Equal(t, codersdk.UserStatusDormant, user1.Status)
		version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil)
		template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID)

		err = anotherClient.UpdateTemplateACL(ctx, template.ID, codersdk.UpdateTemplateACL{
		UserPerms: map[string]codersdk.TemplateRole{
		user1.ID.String(): codersdk.TemplateRoleUse,
		},
		})
		require.NoError(t, err)

		acl, err := anotherClient.TemplateACL(ctx, template.ID)
		require.NoError(t, err)
		require.Contains(t, acl.Users, codersdk.TemplateUser{
		User: user1,
		Role: codersdk.TemplateRoleUse,
		})
		})

		// Test that we do not return suspended users.
		t.Run("FilterSuspendedUsers", func(t *testing.T) {
Copy link MemberAuthor code-asherAug 27, 2024• edited Loading 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. Copy link Member EmyrkAug 29, 2024 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 MemberAuthor code-asherAug 29, 2024 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
		t.Parallel()
Expand Down