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

[PM-11162] Assign to Collection Permission Update #4844

Merged
merged 31 commits into from
Feb 4, 2025

Conversation

Jingo88
Copy link
Contributor

@Jingo88 Jingo88 commented Oct 2, 2024

🎟️ Tracking

PM-11162

📔 Objective

Users with Can Edit Except Password will not be allowed to assign collections to ciphers.

Updates made to CipherDetails_ReadByIdUserId.sql to get accurate ViewPassword value when user assigns/unassigns collections (This addresses bug PM-14046)

NOTE: EntityFramework update being looked at later on.

📸 Screenshots

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 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

@Jingo88 Jingo88 requested a review from a team as a code owner October 2, 2024 20:27
Copy link
Contributor

github-actions bot commented Oct 2, 2024

Logo
Checkmarx One – Scan Summary & Details4d56d510-341b-4fe1-ac6e-5943eff9b236

New Issues (171)

Checkmarx found the following issues in this Pull Request

Severity Issue Source File / Package Checkmarx Insight
HIGH Reflected_XSS_All_Clients /test/Identity.IntegrationTest/Endpoints/IdentityServerTwoFactorTests.cs: 357
detailsThe method AssertResponseTypeIs embeds untrusted data in generated output with Response, at line 225 of /test/Common/Helpers/AssertHelper.cs. This ...
Attack Vector
HIGH Reflected_XSS_All_Clients /test/Identity.IntegrationTest/Endpoints/IdentityServerTwoFactorTests.cs: 320
detailsThe method AssertResponseTypeIs embeds untrusted data in generated output with Response, at line 225 of /test/Common/Helpers/AssertHelper.cs. This ...
Attack Vector
HIGH Reflected_XSS_All_Clients /test/Identity.IntegrationTest/Endpoints/IdentityServerTwoFactorTests.cs: 259
detailsThe method AssertResponseTypeIs embeds untrusted data in generated output with Response, at line 225 of /test/Common/Helpers/AssertHelper.cs. This ...
Attack Vector
HIGH Reflected_XSS_All_Clients /test/Identity.IntegrationTest/Endpoints/IdentityServerTwoFactorTests.cs: 141
detailsThe method AssertResponseTypeIs embeds untrusted data in generated output with Response, at line 225 of /test/Common/Helpers/AssertHelper.cs. This ...
Attack Vector
HIGH Reflected_XSS_All_Clients /test/IntegrationTestCommon/Factories/IdentityApplicationFactory.cs: 73
detailsThe method AssertResponseTypeIs embeds untrusted data in generated output with Response, at line 225 of /test/Common/Helpers/AssertHelper.cs. This ...
Attack Vector
HIGH Reflected_XSS_All_Clients /test/IntegrationTestCommon/Factories/IdentityApplicationFactory.cs: 101
detailsThe method AssertResponseTypeIs embeds untrusted data in generated output with Response, at line 225 of /test/Common/Helpers/AssertHelper.cs. This ...
Attack Vector
MEDIUM CSRF /src/Admin/AdminConsole/Controllers/ProvidersController.cs: 419
detailsMethod CreateOrganization at line 419 of /src/Admin/AdminConsole/Controllers/ProvidersController.cs gets a parameter from a user request from provi...
Attack Vector
MEDIUM CSRF /src/Api/AdminConsole/Controllers/ProviderOrganizationsController.cs: 61
detailsMethod Add at line 61 of /src/Api/AdminConsole/Controllers/ProviderOrganizationsController.cs gets a parameter from a user request from providerId....
Attack Vector
MEDIUM CSRF /src/Billing/Controllers/StripeController.cs: 164
detailsMethod TryParseEventFromRequestBodyAsync at line 164 of /src/Billing/Controllers/StripeController.cs gets a parameter from a user request from Body...
Attack Vector
MEDIUM CSRF /src/Billing/Controllers/RecoveryController.cs: 38
detailsMethod ProcessEventsAsync at line 38 of /src/Billing/Controllers/RecoveryController.cs gets a parameter from a user request from requestBody. This ...
Attack Vector
MEDIUM CSRF /src/Api/AdminConsole/Controllers/PoliciesController.cs: 197
detailsMethod Put at line 197 of /src/Api/AdminConsole/Controllers/PoliciesController.cs gets a parameter from a user request from orgId. This parameter v...
Attack Vector
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationDomainController.cs: 111
detailsMethod RemoveDomain at line 111 of /src/Api/AdminConsole/Controllers/OrganizationDomainController.cs gets a parameter from a user request from id. ...
Attack Vector
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationDomainController.cs: 111
detailsMethod RemoveDomain at line 111 of /src/Api/AdminConsole/Controllers/OrganizationDomainController.cs gets a parameter from a user request from orgI...
Attack Vector
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 220
detailsMethod PutAdmin at line 220 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This parameter value f...
Attack Vector
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationDomainController.cs: 78
detailsMethod Post at line 78 of /src/Api/AdminConsole/Controllers/OrganizationDomainController.cs gets a parameter from a user request from model. This p...
Attack Vector
MEDIUM CSRF /src/Api/Tools/Controllers/SendsController.cs: 166
detailsMethod PostFile at line 166 of /src/Api/Tools/Controllers/SendsController.cs gets a parameter from a user request from model. This parameter value ...
Attack Vector
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationDomainController.cs: 77
detailsMethod Post at line 77 of /src/Api/AdminConsole/Controllers/OrganizationDomainController.cs gets a parameter from a user request from orgId. This p...
Attack Vector
MEDIUM CSRF /src/Api/Tools/Controllers/ImportCiphersController.cs: 47
detailsMethod PostImport at line 47 of /src/Api/Tools/Controllers/ImportCiphersController.cs gets a parameter from a user request from model. This paramet...
Attack Vector
MEDIUM CSRF /src/Api/Auth/Controllers/TwoFactorController.cs: 406
detailsMethod PostRecover at line 406 of /src/Api/Auth/Controllers/TwoFactorController.cs gets a parameter from a user request from model. This parameter ...
Attack Vector
MEDIUM CSRF /src/Events/Controllers/CollectController.cs: 41
detailsMethod Post at line 41 of /src/Events/Controllers/CollectController.cs gets a parameter from a user request from model. This parameter value flows ...
Attack Vector
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 220
detailsMethod PutAdmin at line 220 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from model. This parameter valu...
Attack Vector
MEDIUM CSRF /src/Api/Tools/Controllers/SendsController.cs: 156
detailsMethod Post at line 156 of /src/Api/Tools/Controllers/SendsController.cs gets a parameter from a user request from model. This parameter value flow...
Attack Vector
MEDIUM CSRF /src/Api/Tools/Controllers/SendsController.cs: 272
detailsMethod Put at line 272 of /src/Api/Tools/Controllers/SendsController.cs gets a parameter from a user request from model. This parameter value flows...
Attack Vector
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 173
detailsMethod PostAdmin at line 173 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from model. This parameter val...
Attack Vector
MEDIUM CSRF /src/Api/Billing/Controllers/ProviderBillingController.cs: 51
detailsMethod GenerateClientInvoiceReportAsync at line 51 of /src/Api/Billing/Controllers/ProviderBillingController.cs gets a parameter from a user reques...
Attack Vector
MEDIUM CSRF /src/Identity/Controllers/AccountsController.cs: 184
detailsMethod PostRegisterFinish at line 184 of /src/Identity/Controllers/AccountsController.cs gets a parameter from a user request from PostRegisterFini...
Attack Vector
MEDIUM CSRF /src/Identity/Controllers/AccountsController.cs: 152
detailsMethod PostRegisterVerificationEmailClicked at line 152 of /src/Identity/Controllers/AccountsController.cs gets a parameter from a user request fro...
Attack Vector
MEDIUM CSRF /src/Api/Tools/Controllers/ReportsController.cs: 150
detailsMethod AddPasswordHealthReportApplications at line 150 of /src/Api/Tools/Controllers/ReportsController.cs gets a parameter from a user request from...
Attack Vector
MEDIUM CSRF /src/Api/Tools/Controllers/ReportsController.cs: 125
detailsMethod AddPasswordHealthReportApplication at line 125 of /src/Api/Tools/Controllers/ReportsController.cs gets a parameter from a user request from ...
Attack Vector
MEDIUM CSRF /src/Api/SecretsManager/Controllers/CountsController.cs: 83
detailsMethod GetByServiceAccountAsync at line 83 of /src/Api/SecretsManager/Controllers/CountsController.cs gets a parameter from a user request from Get...
Attack Vector
MEDIUM CSRF /src/Api/SecretsManager/Controllers/CountsController.cs: 62
detailsMethod GetByProjectAsync at line 62 of /src/Api/SecretsManager/Controllers/CountsController.cs gets a parameter from a user request from GetByProje...
Attack Vector
MEDIUM CSRF /src/Api/Billing/Controllers/OrganizationSponsorshipsController.cs: 106
detailsMethod PreValidateSponsorshipToken at line 106 of /src/Api/Billing/Controllers/OrganizationSponsorshipsController.cs gets a parameter from a user r...
Attack Vector
MEDIUM CSRF /src/Api/Auth/Controllers/TwoFactorController.cs: 214
detailsMethod PutOrganizationDuo at line 214 of /src/Api/Auth/Controllers/TwoFactorController.cs gets a parameter from a user request from PutOrganization...
Attack Vector
MEDIUM CSRF /src/Api/Auth/Controllers/TwoFactorController.cs: 180
detailsMethod PutDuo at line 180 of /src/Api/Auth/Controllers/TwoFactorController.cs gets a parameter from a user request from PutDuo. This parameter valu...
Attack Vector
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 980
detailsMethod SetUserVerifyDevicesAsync at line 980 of /src/Api/Auth/Controllers/AccountsController.cs gets a parameter from a user request from SetUserVe...
Attack Vector
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 582
detailsMethod BulkDeleteAccount at line 582 of /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs gets a parameter from a user request from ...
Attack Vector

More results are available on the CxOne platform

Fixed Issues (7)
Great job! The following issues were fixed in this Pull Request

Severity Issue Source File / Package
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 972
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 639
MEDIUM CSRF /src/Api/Auth/Controllers/TwoFactorController.cs: 108
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 615
MEDIUM SSL_Verification_Bypass /src/Core/Services/Implementations/MailKitSmtpMailDeliveryService.cs: 73
MEDIUM SSL_Verification_Bypass /src/Core/Services/Implementations/MailKitSmtpMailDeliveryService.cs: 73
MEDIUM SSL_Verification_Bypass /src/Core/Services/Implementations/MailKitSmtpMailDeliveryService.cs: 73

Copy link
Member

@shane-melton shane-melton left a comment

Choose a reason for hiding this comment

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

Changes look good, but looks like we need to update CipherController tests.

@Jingo88 Jingo88 requested a review from shane-melton October 4, 2024 19:56
Copy link

codecov bot commented Oct 4, 2024

Codecov Report

Attention: Patch coverage is 3.22581% with 30 lines in your changes missing coverage. Please review.

Project coverage is 44.22%. Comparing base (72b78ed) to head (d3152cb).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/Api/Vault/Controllers/CiphersController.cs 3.33% 29 Missing ⚠️
...ture.Dapper/Vault/Repositories/CipherRepository.cs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4844      +/-   ##
==========================================
- Coverage   44.24%   44.22%   -0.02%     
==========================================
  Files        1492     1492              
  Lines       68841    68869      +28     
  Branches     6199     6203       +4     
==========================================
  Hits        30456    30456              
- Misses      37066    37094      +28     
  Partials     1319     1319              

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

shane-melton
shane-melton previously approved these changes Oct 7, 2024
Copy link
Member

@shane-melton shane-melton left a comment

Choose a reason for hiding this comment

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

Changes look good! Just two small nits on the formatting of the sproc and we'll want to update the migration script date.

[Reprompt],
[Key],
[OrganizationUseTotp]
, MAX ([Edit]) AS [Edit]
Copy link
Member

Choose a reason for hiding this comment

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

⛏️ Can we fix the formatting here so that the , comes at the end of each line (to match the rest)?

[Reprompt],
[Key],
[OrganizationUseTotp]

Copy link
Member

Choose a reason for hiding this comment

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

⛏️ Extra whitespace we can remove

Copy link
Member

Choose a reason for hiding this comment

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

🎨 We'll want to update the date in this migration script to todays date since we already have a few others on main with a more recent date.

@Jingo88 Jingo88 requested a review from shane-melton November 6, 2024 19:05
shane-melton
shane-melton previously approved these changes Nov 6, 2024
Copy link
Member

@shane-melton shane-melton left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good to go!

rkac-bw
rkac-bw previously approved these changes Nov 7, 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.

lgtm

Copy link
Member

@shane-melton shane-melton left a comment

Choose a reason for hiding this comment

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

I was looking at this again after I ran into something similar in my current work. I think we may need to tweak another sproc.

@Jingo88 Jingo88 dismissed stale reviews from rkac-bw and shane-melton via ced2997 December 11, 2024 20:04
rkac-bw
rkac-bw previously approved these changes Dec 11, 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.

lgtm

shane-melton
shane-melton previously approved these changes Dec 11, 2024
@Jingo88 Jingo88 dismissed stale reviews from shane-melton and rkac-bw via c40ce6d December 23, 2024 22:04
@Jingo88
Copy link
Contributor Author

Jingo88 commented Jan 7, 2025

After speaking with product and design we have decided to move away from hiding Can Edit Except Passwords collections from the dropdown. The HidePasswords value is no longer needed.

shane-melton
shane-melton previously approved these changes Jan 8, 2025
rkac-bw
rkac-bw previously approved these changes Jan 15, 2025
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.

lgtm

@Jingo88 Jingo88 dismissed stale reviews from rkac-bw and shane-melton via d3152cb February 4, 2025 20:37
@Jingo88 Jingo88 requested a review from rkac-bw February 4, 2025 20:42
@Jingo88 Jingo88 removed the needs-qa label Feb 4, 2025
@Jingo88 Jingo88 merged commit 412c6f9 into main Feb 4, 2025
55 of 56 checks passed
@Jingo88 Jingo88 deleted the PM-11162-assign-to-collection-perm-update branch February 4, 2025 20:45
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.

5 participants