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

fix: password is removed from DB if registry is modified without checking the change password option #2875

Merged
merged 1 commit into from
Nov 26, 2024

Conversation

agatha197
Copy link
Contributor

@agatha197 agatha197 commented Nov 24, 2024

resolves #2874

Changes:
Added a new mutation ContainerRegistryEditorModalModifyWithoutPasswordMutation to handle registry modifications when the password remains unchanged. This prevents sending unnecessary password data during updates.

Implementation Details:

  • Created a separate mutation path for registry modifications without password changes
  • Updated the handleSave function to use the appropriate mutation based on whether the password was modified
  • Added loading state tracking for the new mutation in the modal's confirm button

Rationale:
This change improves security by not transmitting password data when it hasn't been modified, following the principle of least privilege.

Checklist:

  • Mention to the original issue
  • Documentation
  • Minium required manager version: 24.09
  • Specific setting for review: refer issue
  • Minimum requirements to check during review
  • Test case(s) to demonstrate the difference of before/after

Testing Requirements:

  1. Verify registry updates work when password is not modified
  2. Confirm password updates still function when modified
  3. Check error handling for both mutation paths

Copy link

graphite-app bot commented Nov 24, 2024

Your org requires the Graphite merge queue for merging into main

Add the label “flow:merge-queue” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “flow:hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

Copy link
Contributor Author

@github-actions github-actions bot added the size:L 100~500 LoC label Nov 24, 2024
Copy link

github-actions bot commented Nov 24, 2024

Coverage report for ./react

St.
Category Percentage Covered / Total
🔴 Statements
5.27% (-0.01% 🔻)
395/7491
🔴 Branches
4.57% (-0.01% 🔻)
237/5182
🔴 Functions
3.16% (-0% 🔻)
78/2471
🔴 Lines
5.19% (-0.01% 🔻)
380/7323

Test suite run success

124 tests passing in 14 suites.

Report generated by 🧪jest coverage report action from a37c79c

@agatha197 agatha197 requested a review from yomybaby November 24, 2024 23:26
Copy link
Contributor Author

@agatha197 agatha197 left a comment

Choose a reason for hiding this comment

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

It will be solved by the core part.

@agatha197 agatha197 closed this Nov 24, 2024
@agatha197 agatha197 reopened this Nov 25, 2024
@agatha197 agatha197 marked this pull request as ready for review November 25, 2024 04:02
@github-actions github-actions bot added field:UI / UX type:fix Fix features that are not working urgency:3 Must be finished within a certain time frame. labels Nov 25, 2024
Copy link
Contributor Author

agatha197 commented Nov 25, 2024

Merge activity

  • Nov 24, 11:02 PM EST: The merge label 'flow:hotfix' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Nov 25, 10:08 PM EST: A user added this pull request to the Graphite merge queue.
  • Nov 25, 10:10 PM EST: A user merged this pull request with the Graphite merge queue.

@agatha197 agatha197 added urgency:blocker IT SHOULD BE RESOLVED BEFORE DISTRIBUTING urgency:4 As soon as feasible, implementation is essential. labels Nov 25, 2024 — with Graphite App
@agatha197 agatha197 removed the urgency:3 Must be finished within a certain time frame. label Nov 25, 2024
@github-actions github-actions bot added the urgency:3 Must be finished within a certain time frame. label Nov 25, 2024
@agatha197 agatha197 force-pushed the fix/password-is-removed-when-modifying branch 2 times, most recently from 457c15c to 93f2faa Compare November 26, 2024 01:50
@agatha197 agatha197 requested a review from yomybaby November 26, 2024 01:51
Copy link
Member

@yomybaby yomybaby left a comment

Choose a reason for hiding this comment

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

LGTM

…king the change password option (#2875)

resolves #2874

**Changes:**
Added a new mutation `ContainerRegistryEditorModalModifyWithoutPasswordMutation` to handle registry modifications when the password remains unchanged. This prevents sending unnecessary password data during updates.

**Implementation Details:**
- Created a separate mutation path for registry modifications without password changes
- Updated the `handleSave` function to use the appropriate mutation based on whether the password was modified
- Added loading state tracking for the new mutation in the modal's confirm button

**Rationale:**
This change improves security by not transmitting password data when it hasn't been modified, following the principle of least privilege.

**Checklist:**
- [x] Mention to the original issue
- [ ] Documentation
- [x] Minium required manager version: 24.09
- [x] Specific setting for review: refer issue
- [ ] Minimum requirements to check during review
- [ ] Test case(s) to demonstrate the difference of before/after

**Testing Requirements:**
1. Verify registry updates work when password is not modified
2. Confirm password updates still function when modified
3. Check error handling for both mutation paths
@agatha197 agatha197 force-pushed the fix/password-is-removed-when-modifying branch from 93f2faa to a37c79c Compare November 26, 2024 03:08
@graphite-app graphite-app bot merged commit a37c79c into main Nov 26, 2024
7 checks passed
@graphite-app graphite-app bot deleted the fix/password-is-removed-when-modifying branch November 26, 2024 03:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
field:UI / UX flow:hotfix size:L 100~500 LoC type:fix Fix features that are not working urgency:blocker IT SHOULD BE RESOLVED BEFORE DISTRIBUTING urgency:3 Must be finished within a certain time frame. urgency:4 As soon as feasible, implementation is essential.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Password removed from DB if registry is modified without checking the change password option.
2 participants