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

Migration: Backend, Rake file, Photos import #8298

Merged
merged 6 commits into from
Nov 25, 2021
Merged

Migration: Backend, Rake file, Photos import #8298

merged 6 commits into from
Nov 25, 2021

Conversation

tclaus
Copy link
Member

@tclaus tclaus commented Sep 19, 2021

This is just the backend part extracted from the larger Upload Migration PR #8274
All UI stuff relies on this backend.

Rake file for Command line Import is included. I hope that with this smaller PR (based on the latest development) we can let thinks work faster.

@Flaburgan
Salutations, mon ami.

SuperTux88
SuperTux88 requested changes Sep 19, 2021
View reviewed changes
Copy link
Member

@SuperTux88 SuperTux88 left a comment

Choose a reason for hiding this comment

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

Thanks for the separate PR 🍪

app/models/photo.rb Outdated Show resolved Hide resolved
app/models/user.rb Outdated Show resolved Hide resolved
app/models/user.rb Outdated Show resolved Hide resolved
app/models/user.rb Outdated Show resolved Hide resolved
app/services/migration_service.rb Outdated Show resolved Hide resolved
app/uploaders/exported_user.rb Outdated Show resolved Hide resolved
lib/archive_importer.rb Outdated Show resolved Hide resolved
lib/archive_importer/post_importer.rb Outdated Show resolved Hide resolved
lib/tasks/accounts.rake Outdated Show resolved Hide resolved
lib/archive_importer.rb Outdated Show resolved Hide resolved
@tclaus
Copy link
Member Author

tclaus commented Sep 24, 2021

@Flaburgan @SuperTux88 I've sent some changes. What else is a MUST to change here?

@Flaburgan Flaburgan mentioned this pull request Sep 29, 2021
Closed
SuperTux88
SuperTux88 requested changes Sep 30, 2021
View reviewed changes
app/controllers/users_controller.rb Outdated Show resolved Hide resolved
app/models/photo.rb Outdated Show resolved Hide resolved
app/models/user.rb Outdated Show resolved Hide resolved
app/models/user.rb Outdated Show resolved Hide resolved
app/models/user.rb Outdated Show resolved Hide resolved
app/services/import_profile_service.rb Outdated Show resolved Hide resolved
app/services/import_profile_service.rb Outdated Show resolved Hide resolved
app/uploaders/exported_photos.rb Outdated Show resolved Hide resolved
app/services/import_profile_service.rb Outdated Show resolved Hide resolved
app/services/import_profile_service.rb Outdated Show resolved Hide resolved
diaspora-code-review
diaspora-code-review reviewed Oct 2, 2021
View reviewed changes
app/uploaders/secure_uploader.rb Show resolved Hide resolved
lib/archive_importer.rb Outdated Show resolved Hide resolved
SuperTux88
SuperTux88 requested changes Oct 23, 2021
View reviewed changes
Copy link
Member

@SuperTux88 SuperTux88 left a comment

Choose a reason for hiding this comment

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

So there is still the security problem with importing photos. But I'm fine with excluding photos import from this PR, but then it needs to be removed from here, since the current state is an active security problem.

I've also created diaspora/diaspora_federation#119 for the federation part of the photos-migration, to tell other pods the new path so they can migrate photos on bulk and don't need a message for every single photo. If there are conflicts with filenames, where a photo from the archive can't be imported, it either needs to be deleted (and then also federate the deletion), or be imported on a different filename (and then it would need to federate the updated filename with a single photo message). But since I don't expect many conflicts, this should be fine, but it still needs to be handled correctly.

app/models/user.rb Outdated Show resolved Hide resolved
app/models/user.rb Outdated Show resolved Hide resolved
app/uploaders/secure_uploader.rb Outdated Show resolved Hide resolved
app/workers/import_user.rb Outdated Show resolved Hide resolved
app/workers/import_user.rb Outdated Show resolved Hide resolved
app/workers/import_user.rb Outdated Show resolved Hide resolved
lib/archive_importer.rb Outdated Show resolved Hide resolved
@SuperTux88 SuperTux88 mentioned this pull request Oct 30, 2021
Closed
SuperTux88
SuperTux88 requested changes Nov 7, 2021
View reviewed changes
Copy link
Member

@SuperTux88 SuperTux88 left a comment

Choose a reason for hiding this comment

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

The PR still contains the photo import logic including the security problem, if you want to move the handling of photos to another PR (which I'm fine with), it needs to be removed here.

And besides a few small other things I think this is looking good. 👍

lib/archive_importer.rb Outdated Show resolved Hide resolved
app/uploaders/exported_photos.rb Outdated Show resolved Hide resolved
app/uploaders/exported_photos.rb Outdated Show resolved Hide resolved
app/workers/import_user.rb Outdated Show resolved Hide resolved
@tclaus
Copy link
Member Author

tclaus commented Nov 23, 2021

Fixes #8255

@SuperTux88
Send new remote_photo_path in migration message
e9f7bf3
@SuperTux88
Don't overwrite photos of other users during import
d3c2407 
If a photo with the same filename already exists, generate a new random
filename, and re-federate the photo with that filename. This ensures
users can't modify their archive to overwrite other users photos.
@tclaus
Copy link
Member Author

tclaus commented Nov 24, 2021

Looks good to me ! 👍🏻
Thanks to Benjamin for the hard rework.

@SuperTux88 SuperTux88 added this to the 0.8.0.0 milestone Nov 24, 2021
@SuperTux88 SuperTux88 merged commit 37a7c0b into diaspora:develop Nov 25, 2021
@SuperTux88
Copy link
Member

Merged, thanks for the initial work and for the fast review of my changes to it @tclaus 🍪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Flaburgan Flaburgan Flaburgan left review comments

@diaspora-code-review diaspora-code-review diaspora-code-review left review comments

@SuperTux88 SuperTux88 SuperTux88 approved these changes

Assignees
No one assigned
Labels
🚚 migration
Projects
None yet
Milestone
1.0.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@tclaus @SuperTux88 @Flaburgan @diaspora-code-review