From 56898df93fd8ac004313c2777dfd9425da67fe98 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Fri, 3 Jun 2022 12:08:49 +0200 Subject: [PATCH] [ADD] runbot_merge: remote user provisioning New accounts endpoint such that the SSO can push new pre-configured users / employees directly. This lowers maintenance burden. Also remove one of the source partners from the merge test, as ordering seems wonky for unclear reasons leading to random failures of that test. --- conftest.py | 30 +++++- .../controllers/reviewer_provisioning.py | 101 ++++++++++++++++-- runbot_merge/models/res_partner.py | 1 + runbot_merge/tests/test_oddities.py | 38 ++++++- runbot_merge/tests/test_provisioning.py | 93 ++++++++++++++++ 5 files changed, 251 insertions(+), 12 deletions(-) create mode 100644 runbot_merge/tests/test_provisioning.py diff --git a/conftest.py b/conftest.py index 57a240e1..33c72ffc 100644 --- a/conftest.py +++ b/conftest.py @@ -50,11 +50,14 @@ import functools import http.client import itertools import os +import pathlib +import pprint import random import re import socket import subprocess import sys +import tempfile import time import uuid import warnings @@ -271,7 +274,7 @@ class DbDict(dict): subprocess.run([ 'odoo', '--no-http', '--addons-path', self._adpath, - '-d', db, '-i', module, + '-d', db, '-i', module + ',auth_oauth', '--max-cron-threads', '0', '--stop-after-init', '--log-level', 'warn' @@ -334,17 +337,38 @@ def port(): s.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) return s.getsockname()[1] +@pytest.fixture(scope='session') +def dummy_addons_path(): + with tempfile.TemporaryDirectory() as dummy_addons_path: + mod = pathlib.Path(dummy_addons_path, 'saas_worker') + mod.mkdir(0o700) + (mod / '__init__.py').write_bytes(b'') + (mod / '__manifest__.py').write_text(pprint.pformat({ + 'name': 'dummy saas_worker', + 'version': '1.0', + }), encoding='utf-8') + (mod / 'util.py').write_text("""\ +def from_role(_): + return lambda fn: fn +""", encoding='utf-8') + + yield dummy_addons_path + @pytest.fixture -def server(request, db, port, module): +def server(request, db, port, module, dummy_addons_path): log_handlers = [ 'odoo.modules.loading:WARNING', ] if not request.config.getoption('--log-github'): log_handlers.append('github_requests:WARNING') + addons_path = ','.join(map(str, [ + request.config.getoption('--addons-path'), + dummy_addons_path, + ])) p = subprocess.Popen([ 'odoo', '--http-port', str(port), - '--addons-path', request.config.getoption('--addons-path'), + '--addons-path', addons_path, '-d', db, '--max-cron-threads', '0', # disable cron threads (we're running crons by hand) *itertools.chain.from_iterable(('--log-handler', h) for h in log_handlers), diff --git a/runbot_merge/controllers/reviewer_provisioning.py b/runbot_merge/controllers/reviewer_provisioning.py index 75abf463..9d760dc4 100644 --- a/runbot_merge/controllers/reviewer_provisioning.py +++ b/runbot_merge/controllers/reviewer_provisioning.py @@ -1,18 +1,107 @@ # -*- coding: utf-8 -*- +import logging + +from odoo.http import Controller, request, route -from odoo import http -from odoo.http import request try: from odoo.addons.saas_worker.util import from_role except ImportError: def from_role(_): return lambda _: None - -class MergebotReviewerProvisioning(http.Controller): +_logger = logging.getLogger(__name__) +class MergebotReviewerProvisioning(Controller): + @from_role('accounts') + @route('/runbot_merge/users', type='json', auth='public') + def list_users(self): + env = request.env(su=True) + return [{ + 'github_login': u.github_login, + 'email': u.email, + } + for u in env['res.users'].search([]) + ] @from_role('accounts') - @http.route(['/runbot_merge/get_reviewers'], type='json', auth='public') + @route('/runbot_merge/provision', type='json', auth='public') + def provision_user(self, users): + _logger.info('Provisioning %s users: %s.', len(users), ', '.join(map( + '{email} ({github_login})'.format_map, + users + ))) + env = request.env(su=True) + Partners = env['res.partner'] + Users = env['res.users'] + + existing_partners = Partners.search([ + '|', ('email', 'in', [u['email'] for u in users]), + ('github_login', 'in', [u['github_login'] for u in users]) + ]) + _logger.info("Found %d existing matching partners.", len(existing_partners)) + partners = {} + for p in existing_partners: + if p.email: + # email is not unique, though we want it to be (probably) + current = partners.get(p.email) + if current: + _logger.warning( + "Lookup conflict: %r set on two partners %r and %r.", + p.email, current.display_name, p.display_name, + ) + else: + partners[p.email] = p + + if p.github_login: + # assume there can't be an existing one because github_login is + # unique, and should not be able to collide with emails + partners[p.github_login] = p + + if 'oauth_provider_id' in Users: + odoo_provider = env.ref('auth_oauth.provider_openerp', False) + if odoo_provider: + for new in users: + if 'sub' in new: + new['oauth_provider_id'] = odoo_provider.id + new['oauth_uid'] = new.pop('sub') + + to_create = [] + created = updated = 0 + for new in users: + # prioritise by github_login as that's the unique-est point of information + current = partners.get(new['github_login']) or partners.get(new['email']) or Partners + # entry doesn't have user -> create user + if not current.user_ids: + new['login'] = new['email'] + # entry has partner -> create user linked to existing partner + # (and update partner implicitly) + if current: + new['partner_id'] = current.id + to_create.append(new) + continue + + # 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) + user = user[:1] + update_vals = { + k: v + for k, v in new.items() + if v not in ('login', 'email') + if v != (user[k] if k != 'oauth_provider_id' else user[k].id) + } + if update_vals: + user.write(update_vals) + updated += 1 + if to_create: + Users.create(to_create) + created = len(to_create) + + _logger.info("Provisioning: created %d updated %d.", created, updated) + return [created, updated] + + @from_role('accounts') + @route(['/runbot_merge/get_reviewers'], type='json', auth='public') def fetch_reviewers(self, **kwargs): reviewers = request.env['res.partner.review'].sudo().search([ '|', ('review', '=', True), ('self_review', '=', True) @@ -20,7 +109,7 @@ class MergebotReviewerProvisioning(http.Controller): return reviewers @from_role('accounts') - @http.route(['/runbot_merge/remove_reviewers'], type='json', auth='public', methods=['POST']) + @route(['/runbot_merge/remove_reviewers'], type='json', auth='public', methods=['POST']) def update_reviewers(self, github_logins, **kwargs): partners = request.env['res.partner'].sudo().search([('github_login', 'in', github_logins)]) partners.write({ diff --git a/runbot_merge/models/res_partner.py b/runbot_merge/models/res_partner.py index 3956af9c..d627b2f4 100644 --- a/runbot_merge/models/res_partner.py +++ b/runbot_merge/models/res_partner.py @@ -13,6 +13,7 @@ class CIText(fields.Char): class Partner(models.Model): _inherit = 'res.partner' + email = fields.Char(index=True) github_login = CIText() delegate_reviewer = fields.Many2many('runbot_merge.pull_requests') formatted_email = fields.Char(string="commit email", compute='_rfc5322_formatted') diff --git a/runbot_merge/tests/test_oddities.py b/runbot_merge/tests/test_oddities.py index b4a1fcf2..3ad1289b 100644 --- a/runbot_merge/tests/test_oddities.py +++ b/runbot_merge/tests/test_oddities.py @@ -1,11 +1,10 @@ +import requests + from utils import Commit, to_pr def test_partner_merge(env): p_src = env['res.partner'].create({ - 'name': 'kfhsf', - 'github_login': 'tyu' - }) | env['res.partner'].create({ 'name': "xxx", 'github_login': 'xxx' }) @@ -97,3 +96,36 @@ def test_message_desync(env, project, make_repo, users, setreviewers, config): assert st.message.startswith('title\n\nbody'),\ "the stored PR message should have been ignored when staging" assert st.parents == [m, c], "check the staging's ancestry is the right one" + +def test_unreviewer(env, project, port): + repo = env['runbot_merge.repository'].create({ + 'project_id': project.id, + 'name': 'a_test_repo', + 'status_ids': [(0, 0, {'context': 'status'})] + }) + p = env['res.partner'].create({ + 'name': 'George Pearce', + 'github_login': 'emubitch', + 'review_rights': [(0, 0, {'repository_id': repo.id, 'review': True})] + }) + + r = requests.post(f'http://localhost:{port}/runbot_merge/get_reviewers', json={ + 'jsonrpc': '2.0', + 'id': None, + 'method': 'call', + 'params': {}, + }) + r.raise_for_status() + assert 'error' not in r.json() + assert r.json()['result'] == ['emubitch'] + + r = requests.post(f'http://localhost:{port}/runbot_merge/remove_reviewers', json={ + 'jsonrpc': '2.0', + 'id': None, + 'method': 'call', + 'params': {'github_logins': ['emubitch']}, + }) + r.raise_for_status() + assert 'error' not in r.json() + + assert p.review_rights == env['res.partner.review'] diff --git a/runbot_merge/tests/test_provisioning.py b/runbot_merge/tests/test_provisioning.py new file mode 100644 index 00000000..debfa3b5 --- /dev/null +++ b/runbot_merge/tests/test_provisioning.py @@ -0,0 +1,93 @@ +import pytest +import requests + +GEORGE = { + 'name': "George Pearce", + 'email': 'george@example.org', + 'github_login': 'emubitch', + 'sub': '19321102' +} +def test_basic_provisioning(env, port): + + r = provision_user(port, [GEORGE]) + assert r == [1, 0] + + g = env['res.users'].search([('login', '=', GEORGE['email'])]) + assert g.partner_id.name == GEORGE['name'] + assert g.partner_id.github_login == GEORGE['github_login'] + assert g.oauth_uid == GEORGE['sub'] + + # repeated provisioning should be a no-op + r = provision_user(port, [GEORGE]) + assert r == [0, 0] + + # the email (real login) should be the determinant, any other field is + # updatable + r = provision_user(port, [{**GEORGE, 'name': "x"}]) + assert r == [0, 1] + + 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 + p = env['res.partner'].create({ + 'name': GEORGE['name'], + 'email': GEORGE['email'], + }) + r = provision_user(port, [GEORGE]) + assert r == [1, 0] + assert p.user_ids.read(['email', 'github_login', 'oauth_uid']) == [{ + 'id': p.user_ids.id, + 'github_login': GEORGE['github_login'], + 'oauth_uid': GEORGE['sub'], + 'email': GEORGE['email'], + }] + + p.user_ids.unlink() + p.unlink() + + p = env['res.partner'].create({ + 'name': GEORGE['name'], + 'github_login': GEORGE['github_login'], + }) + r = provision_user(port, [GEORGE]) + assert r == [1, 0] + assert p.user_ids.read(['email', 'github_login', 'oauth_uid']) == [{ + 'id': p.user_ids.id, + 'github_login': GEORGE['github_login'], + 'oauth_uid': GEORGE['sub'], + 'email': GEORGE['email'], + }] + + p.user_ids.unlink() + p.unlink() + +def provision_user(port, users): + r = requests.post(f'http://localhost:{port}/runbot_merge/provision', json={ + 'jsonrpc': '2.0', + 'id': None, + 'method': 'call', + 'params': {'users': users}, + }) + r.raise_for_status() + json = r.json() + assert 'error' not in json + + return json['result']