[PM-11249] Update cipher revision date when an attachment is added or deleted

[nick-livefront]

🎟️ Tracking

PM-11249

📔 Objective

Current Issue: When an attachment is added or deleted, web/browser clients are not being updated of the change.

Cause: The logic within the CoreSyncService on the client side uses the cipher revision date to determine if a sync is needed. Server side the cipher revision date isn't updated.

Proposed Fix:

• Update the revision date on the cipher if an attachment is added or deleted and store in the DB
• In order to support this, the delete attachment endpoint needs to return the cipher as clients will need to know of the new revision date on the cipher.
• client PR for the parallel changes to support this.

📸 Screenshots

Before	After
attachments-before.mov	attachments-after.mov

🦮 Reviewer guidelines

• 👍 (:+1:) or similar for great changes
• 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
• ❓ (:question:) for questions
• 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
• 🎨 (:art:) for suggestions / improvements
• ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
• 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
• ⛏ (:pick:) for minor or nitpick changes

[shane-melton]

❓ Are you still able to modify the cipher after uploading an attachment? I'm worried that we don't return the updated cipher in the attachment endpoint so the cipher.revisionDate will be stale client side, which will prevent users from making changes until they manually re-sync or refresh their vault (on the client that uploaded the attachment).

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 15 lines in your changes missing coverage. Please review.

Project coverage is 44.31%. Comparing base (a9a1230) to head (72fe323).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...re/Vault/Services/Implementations/CipherService.cs	0.00%	9 Missing ⚠️
...e/Vault/Models/Data/DeleteAttachmentReponseData.cs	0.00%	5 Missing ⚠️
src/Api/Vault/Controllers/CiphersController.cs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4873      +/-   ##
==========================================
- Coverage   44.32%   44.31%   -0.01%     
==========================================
  Files        1482     1483       +1     
  Lines       68376    68388      +12     
  Branches     6172     6172              
==========================================
  Hits        30307    30307              
- Misses      36761    36773      +12     
  Partials     1308     1308              

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

[nick-livefront]

❓ Are you still able to modify the cipher after uploading an attachment? I'm worried that we don't return the updated cipher in the attachment endpoint so the cipher.revisionDate will be stale client side, which will prevent users from making changes until they manually re-sync or refresh their vault (on the client that uploaded the attachment).

Nice catch, I only tested on the client that received the update which in turn would have the updated cipher.

On the same client you are not able to. 🤔

Screen.Recording.2024-10-09.at.1.32.52.PM.mov

[github-actions]

Checkmarx One – Scan Summary & Details – fcd6880d-cbd0-45e0-a445-4bb8ac2be417

New Issues (12)

Checkmarx found the following issues in this Pull Request

Severity	Issue	Source File / Package	Checkmarx Insight
	CSRF	/src/Billing/Controllers/PayPalController.cs: 66	details Method PostIpn at line 66 of /src/Billing/Controllers/PayPalController.cs gets a parameter from a user request from Body. This parameter value flow... Attack Vector
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1100	details Method DeleteAttachment at line 1100 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This paramete... Attack Vector
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1100	details Method DeleteAttachment at line 1100 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This paramete... Attack Vector
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1100	details Method DeleteAttachment at line 1100 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from DeleteAttachment.... Attack Vector
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1023	details Method PostAttachment at line 1023 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This parameter ... Attack Vector
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 997	details Method PostFileForExistingAttachment at line 997 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. T... Attack Vector
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1046	details Method PostAttachmentAdmin at line 1046 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This param... Attack Vector
	Privacy_Violation	/src/Core/AdminConsole/OrganizationAuth/UpdateOrganizationAuthRequestCommand.cs: 84	details Method UpdateAsync at line 84 of /src/Core/AdminConsole/OrganizationAuth/UpdateOrganizationAuthRequestCommand.cs sends user information outside the... Attack Vector
	Privacy_Violation	/src/Core/NotificationHub/NotificationHubPushNotificationService.cs: 195	details Method PushAuthRequestAsync at line 195 of /src/Core/NotificationHub/NotificationHubPushNotificationService.cs sends user information outside the a... Attack Vector
	Log_Forging	/src/Api/Auth/Controllers/AuthRequestsController.cs: 87	details Method PostAdminRequest at line 87 of /src/Api/Auth/Controllers/AuthRequestsController.cs gets user input from element model. This element’s value ... Attack Vector
	Log_Forging	/src/Api/Auth/Controllers/AuthRequestsController.cs: 75	details Method Post at line 75 of /src/Api/Auth/Controllers/AuthRequestsController.cs gets user input from element model. This element’s value flows throug... Attack Vector
	Missing_CSP_Header	/src/Core/MailTemplates/Handlebars/AdminConsole/DomainClaimedByOrganization.html.hbs: 7	details A Content Security Policy is not explicitly defined within the web-application. Attack Vector

Fixed Issues (28)

Great job! The following issues were fixed in this Pull Request

Severity	Issue	Source File / Package
	CSRF	/src/Api/Tools/Controllers/OrganizationExportController.cs: 53
	CSRF	/src/Admin/AdminConsole/Controllers/OrganizationsController.cs: 375
	CSRF	/src/Api/AdminConsole/Controllers/GroupsController.cs: 80
	CSRF	/src/Api/AdminConsole/Controllers/GroupsController.cs: 121
	CSRF	/src/Api/AdminConsole/Public/Controllers/PoliciesController.cs: 46
	CSRF	/src/Api/AdminConsole/Public/Controllers/PoliciesController.cs: 65
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 470
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 220
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 220
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1100
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 173
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 533
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 533
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1100
	Privacy_Violation	/src/Api/Auth/Models/Request/Accounts/SetPasswordRequestModel.cs: 28
	Log_Forging	/src/Api/Billing/Controllers/OrganizationBillingController.cs: 238
	Log_Forging	/src/Api/Billing/Controllers/OrganizationBillingController.cs: 238
	Log_Forging	/src/Api/Billing/Controllers/OrganizationBillingController.cs: 238
	Log_Forging	/src/Api/Billing/Controllers/ProviderBillingController.cs: 104
	Log_Forging	/src/Api/Billing/Controllers/ProviderBillingController.cs: 104
	Log_Forging	/src/Api/Billing/Controllers/ProviderBillingController.cs: 104
	Log_Forging	/src/Billing/Controllers/RecoveryController.cs: 38
	Log_Forging	/src/Billing/Controllers/RecoveryController.cs: 38
	Log_Forging	/src/Api/Vault/Controllers/CiphersController.cs: 173
	Log_Forging	/src/Billing/Controllers/StripeController.cs: 164
	Log_Forging	/src/Billing/Controllers/StripeController.cs: 164
	Log_Forging	/src/Api/Auth/Controllers/AccountsController.cs: 261
	Log_Forging	/src/Api/Auth/Controllers/AccountsController.cs: 381

[nick-livefront]

Marking this a draft as I have yet to track down all of the necessary changes on server and client side. Priorities have changed so I'll be taking up some other work in the mean time.

See the ticket for more details about solutioning these scenarios.

[nick-livefront]

@shane-melton I order to support this change for deleting an attachment, the UI needed to know the updated revision timestamp. I added logic to return the updated cipher to both the client and here 13acab5

[gbubemismith]

Changes look good

[nick-livefront]

Moving to a draft as this work is being tabled for a couple weeks why we wait for the extension refresh to roll out.