Skip to content

[PM-42318] Add the collection reads behind PAM's access rules #8243

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.

Sign up for GitHub

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

Open
Hinton wants to merge 6 commits into
base: main
Choose a base branch
from
+948 −35
Open

[PM-42318] Add the collection reads behind PAM's access rules #8243

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3642d1a
Read the users who can manage a collection
Hinton Aug 21, 2026
567860e
Tell clients which collections a PAM access rule governs
Hinton Aug 20, 2026
c8d0cf7
Ship the collection reads in one dated migration
Hinton Aug 21, 2026
864e2d2
Resolve a collection's managers in the database
Hinton Aug 25, 2026
80d2401
Drop the access-rule notes from the collection read procedures
Hinton Aug 25, 2026
2f88669
Re-date the collection reads migration to the day it lands
Hinton Aug 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions src/Api/AdminConsole/Models/Response/CollectionResponseModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,18 @@ public CollectionDetailsResponseModel(CollectionDetails collectionDetails)
HidePasswords = collectionDetails.HidePasswords;
Manage = collectionDetails.Manage;
DefaultUserCollectionEmail = collectionDetails.DefaultUserCollectionEmail;
HasEnabledAccessRule = collectionDetails.HasEnabledAccessRule;
}

public bool ReadOnly { get; set; }
public bool HidePasswords { get; set; }
public bool Manage { get; set; }

/// <summary>
/// True if the collection is governed by an access rule that is currently enabled. Lets a client
/// mark the collection as privileged without reading the organization's access rules.
/// </summary>
public bool HasEnabledAccessRule { get; set; }
}

public class CollectionAccessDetailsResponseModel : CollectionResponseModel
Expand Down Expand Up @@ -100,6 +107,7 @@ public CollectionAccessDetailsResponseModel(CollectionAdminDetails collection)
HidePasswords = collection.HidePasswords;
Manage = collection.Manage;
Unmanaged = collection.Unmanaged;
HasEnabledAccessRule = collection.HasEnabledAccessRule;
Groups = collection.Groups?.Select(g => new SelectionReadOnlyResponseModel(g)) ?? Enumerable.Empty<SelectionReadOnlyResponseModel>();
Users = collection.Users?.Select(g => new SelectionReadOnlyResponseModel(g)) ?? Enumerable.Empty<SelectionReadOnlyResponseModel>();
}
Expand All @@ -116,4 +124,16 @@ public CollectionAccessDetailsResponseModel(CollectionAdminDetails collection)
public bool HidePasswords { get; set; }
public bool Manage { get; set; }
public bool Unmanaged { get; set; }

/// <summary>
/// True if the collection is governed by an access rule that is currently enabled. Lets a client
/// mark the collection as privileged without reading the organization's access rules.
/// </summary>
/// <remarks>
/// Only the <see cref="CollectionAdminDetails"/> constructor can populate this — it is computed by
/// the collection read paths. The bare <see cref="Collection"/> constructors are used for the
/// create/update responses and the provider fallbacks, which have no rule state to report and so
/// leave it false.
/// </remarks>
public bool HasEnabledAccessRule { get; set; }
}
11 changes: 11 additions & 0 deletions src/Core/AdminConsole/Models/Data/CollectionDetails.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,15 @@ public class CollectionDetails : Collection
/// deleting it, and assigning access for other users and groups.
/// </summary>
public bool Manage { get; set; }
/// <summary>
/// If true, the collection is governed by an <c>AccessRule</c> that is currently enabled, so
/// items in it are gated behind PAM leasing.
/// </summary>
/// <remarks>
/// Derived, not stored: <see cref="Collection.AccessRuleId"/> records the association, but a
/// disabled rule gates nothing, so the read paths join the rule and report whether it is switched
/// on. Computed by the collection read procedures and their Entity Framework equivalents; a
/// <see cref="Collection"/> loaded outside those paths cannot tell you this.
/// </remarks>
public bool HasEnabledAccessRule { get; set; }
}
8 changes: 8 additions & 0 deletions src/Core/AdminConsole/Repositories/ICollectionRepository.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,14 @@ public interface ICollectionRepository : IRepository<Collection, Guid>
Task DeleteUserAsync(Guid collectionId, Guid organizationUserId);
Task UpdateUsersAsync(Guid id, IEnumerable<CollectionAccessSelection> users);
Task<ICollection<CollectionAccessSelection>> GetManyUsersByIdAsync(Guid id);

/// <summary>
/// Returns the distinct user ids of every confirmed member who can Manage the collection: direct Manage
/// assignments, Manage via group, org Owners/Admins (when the organization allows admin access to all collection
/// items), and Custom users with the EditAnyCollection permission.
/// </summary>
Task<ICollection<Guid>> GetManagingUserIdsAsync(Guid collectionId);

Task DeleteManyAsync(IEnumerable<Guid> collectionIds);

/// <summary>
Expand Down
13 changes: 13 additions & 0 deletions src/Infrastructure.Dapper/AdminConsole/Repositories/CollectionRepository.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,6 +398,19 @@ public async Task<ICollection<CollectionAccessSelection>> GetManyUsersByIdAsync(
}
}

public async Task<ICollection<Guid>> GetManagingUserIdsAsync(Guid collectionId)
{
using (var connection = new SqlConnection(ConnectionString))
{
var results = await connection.QueryAsync<Guid>(
$"[{Schema}].[Collection_ReadManagingUserIds]",
new { CollectionId = collectionId },
commandType: CommandType.StoredProcedure);

return results.ToList();
}
}

public async Task CreateDefaultCollectionsAsync(Guid organizationId, IEnumerable<Guid> organizationUserIds, string defaultCollectionName)
{
organizationUserIds = organizationUserIds.ToList();
Expand Down
87 changes: 77 additions & 10 deletions src/Infrastructure.EntityFramework/AdminConsole/Repositories/CollectionRepository.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
using Bit.Core.Enums;
using Bit.Core.Models.Data;
using Bit.Core.Repositories;
using Bit.Core.Utilities;
using Bit.Infrastructure.EntityFramework.AdminConsole.Models;
using Bit.Infrastructure.EntityFramework.AdminConsole.Repositories.Queries;
using Bit.Infrastructure.EntityFramework.Repositories;
Expand Down Expand Up @@ -259,7 +260,8 @@ public async Task<ICollection<CollectionDetails>> GetManyByUserIdAsync(Guid user
c.CreationDate,
c.RevisionDate,
c.ExternalId,
c.Type
c.Type,
c.HasEnabledAccessRule
})
.Select(collectionGroup => new CollectionDetails
{
Expand All @@ -273,6 +275,7 @@ public async Task<ICollection<CollectionDetails>> GetManyByUserIdAsync(Guid user
HidePasswords = Convert.ToBoolean(collectionGroup.Min(c => Convert.ToInt32(c.HidePasswords))),
Manage = Convert.ToBoolean(collectionGroup.Max(c => Convert.ToInt32(c.Manage))),
Type = collectionGroup.Key.Type,
HasEnabledAccessRule = collectionGroup.Key.HasEnabledAccessRule,
})
.ToList();
}
Expand All @@ -286,7 +289,8 @@ public async Task<ICollection<CollectionDetails>> GetManyByUserIdAsync(Guid user
c.CreationDate,
c.RevisionDate,
c.ExternalId,
c.Type
c.Type,
c.HasEnabledAccessRule
} into collectionGroup
select new CollectionDetails
{
Expand All @@ -300,6 +304,7 @@ public async Task<ICollection<CollectionDetails>> GetManyByUserIdAsync(Guid user
HidePasswords = Convert.ToBoolean(collectionGroup.Min(c => Convert.ToInt32(c.HidePasswords))),
Manage = Convert.ToBoolean(collectionGroup.Max(c => Convert.ToInt32(c.Manage))),
Type = collectionGroup.Key.Type,
HasEnabledAccessRule = collectionGroup.Key.HasEnabledAccessRule,
}).ToListAsync();
}
}
Expand Down Expand Up @@ -327,7 +332,8 @@ public async Task<ICollection<CollectionAdminDetails>> GetManySharedByOrganizati
c.RevisionDate,
c.ExternalId,
c.Unmanaged,
c.DefaultUserCollectionEmail
c.DefaultUserCollectionEmail,
c.HasEnabledAccessRule
}).Select(collectionGroup => new CollectionAdminDetails
{
Id = collectionGroup.Key.Id,
Expand All @@ -342,7 +348,8 @@ public async Task<ICollection<CollectionAdminDetails>> GetManySharedByOrganizati
Manage = Convert.ToBoolean(collectionGroup.Max(c => Convert.ToInt32(c.Manage))),
Assigned = Convert.ToBoolean(collectionGroup.Max(c => Convert.ToInt32(c.Assigned))),
Unmanaged = collectionGroup.Key.Unmanaged,
DefaultUserCollectionEmail = collectionGroup.Key.DefaultUserCollectionEmail
DefaultUserCollectionEmail = collectionGroup.Key.DefaultUserCollectionEmail,
HasEnabledAccessRule = collectionGroup.Key.HasEnabledAccessRule
}).ToList();
}
else
Expand All @@ -357,7 +364,8 @@ public async Task<ICollection<CollectionAdminDetails>> GetManySharedByOrganizati
c.RevisionDate,
c.ExternalId,
c.Unmanaged,
c.DefaultUserCollectionEmail
c.DefaultUserCollectionEmail,
c.HasEnabledAccessRule
}
into collectionGroup
select new CollectionAdminDetails
Expand All @@ -374,7 +382,8 @@ into collectionGroup
Manage = Convert.ToBoolean(collectionGroup.Max(c => Convert.ToInt32(c.Manage))),
Assigned = Convert.ToBoolean(collectionGroup.Max(c => Convert.ToInt32(c.Assigned))),
Unmanaged = collectionGroup.Key.Unmanaged,
DefaultUserCollectionEmail = collectionGroup.Key.DefaultUserCollectionEmail
DefaultUserCollectionEmail = collectionGroup.Key.DefaultUserCollectionEmail,
HasEnabledAccessRule = collectionGroup.Key.HasEnabledAccessRule
}).ToListAsync();
}

Expand Down Expand Up @@ -440,7 +449,8 @@ group cu by cu.CollectionId into u
c.Name,
c.CreationDate,
c.RevisionDate,
c.ExternalId
c.ExternalId,
c.HasEnabledAccessRule
}).Select(collectionGroup => new CollectionAdminDetails
{
Id = collectionGroup.Key.Id,
Expand All @@ -454,7 +464,8 @@ group cu by cu.CollectionId into u
Convert.ToBoolean(collectionGroup.Min(c => Convert.ToInt32(c.HidePasswords))),
Manage = Convert.ToBoolean(collectionGroup.Max(c => Convert.ToInt32(c.Manage))),
Assigned = Convert.ToBoolean(collectionGroup.Max(c => Convert.ToInt32(c.Assigned))),
Unmanaged = collectionGroup.Select(c => c.Unmanaged).FirstOrDefault()
Unmanaged = collectionGroup.Select(c => c.Unmanaged).FirstOrDefault(),
HasEnabledAccessRule = collectionGroup.Key.HasEnabledAccessRule
}).FirstOrDefault();
}
else
Expand All @@ -467,7 +478,8 @@ group cu by cu.CollectionId into u
c.Name,
c.CreationDate,
c.RevisionDate,
c.ExternalId
c.ExternalId,
c.HasEnabledAccessRule
}
into collectionGroup
select new CollectionAdminDetails
Expand All @@ -483,7 +495,8 @@ into collectionGroup
Convert.ToBoolean(collectionGroup.Min(c => Convert.ToInt32(c.HidePasswords))),
Manage = Convert.ToBoolean(collectionGroup.Max(c => Convert.ToInt32(c.Manage))),
Assigned = Convert.ToBoolean(collectionGroup.Max(c => Convert.ToInt32(c.Assigned))),
Unmanaged = collectionGroup.Select(c => c.Unmanaged).FirstOrDefault()
Unmanaged = collectionGroup.Select(c => c.Unmanaged).FirstOrDefault(),
HasEnabledAccessRule = collectionGroup.Key.HasEnabledAccessRule
}).FirstOrDefaultAsync();
}

Expand Down Expand Up @@ -538,6 +551,60 @@ public async Task<ICollection<CollectionAccessSelection>> GetManyUsersByIdAsync(
}
}

public async Task<ICollection<Guid>> GetManagingUserIdsAsync(Guid collectionId)
{
using (var scope = ServiceScopeFactory.CreateScope())
{
var dbContext = GetDatabaseContext(scope);

var directManageUserIds = from cu in dbContext.CollectionUsers
where cu.CollectionId == collectionId && cu.Manage
join ou in dbContext.OrganizationUsers on cu.OrganizationUserId equals ou.Id
where ou.Status == OrganizationUserStatusType.Confirmed && ou.UserId != null
select ou.UserId!.Value;

var groupManageUserIds = from cg in dbContext.CollectionGroups
where cg.CollectionId == collectionId && cg.Manage
join gu in dbContext.GroupUsers on cg.GroupId equals gu.GroupId
join ou in dbContext.OrganizationUsers on gu.OrganizationUserId equals ou.Id
where ou.Status == OrganizationUserStatusType.Confirmed && ou.UserId != null
select ou.UserId!.Value;

var adminManageUserIds = from c in dbContext.Collections
where c.Id == collectionId
join o in dbContext.Organizations on c.OrganizationId equals o.Id
join ou in dbContext.OrganizationUsers on c.OrganizationId equals ou.OrganizationId
where ou.Status == OrganizationUserStatusType.Confirmed
&& ou.UserId != null
&& (ou.Type == OrganizationUserType.Owner
|| ou.Type == OrganizationUserType.Admin)
&& o.AllowAdminAccessToAllCollectionItems
select ou.UserId!.Value;

var managerUserIds = await directManageUserIds
.Union(groupManageUserIds)
.Union(adminManageUserIds)
.ToListAsync();

@r-tome r-tome Aug 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is loading all confirmed members from the database and then filters everything in memory. This could be more efficient we filtered it all in the database. We can use IQueryable to build the queries. For example:

var confirmed = dbContext.OrganizationUsers
  .Where(ou => ou.OrganizationId == organizationId
      && ou.Status == OrganizationUserStatusType.Confirmed
      && ou.UserId != null);
var userIdsWithDirectManage = confirmed
      .Where(ou => dbContext.CollectionUsers.Any(cu =>
          cu.CollectionId == collectionId && cu.Manage && cu.OrganizationUserId == ou.Id))
      .Select(ou => ou.UserId!.Value);
  var userIdsWithGroupManage = confirmed
      .Where(ou => dbContext.GroupUsers.Any(gu => gu.OrganizationUserId == ou.Id
          && dbContext.CollectionGroups.Any(cg =>
              cg.CollectionId == collectionId && cg.Manage && cg.GroupId == gu.GroupId)))
      .Select(ou => ou.UserId!.Value);
var managerUserIds = await userIdsWithDirectManage.Union(userIdsWithGroupManage).ToListAsync();


// TODO: Update to JSON query after upgrading to EF 10.
var customMembers = await (from c in dbContext.Collections
where c.Id == collectionId
join ou in dbContext.OrganizationUsers on c.OrganizationId equals ou.OrganizationId
where ou.Status == OrganizationUserStatusType.Confirmed
&& ou.UserId != null
&& ou.Type == OrganizationUserType.Custom
select new { UserId = ou.UserId!.Value, ou.Permissions })
.ToListAsync();

return managerUserIds
.Concat(customMembers
.Where(m => CoreHelpers.LoadClassFromJsonData<Permissions>(m.Permissions).EditAnyCollection)
.Select(m => m.UserId))
.Distinct()
.ToList();
}
}

public async Task ReplaceAsync(Core.Entities.Collection collection, IEnumerable<CollectionAccessSelection>? groups,
IEnumerable<CollectionAccessSelection>? users)
{
Expand Down
2 changes: 2 additions & 0 deletions ...tructure.EntityFramework/AdminConsole/Repositories/Queries/CollectionAdminDetailsQuery.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,8 @@ where cg.Manage
Manage = (bool?)x.cu.Manage ?? (bool?)x.cg.Manage ?? false,
Assigned = x.cu != null || x.cg != null,
Unmanaged = !activeUserManageRights.Contains(x.c.Id) && !activeGroupManageRights.Contains(x.c.Id),
// A disabled rule gates nothing, so the association alone is not enough.
HasEnabledAccessRule = dbContext.AccessRules.Any(ar => ar.Id == x.c.AccessRuleId && ar.Enabled),
});
}

Expand Down
4 changes: 3 additions & 1 deletion ...structure.EntityFramework/AdminConsole/Repositories/Queries/UserCollectionDetailsQuery.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,9 @@ from cg in cg_g.DefaultIfEmpty()
ReadOnly = (bool?)row.cu.ReadOnly ?? (bool?)row.cg.ReadOnly ?? false,
HidePasswords = (bool?)row.cu.HidePasswords ?? (bool?)row.cg.HidePasswords ?? false,
Manage = (bool?)row.cu.Manage ?? (bool?)row.cg.Manage ?? false,
Type = row.c.Type
Type = row.c.Type,
// A disabled rule gates nothing, so the association alone is not enough.
HasEnabledAccessRule = dbContext.AccessRules.Any(ar => ar.Id == row.c.AccessRuleId && ar.Enabled)
});
}
}
5 changes: 4 additions & 1 deletion src/Sql/dbo/AdminConsole/Stored Procedures/Collection_ReadByIdWithPermissions.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,8 @@ BEGIN
)
THEN 1
ELSE 0
END AS [Unmanaged]
END AS [Unmanaged],
MAX(CASE WHEN AR.[Enabled] = 1 THEN 1 ELSE 0 END) AS [HasEnabledAccessRule]
FROM
[dbo].[CollectionView] C
LEFT JOIN
Expand All @@ -65,6 +66,8 @@ BEGIN
[dbo].[Group] G ON G.[Id] = GU.[GroupId]
LEFT JOIN
[dbo].[CollectionGroup] CG ON CG.[CollectionId] = C.[Id] AND CG.[GroupId] = GU.[GroupId]
LEFT JOIN
[dbo].[AccessRule] AR ON AR.[Id] = C.[AccessRuleId]
WHERE
C.[Id] = @CollectionId
GROUP BY
Expand Down
47 changes: 25 additions & 22 deletions src/Sql/dbo/AdminConsole/Stored Procedures/Collection_ReadByUserId.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,28 +5,31 @@ BEGIN
SET NOCOUNT ON

SELECT
Id,
OrganizationId,
[Name],
CreationDate,
RevisionDate,
ExternalId,
MIN([ReadOnly]) AS [ReadOnly],
MIN([HidePasswords]) AS [HidePasswords],
MAX([Manage]) AS [Manage],
[DefaultUserCollectionEmail],
[Type],
[AccessRuleId]
UCD.[Id],
UCD.[OrganizationId],
UCD.[Name],
UCD.[CreationDate],
UCD.[RevisionDate],
UCD.[ExternalId],
MIN(UCD.[ReadOnly]) AS [ReadOnly],
MIN(UCD.[HidePasswords]) AS [HidePasswords],
MAX(UCD.[Manage]) AS [Manage],
UCD.[DefaultUserCollectionEmail],
UCD.[Type],
UCD.[AccessRuleId],
MAX(CASE WHEN AR.[Enabled] = 1 THEN 1 ELSE 0 END) AS [HasEnabledAccessRule]
FROM
[dbo].[UserCollectionDetails](@UserId)
[dbo].[UserCollectionDetails](@UserId) UCD
LEFT JOIN
[dbo].[AccessRule] AR ON AR.[Id] = UCD.[AccessRuleId]
GROUP BY
Id,
OrganizationId,
[Name],
CreationDate,
RevisionDate,
ExternalId,
[DefaultUserCollectionEmail],
[Type],
[AccessRuleId]
UCD.[Id],
UCD.[OrganizationId],
UCD.[Name],
UCD.[CreationDate],
UCD.[RevisionDate],
UCD.[ExternalId],
UCD.[DefaultUserCollectionEmail],
UCD.[Type],
UCD.[AccessRuleId]
END
Loading
Loading