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.

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

Jump to bottom

Clean up account creation and deletion code #5416

Merged
merged 7 commits into from
Feb 14, 2023
Merged

Clean up account creation and deletion code #5416

merged 7 commits into from
Feb 14, 2023

Conversation

claucambra
Copy link
Collaborator

@claucambra claucambra commented Feb 7, 2023 •
edited
Loading

This PR cleans up some of our code relating to account creation and deletion. It also improves some of the logging

@claucambra claucambra self-assigned this Feb 7, 2023
@claucambra claucambra changed the title Fix freezing and crash when adding and removing accounts on some platforms (mainly macOS) Fix freezing, crashes, and related issues when adding and removing accounts on some platforms (mainly macOS) Feb 7, 2023
@claucambra claucambra force-pushed the bugfix/remove-account-crash branch from 119ad49 to 6f72ce0 Compare February 7, 2023 19:36
@codecov
Copy link

codecov bot commented Feb 7, 2023 •
edited
Loading

Codecov Report

Merging #5416 (4477e5f) into master (0e2cf63) will decrease coverage by 0.17%.
The diff coverage is 5.37%.

❗ Current head 4477e5f differs from pull request most recent head 72fb279. Consider uploading reports for the commit 72fb279 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #5416      +/-   ##
==========================================
- Coverage   57.98%   57.81%   -0.17%     
==========================================
  Files         141      141              
  Lines       18133    18148      +15     
==========================================
- Hits        10515    10493      -22     
- Misses       7618     7655      +37
Impacted Files Coverage Δ
src/libsync/account.cpp 35.85% <0.00%> (-0.41%) ⬇️
src/libsync/clientsideencryption.h 30.76% <ø> (ø)
src/libsync/creds/keychainchunk.cpp 0.00% <0.00%> (ø)
src/libsync/creds/httpcredentials.cpp 6.89% <1.51%> (ø)
src/libsync/clientsideencryption.cpp 26.07% <15.25%> (-0.78%) ⬇️
src/libsync/creds/abstractcredentials.cpp 14.28% <0.00%> (-57.15%) ⬇️
src/libsync/propagateremotemkdir.h 50.00% <0.00%> (-25.00%) ⬇️
src/libsync/discovery.cpp 85.93% <0.00%> (-0.29%) ⬇️
src/libsync/logger.cpp 27.90% <0.00%> (ø)
... and 1 more

@allexzander
Copy link
Contributor

one comment and pleas also make sure to squash commits

@claucambra claucambra force-pushed the bugfix/remove-account-crash branch from d7cd797 to e3a119a Compare February 9, 2023 14:13
@claucambra
Copy link
Collaborator Author

one comment and pleas also make sure to squash commits

thanks, done

@claucambra claucambra force-pushed the bugfix/remove-account-crash branch 2 times, most recently from 4477e5f to 22eb749 Compare February 9, 2023 18:00
@claucambra claucambra changed the title Fix freezing, crashes, and related issues when adding and removing accounts on some platforms (mainly macOS) Clean up account creation and deletion code Feb 10, 2023
@claucambra claucambra force-pushed the bugfix/remove-account-crash branch from 22eb749 to 747c8df Compare February 10, 2023 15:26
@claucambra claucambra force-pushed the bugfix/remove-account-crash branch 3 times, most recently from 955da6d to 47c036f Compare February 14, 2023 11:32
@claucambra
Copy link
Collaborator Author

/backport to stable-3.7

@nextcloud-desktop-bot
Copy link

AppImage file: nextcloud-PR-5416-47c036fa5fee334436bbbda1045ac37baf589365-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

@claucambra
Don't early return in sensitive data deletion for clientsideencryptio…
4ded5e1 
…n if the entries were never stored in keychain

Signed-off-by: Claudio Cambra <claudio.cambra@nextcloud.com>
@claucambra
Make sure to return in checkAllSensitiveDataDeleted
e7c9785 
Signed-off-by: Claudio Cambra <claudio.cambra@nextcloud.com>
@claucambra
Add sensitiveDataRemaining convenience method to clientsideencryption
2c17305 
Signed-off-by: Claudio Cambra <claudio.cambra@nextcloud.com>
@claucambra
Avoid trying to delete keychain encryption data if there is no encryp…
446bb29 
…tion data configured

Signed-off-by: Claudio Cambra <claudio.cambra@nextcloud.com>
@claucambra
Remove unused AccountState::writeToSettings
7084310 
Signed-off-by: Claudio Cambra <claudio.cambra@nextcloud.com>
@claucambra
Remove unused slotDeleteAccount in AccountSettings
893adb8 
Signed-off-by: Claudio Cambra <claudio.cambra@nextcloud.com>
@claucambra
More accurately log what happens when deleteAppPassword is complete i…
72fb279 
…n Account

Signed-off-by: Claudio Cambra <claudio.cambra@nextcloud.com>
@claucambra claucambra force-pushed the bugfix/remove-account-crash branch from 47c036f to 72fb279 Compare February 14, 2023 19:22
@sonarcloud
Copy link

sonarcloud bot commented Feb 14, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell B 6 Code Smells

38.9% 38.9% Coverage
0.0% 0.0% Duplication

@claucambra claucambra merged commit ccedff3 into master Feb 14, 2023
@claucambra claucambra deleted the bugfix/remove-account-crash branch February 14, 2023 20:18
@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Feb 14, 2023
Merged
@mgallien mgallien added this to the 3.8.0 milestone Mar 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@allexzander allexzander allexzander approved these changes

@camilasan camilasan camilasan approved these changes

@mgallien mgallien Awaiting requested review from mgallien

Assignees

@claucambra claucambra

Labels
None yet
Projects
None yet
Milestone
3.8.0
Development

Successfully merging this pull request may close these issues.
5 participants
@claucambra @allexzander @nextcloud-desktop-bot @camilasan @mgallien