diff --git a/runbot_merge/controllers/reviewer_provisioning.py b/runbot_merge/controllers/reviewer_provisioning.py index e0aa0fc7..35f07d24 100644 --- a/runbot_merge/controllers/reviewer_provisioning.py +++ b/runbot_merge/controllers/reviewer_provisioning.py @@ -35,6 +35,11 @@ class MergebotReviewerProvisioning(Controller): Partners = env['res.partner'] Users = env['res.users'] + existing_logins = set() + existing_oauth = set() + for u in Users.with_context(active_test=False).search([]): + existing_logins.add(u.login) + existing_oauth .add((u.oauth_provider_id.id, u.oauth_uid)) existing_partners = Partners.with_context(active_test=False).search([ '|', ('email', 'in', [u['email'] for u in users]), ('github_login', 'in', [u['github_login'] for u in users]) @@ -76,9 +81,30 @@ class MergebotReviewerProvisioning(Controller): to_activate |= current # entry doesn't have user -> create user if not current.user_ids: - # skip users without an email (= login) as that - # fails if not new['email']: + _logger.info( + "Unable to create user for %s: no email in provisioning data", + current.display_name + ) + continue + if 'oauth_uid' in new: + if (new['oauth_provider_id'], new['oauth_uid']) in existing_oauth: + _logger.warning( + "Attempted to create user with duplicate oauth uid " + "%s with provider %r for provisioning entry %r. " + "There is likely a duplicate partner (one version " + "with email, one with github login)", + new['oauth_uid'], odoo_provider.display_name, new, + ) + continue + if new['email'] in existing_logins: + _logger.warning( + "Attempted to create user with duplicate login %s for " + "provisioning entry %r. There is likely a duplicate " + "partner (one version with email, one with github " + "login)", + new['email'], new, + ) continue new['login'] = new['email'] @@ -93,7 +119,7 @@ class MergebotReviewerProvisioning(Controller): # otherwise update user (if there is anything to update) user = current.user_ids if len(user) != 1: - _logger.warning("Got %d users for partner %s.", len(user), current.display_name) + _logger.warning("Got %d users for partner %s, updating first.", len(user), current.display_name) user = user[:1] new.setdefault("active", True) update_vals = { diff --git a/runbot_merge/tests/test_provisioning.py b/runbot_merge/tests/test_provisioning.py index af939305..db26e5e7 100644 --- a/runbot_merge/tests/test_provisioning.py +++ b/runbot_merge/tests/test_provisioning.py @@ -1,4 +1,3 @@ -import pytest import requests GEORGE = { @@ -30,24 +29,13 @@ def test_basic_provisioning(env, port): r = provision_user(port, [dict(GEORGE, name="x", github_login="y", sub="42")]) assert r == [0, 1] - # can't fail anymore because github_login now used to look up the existing - # user - # with pytest.raises(Exception): - # provision_user(port, [{ - # 'name': "other@example.org", - # 'email': "x", - # 'github_login': "y", - # 'sub': "42" - # }]) - r = provision_user(port, [dict(GEORGE, active=False)]) assert r == [0, 1] assert not env['res.users'].search([('login', '=', GEORGE['email'])]) assert env['res.partner'].search([('email', '=', GEORGE['email'])]) def test_upgrade_partner(env, port): - # If a partner exists for a github login (and / or email?) it can be - # upgraded by creating a user for it + # matching partner with an email but no github login p = env['res.partner'].create({ 'name': GEORGE['name'], 'email': GEORGE['email'], @@ -64,6 +52,7 @@ def test_upgrade_partner(env, port): p.user_ids.unlink() p.unlink() + # matching partner with a github login but no email p = env['res.partner'].create({ 'name': GEORGE['name'], 'github_login': GEORGE['github_login'], @@ -77,14 +66,14 @@ def test_upgrade_partner(env, port): 'email': GEORGE['email'], }] - # deactivate user to see if provisioning re-enables them + # matching partner with a deactivated user p.user_ids.active = False r = provision_user(port, [GEORGE]) assert r == [0, 1] assert len(p.user_ids) == 1, "provisioning should re-enable user" assert p.user_ids.active - # re-enable both user and partner to see if both get re-enabled + # matching deactivated partner (with a deactivated user) p.user_ids.active = False p.active = False r = provision_user(port, [GEORGE]) @@ -92,6 +81,33 @@ def test_upgrade_partner(env, port): assert p.active, "provisioning should re-enable partner" assert p.user_ids.active +def test_duplicates(env, port): + """In case of duplicate data, the handler should probably not blow up, but + instead log a warning (so the data gets fixed eventually) and skip + """ + # dupe 1: old oauth signup account & github interaction account, provisioning + # prioritises the github account & tries to create a user for it, which + # fails because the signup account has the same oauth uid (probably) + env['res.partner'].create({'name': 'foo', 'github_login': 'foo'}) + env['res.users'].create({'login': 'foo@example.com', 'name': 'foo', 'email': 'foo@example.com', 'oauth_provider_id': 1, 'oauth_uid': '42'}) + assert provision_user(port, [{ + 'name': "foo", + 'email': 'foo@example.com', + 'github_login': 'foo', + 'sub': '42' + }]) == [0, 0] + + # dupe 2: old non-oauth signup account & github interaction account, same + # as previous except it breaks on the login instead of the oauth_uid + env['res.partner'].create({'name': 'bar', 'github_login': 'bar'}) + env['res.users'].create({'login': 'bar@example.com', 'name': 'bar', 'email': 'bar@example.com'}) + assert provision_user(port, [{ + 'name': "bar", + 'email': 'bar@example.com', + 'github_login': 'bar', + 'sub': '43' + }]) == [0, 0] + def test_no_email(env, port): """ Provisioning system should ignore email-less entries """ @@ -182,6 +198,6 @@ def provision_user(port, users): }) r.raise_for_status() json = r.json() - assert 'error' not in json + assert 'error' not in json, json['error']['data']['debug'] return json['result']