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

[AC-1124] Restrict admins from accessing items in Collections tab #3676

Merged
merged 17 commits into from
Feb 8, 2024

Conversation

shane-melton
Copy link
Member

Type of change

- [ ] Bug fix
- [X] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

Restrict admins from accessing organization ciphers when the organization collection setting is disabled.

See corresponding client PR: bitwarden/clients#7537

Code changes

src/Api/Vault/Controllers/CiphersController.cs

  • Update /ciphers/organization-details endpoint to always return all collections for admins when flexible collections V1 is enabled. Otherwise, continue with existing behavior.
    • In flexible collections V1, the endpoint will become only available to admin/owners/providers. Others should use the new /ciphers/organization-details/assigned endpoint.
  • Add /ciphers/organization-details/assigned endpoint to return all ciphers the requesting user has access to for an organization. This includes unassigned ciphers for admin/owners/providers.
  • Add access helpers that will be moved to their own service / auth handler in AC-2062

src/Core/Vault/Models/Data/CipherDetails.cs

  • Add new CipherDetailsWithCollections domain model to include both cipher permissions and collectionIds in the new endpoint response. Normally the admin endpoints have excluded the permission info as it was unnecessary and available via sync data. The new /ciphers/organization-details/assigned endpoint is now expected to include this info so the appropriate edit/delete options can be presented to the user in the admin console when admins are restricted from editing ciphers.

src/Core/Vault/Queries/IOrganizationCiphersQuery.csc

  • Introduce the IOrganizationCiphersQuery service to group cipher queries related to a specific organization. This should ultimately replace organizational cipher query methods in the CipherService once flexible collections is fully release.

src/Core/Vault/VaultServiceCollectionExtensions.cs

  • Introduce AddVaultServices service collection extension method to add additional Vault services (currently only the IOrganizationCiphersQuery).

src/Core/Vault/Repositories/ICipherRepository.cs

  • Add new GetManyUnassignedOrganizationDetailsByOrganizationIdAsync method to retrieved organization ciphers that are not assigned to any collections.

EntityFramework/Vault/Repositories/Queries/CipherOrganizationDetailsReadByOrganizationIdQuery.cs

  • Update the EF query to support querying unassigned ciphers only to match the functionality added via the new sproc for Dapper.

src/Sql/Vault/dbo/Stored Procedures/Cipher/CipherOrganizationDetails_ReadUnassignedByOrganizationId.sql

  • New MSSQL sproc for querying unassigned organization ciphers.

Before you submit

  • Please check for formatting errors (dotnet format --verify-no-changes) (required)
  • If making database changes - make sure you also update Entity Framework queries and/or migrations
  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team

Copy link

codecov bot commented Jan 16, 2024

Codecov Report

Attention: 213 lines in your changes are missing coverage. Please review.

Comparison is base (9ecc479) 39.32% compared to head (cdb06f7) 39.17%.

Files Patch % Lines
src/Api/Vault/Controllers/CiphersController.cs 3.94% 73 Missing ⚠️
src/Core/Vault/Models/Data/CipherDetails.cs 0.00% 46 Missing ⚠️
src/Core/Vault/Queries/OrganizationCiphersQuery.cs 0.00% 35 Missing ⚠️
...ore/Vault/Models/Data/CipherOrganizationDetails.cs 0.00% 23 Missing ⚠️
...herOrganizationDetailsReadByOrganizationIdQuery.cs 0.00% 11 Missing ⚠️
...ture.Dapper/Vault/Repositories/CipherRepository.cs 0.00% 9 Missing ⚠️
...c/Api/Vault/Models/Response/CipherResponseModel.cs 0.00% 8 Missing ⚠️
...tyFramework/Vault/Repositories/CipherRepository.cs 0.00% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3676      +/-   ##
==========================================
- Coverage   39.32%   39.17%   -0.15%     
==========================================
  Files        1032     1034       +2     
  Lines       51056    51274     +218     
  Branches     4581     4602      +21     
==========================================
+ Hits        20076    20086      +10     
- Misses      30036    30244     +208     
  Partials      944      944              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Jingo88
Jingo88 previously approved these changes Jan 19, 2024
@bitwarden-bot
Copy link

bitwarden-bot commented Jan 24, 2024

Logo
Checkmarx One – Scan Summary & Detailsd9672b8b-ee42-486b-8ea6-92f87dd5c7ad

No New Or Fixed Issues Found

AS
BEGIN
SET NOCOUNT ON

Copy link
Contributor

@rkac-bw rkac-bw Jan 24, 2024

Choose a reason for hiding this comment

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

It would be more efficient to not use the not in clause and do where null:


SELECT
    C.*,
    CASE
        WHEN O.[UseTotp] = 1 THEN 1
        ELSE 0
    END [OrganizationUseTotp]
FROM
    [dbo].[CipherView] C
LEFT JOIN
    [dbo].[OrganizationView] O ON O.[Id] = C.[OrganizationId]
LEFT JOIN
    [dbo].[CollectionCipher] CC ON C.[Id] = CC.[CipherId]
LEFT JOIN
    [dbo].[Collection] S ON S.[Id] = CC.[CollectionId]
    AND S.[OrganizationId] = C.[OrganizationId]
WHERE
    C.[UserId] IS NULL
    AND C.[OrganizationId] = @OrganizationId
    AND CC.[CipherId] IS NULL

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the suggestion! Updated in f29ea21

rkac-bw
rkac-bw previously approved these changes Feb 6, 2024
Copy link
Contributor

@rkac-bw rkac-bw left a comment

Choose a reason for hiding this comment

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

Looks good to me

Jingo88
Jingo88 previously approved these changes Feb 8, 2024
@shane-melton shane-melton dismissed stale reviews from Jingo88 and rkac-bw via cdb06f7 February 8, 2024 20:46
Copy link
Contributor

@rkac-bw rkac-bw left a comment

Choose a reason for hiding this comment

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

Looks good to me

@shane-melton shane-melton merged commit 636f716 into main Feb 8, 2024
48 of 50 checks passed
@shane-melton shane-melton deleted the vault/ac-1124/restrict-admin-access-to-items branch February 8, 2024 22:07
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.

None yet

5 participants