Ensure backend is verified on sync. #30412
Conversation
tomneedham
force-pushed
the
sync-dedup
branch
3 times, most recently
from
debd466 to
df6dc1e
Compare
| } catch (DoesNotExistException $e) { | ||
| // Create a new account for this uid and backend pairing and sync | ||
| $account = $this->createNewAccount(get_class($backend), $uid); | ||
| } catch (MultipleObjectsReturnedException $e) { |
There was a problem hiding this comment.
Generaly this would be a serious bug
There was a problem hiding this comment.
Agreed - can happen with mysql though - and I was fed up with phpstorm complaining at me
|
As discussed, we need to have unit test covering this scenario |
tomneedham
force-pushed
the
sync-dedup
branch
from
df6dc1e to
732c57f
Compare
|
Added test and fixed issue in the logging statement. |
tomneedham
added
3 - To Review
backport-request
4 - To release
and removed
2 - Developing
3 - To Review
labels
Codecov Report
@@ Coverage Diff @@
## master #30412 +/- ##
============================================
+ Coverage 61.9% 61.93% +0.03%
- Complexity 19061 19085 +24
============================================
Files 1091 1092 +1
Lines 61473 61531 +58
============================================
+ Hits 38052 38109 +57
- Misses 23421 23422 +1
Continue to review full report at Codecov.
|
tomneedham
force-pushed
the
sync-dedup
branch
from
732c57f to
780cded
Compare
|
test failures related to federated shares |
|
as discussed use a hack to change the alternate home from the testing app ? |
Added it. But not sure where the backend values should be set back? Where exactly is the testing app used? |
|
The testing app is used by the API "integration" tests and the UI tests as an endpoint that can switch on/off capabilities, run an occ command... so that tests setup settings... |
| // Make existing accounts use the new alternative home backed | ||
| $q = $c->getServer()->getDatabaseConnection()->getQueryBuilder(); | ||
| $q->update('*PREFIX*accounts') | ||
| ->set('backend', AlternativeHomeUserBackend::class) |
There was a problem hiding this comment.
->set('backend', $q->expr()->literal(AlternativeHomeUserBackend::class))
|
Doesn't necessarily need a LDAP app to work. You would utilise the existing AlternativeHomeUserBackend but the testing needs to be more explicit about creating a user for that backend, and using that user - not hot swapping the backend class. |
We'd likely need to adjust all tests that create users, or the code that calls the provisioning API to create them. Currently provisioning API doesn't allow for specifying a backend and I believe it's only designed for DB backend. In any case, it feels like everything related to that is a hack. Another alternative would be to modify the actual DB user backend to also support specifying a home folder, officially, when the user gets created. |
This is fine - since the testing app removes the backends and adds the AltHomeBackend - which is extended from database and works exactly the same - so no issue here. The problem is that this home testing code is hot swapping the backend which we no longer tolerate in core. I still dont understand where this testing app is enabled, and when and why, and when disabled? If we understood that, we could simply recreate the users it needs so they have the right backend, and remove them afterwards.
Couldn't we essentially achieve this by changing the data directory value in config for the usermanager? |
Maybe, but need to first create the users and then swap the data directory. Not sure at what moment exactly during the tests. And when to set it back ? Sounds like a similar problem. |
|
The testing app is enabled once in During each scenario the needed "ordinary" users are created, and an But as you have seen, it is assumed that the ownCloud install comes with an existing |
|
based on what @phil-davis which I think is correct I don't understand why we're getting the above errors then |
|
Should we then exclude the admin user from the alternate home since it's the only user that is always there ? From what I remember most tests are anyway creating separate users. |
|
pushed... let's find out |
|
how come... |
|
I think the problem is that the "admin" account already exists before the testing app is enabled, since we're using a formerly set up system. I'm now struggling to find a way to make the testing app change all backends once at the time the app gets enabled. |
|
Will life be easier if we create an That way the original |
|
A fresh new hack has arrived: now at the time where the testing app gets enabled, it will adjust all DB backends to alternate home backend. This makes sure that existing users like the admin gets their backend converted. We only do it once to avoid the "database is locked" error we had before.
@tomneedham please check if this additional fix is ok, I keep seeing this in the logs for empty string: 1d7276a |
|
if my last hack doesn't work we could try #30412 (comment) next @phil-davis |
|
ohh look... it worked... ok, will resolve conflict and clean up |
For example if enabling an app failed, it must fail and bail out
Fixes some database locking errors with federated sharing tests.
|
Squashed and rebased. @tomneedham please review my fix for the testing app and we can move forward with the review of your part. |
|
fine but fragile - no more than it was before. At some point I suggest we remove this - and get tests to configure their own admin user when necessary. |
|
See #30679 for a little tidy up of the messages in OK, I backported the 3rd commit here, along with the backport of #30679 - see #30683 |
|
Backport stable10 - the 3 commits here are backported in #30638 |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
7 participants
Description
When we sync with an existing account entity, we must always check that we are syncing against the same backend as when the account was created.
Checklist: