-
Notifications
You must be signed in to change notification settings - Fork 25
feat: prevent multiple connections to the same system #93
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
base: feature/migration-logging-refactor
Are you sure you want to change the base?
feat: prevent multiple connections to the same system #93
Conversation
src/Resources/app/administration/src/module/swag-migration/snippet/de.json
Outdated
Show resolved
Hide resolved
…or' into feat/prevent-multiple-connections-to-same-source-shop
…ultiple-connections-to-same-source-shop
| ); | ||
|
|
||
| if ($hasDuplicate) { | ||
| throw MigrationException::duplicateSourceConnection(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't doing all of this in updateConnectionCredentials only prevent the already created duplicate connection from receiving a fingerprint?
I tested this locally (SW6 → SW6) and creating a duplicate connection currently behaves like this:
-
While entering the details for the second connection, an empty connection is already created in
swag_migration_connection -
After submitting the duplicate details, I do see the new "A connection to the same source system already exists." warning, but the connection is updated nonetheless, and the
swag_migration_connectionrow now contains the credentials, only the fingerprint is missing:
-
After dismissing the modal with "Close", the duplicate connection is not only created, but also active
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a general migration design issue we didn't refactor until now. I suspect it was done this way because the wizard is url based: each step lives on its "own page", so state isn't shared and nothing gets stored in vuex. That said, this shouldn't prevent existing duplicate connections from receiving a fingerprint, the service only checks for existing fingerprints and exits early when none are present. So the first connection should get a fingerprint and is valid, the second not...
but in this case it seems different, I will look into that, probably need to add a filter for null in the service 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I don't get is what this PR achieves then, because the only difference to me as a user of this is that I see the warning modal, but nothing "prevent[s] multiple connections to the same system" - what am I missing here? 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i get what you mean now, it validates newly created connections: "prevent creating multiple connections to the same system". Existing connections do not have a fingerprint, that's true, so their are not validated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
found the issue: we update the connection first and then did a connection check, so creds where written before checking. I refactored it and now we first check the connection with given creds and then update the entity & generate the fingerprint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better 👍 but I still think there's something strange about it 🤔
- UI briefly shows connection success before the warning:
Kapture.2025-12-10.at.18.16.43.mp4
- As a user, I am still left with an empty connection when this happens (now without credentials), which is a confusing state to me. While creating an exact duplicate of my existing one was prevented, it would be nicer if this empty connection would either be cleaned up or not created at all, so I am back to my initial (working) connection when I exit this modal, instead of seeing this:
Feel free to move this to follow-up issues due to how all of this works, but I think this feature does not make much sense with the behavior described in 2.
resolves shopware/shopware#13670
SwagMigrationConnector pr: shopware/SwagMigrationConnector#5
This pull request introduces a fingerprinting mechanism for migration connections to prevent duplicate source system connections.
Changes:
sourceSystemFingerprintcolumn to theswag_migration_connectiontable.MigrationFingerprintProvidersto extract data & generate fingerprints for specific migration profilesMigrationFingerprintServiceto generate fingerprints for connections utilizing theMigrationFingerprintProvidersRunService::updateConnectionCredentialsto generate and store the fingerprint, and throw aMigrationExceptionif a duplicate source system connection is detected.DUPLICATE_SOURCE_CONNECTIONerror code and exception to handle duplicate source system connections.Strategy
Shopware 6
For Shopware 6 connections, we use the
shopIdV2property fromsystem_configas the connection fingerprint. This ID uniquely identifies every Shopware 6 shop.Shopware 5
Since Shopware 5 doesn't have a shopId and the SBP identifies shops by domain, we extract
esdKeyandinstallationDatefroms_core_config_elements, hash them together, and use the resulting hash as the connection fingerprint:esdKey: An automatically generated identifier for Shopware 5 shops, present after initial installation. In Shopware 5, it's used to generate serial numbers for digital products. (Note: Theoretically editable by the shop owner, but this is highly unlikely in practice.)installationDate: The datetime set during the Shopware 5 installation process.=> we can of course use other fields and hash them if you have a idea for more appropriate ones