From 75638ffb94fa4ae1753f733d8cb6f7f58f0b6a0a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Fri, 19 Oct 2018 10:46:26 +0000 Subject: [PATCH 1/4] [REF] link_tracker: do not let public read technical / private link data Link tracker models are rather technical and should not be accessed as it is by external people. Let us delegate control to controllers and lessen a bit model accessibility. Currently everyone can use add_click method and generate statistics. Now statistics will be generated only through dedicated routes. It will make statistics more reliable and ensure technical data remain private. Next commit will update code from controllers and main methods in order to be have a more readable click API and avoid link between non dependent modules. This commit is linked to task ID 1904277 and PR #28242. --- addons/link_tracker/controller/main.py | 2 +- addons/link_tracker/security/ir.model.access.csv | 15 +++++++++------ addons/mass_mailing/controllers/main.py | 2 +- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/addons/link_tracker/controller/main.py b/addons/link_tracker/controller/main.py index c03cbf56813..3b23c005446 100644 --- a/addons/link_tracker/controller/main.py +++ b/addons/link_tracker/controller/main.py @@ -11,6 +11,6 @@ class LinkTracker(http.Controller): @http.route('/r/', type='http', auth='none', website=True) def full_url_redirect(self, code, **post): country_code = request.session.geoip and request.session.geoip.get('country_code') or False - request.env['link.tracker.click'].add_click(code, request.httprequest.remote_addr, country_code, stat_id=False) + request.env['link.tracker.click'].sudo().add_click(code, request.httprequest.remote_addr, country_code, stat_id=False) redirect_url = request.env['link.tracker'].get_url_from_code(code) return werkzeug.utils.redirect(redirect_url or '', 301) diff --git a/addons/link_tracker/security/ir.model.access.csv b/addons/link_tracker/security/ir.model.access.csv index ac6c710496c..ed484dc5731 100644 --- a/addons/link_tracker/security/ir.model.access.csv +++ b/addons/link_tracker/security/ir.model.access.csv @@ -1,7 +1,10 @@ id,name,model_id:id,group_id:id,perm_read,perm_write,perm_create,perm_unlink -access_link_tracker_user,link.tracker,model_link_tracker,base.group_user,1,0,0,0 -access_link_tracker_public,link.tracker,model_link_tracker,base.group_public,1,0,0,0 -access_link_tracker_code_user,link.tracker.code,model_link_tracker_code,base.group_user,1,0,0,0 -access_link_tracker_code_public,link.tracker.code,model_link_tracker_code,base.group_public,1,0,0,0 -access_link_tracker_click_user,link.tracker.click,model_link_tracker_click,base.group_user,1,0,0,0 -access_link_tracker_click_public,link.tracker.click,model_link_tracker_click,base.group_public,1,0,0,0 +access_link_tracker_user,access.link.tracker.user,model_link_tracker,base.group_user,1,0,0,0 +access_link_tracker_public,access.link.tracker.public,model_link_tracker,base.group_public,0,0,0,0 +access_link_tracker_system,access.link.tracker.system,model_link_tracker,base.group_system,1,1,1,1 +access_link_tracker_code_user,access.link.tracker.code.user,model_link_tracker_code,base.group_user,1,0,0,0 +access_link_tracker_code_public,access.link.tracker.code.public,model_link_tracker_code,base.group_public,0,0,0,0 +access_link_tracker_code_system,access.link.tracker.code.system,model_link_tracker_code,base.group_system,1,1,1,1 +access_link_tracker_click_user,access.link.tracker.click.user,model_link_tracker_click,base.group_user,1,0,0,0 +access_link_tracker_click_public,access.link.tracker.click.public,model_link_tracker_click,base.group_public,0,0,0,0 +access_link_tracker_click_system,access.link.tracker.click.system,model_link_tracker_click,base.group_system,1,1,1,1 diff --git a/addons/mass_mailing/controllers/main.py b/addons/mass_mailing/controllers/main.py index 11b05685a06..440def20462 100644 --- a/addons/mass_mailing/controllers/main.py +++ b/addons/mass_mailing/controllers/main.py @@ -107,7 +107,7 @@ class MassMailController(http.Controller): # which mass_mailing doesn't depend on country_code = request.session.get('geoip', False) and request.session.geoip.get('country_code', False) - request.env['link.tracker.click'].add_click(code, request.httprequest.remote_addr, country_code, stat_id=stat_id) + request.env['link.tracker.click'].sudo().add_click(code, request.httprequest.remote_addr, country_code, stat_id=stat_id) return werkzeug.utils.redirect(request.env['link.tracker'].get_url_from_code(code), 301) @http.route('/mailing/blacklist/check', type='json', auth='none') From b670911a92883ebea49d427acafe2abac2753ace Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Mon, 29 Oct 2018 14:23:16 +0000 Subject: [PATCH 2/4] [REF] link_tracker, mass_mailing: simplify add_click method and add tests In this commit we rewrite a bit add_click in order to remove the inlined sudo and ease inheritance and parameter management inside the method. Access through controllers is sudo-ed as the main API method is now done with current user access rights. This commit is linked to task ID 1904277 and PR #28242. --- addons/link_tracker/controller/main.py | 7 +- addons/link_tracker/models/link_tracker.py | 41 +++++------- addons/mass_mailing/controllers/main.py | 7 +- .../mass_mailing/data/mass_mailing_demo.xml | 22 ++----- addons/mass_mailing/models/link_tracker.py | 39 ++++++----- addons/test_mass_mailing/tests/__init__.py | 1 + addons/test_mass_mailing/tests/test_link.py | 65 +++++++++++++++++++ 7 files changed, 122 insertions(+), 60 deletions(-) create mode 100644 addons/test_mass_mailing/tests/test_link.py diff --git a/addons/link_tracker/controller/main.py b/addons/link_tracker/controller/main.py index 3b23c005446..d884967b5a6 100644 --- a/addons/link_tracker/controller/main.py +++ b/addons/link_tracker/controller/main.py @@ -8,9 +8,14 @@ from odoo.http import request class LinkTracker(http.Controller): + @http.route('/r/', type='http', auth='none', website=True) def full_url_redirect(self, code, **post): country_code = request.session.geoip and request.session.geoip.get('country_code') or False - request.env['link.tracker.click'].sudo().add_click(code, request.httprequest.remote_addr, country_code, stat_id=False) + request.env['link.tracker.click'].sudo().add_click( + code, + ip=request.httprequest.remote_addr, + country_code=country_code + ) redirect_url = request.env['link.tracker'].get_url_from_code(code) return werkzeug.utils.redirect(redirect_url or '', 301) diff --git a/addons/link_tracker/models/link_tracker.py b/addons/link_tracker/models/link_tracker.py index c8f41c2eaca..7045e3ec3d2 100644 --- a/addons/link_tracker/models/link_tracker.py +++ b/addons/link_tracker/models/link_tracker.py @@ -240,32 +240,25 @@ class LinkTrackerClick(models.Model): ip = fields.Char(string='Internet Protocol') country_id = fields.Many2one('res.country', 'Country') - @api.model - def add_click(self, code, ip, country_code, stat_id=False): - self = self.sudo() - code_rec = self.env['link.tracker.code'].search([('code', '=', code)]) + def _prepare_click_values_from_route(self, **route_values): + click_values = dict((fname, route_values[fname]) for fname in self._fields if fname in route_values) + if not click_values.get('country_id') and route_values.get('country_code'): + click_values['country_id'] = self.env['res.country'].search([('code', '=', route_values['country_code'])], limit=1).id + return click_values - if not code_rec: + @api.model + def add_click(self, code, **route_values): + """ Main API to add a click on a link. """ + tracker_code = self.env['link.tracker.code'].search([('code', '=', code)]) + if not tracker_code: return None - again = self.search_count([('link_id', '=', code_rec.link_id.id), ('ip', '=', ip)]) + ip = route_values.get('ip', False) + existing = self.search_count(['&', ('link_id', '=', tracker_code.link_id.id), ('ip', '=', ip)]) + if existing: + return None - if not again: - self.create( - self._get_click_values_from_route(dict( - code=code, - ip=ip, - country_code=country_code, - stat_id=stat_id, - ))) + route_values['link_id'] = tracker_code.link_id.id + click_values = self._prepare_click_values_from_route(**route_values) - def _get_click_values_from_route(self, route_values): - code = self.env['link.tracker.code'].search([('code', '=', route_values['code'])], limit=1) - country = self.env['res.country'].search([('code', '=', route_values['country_code'])], limit=1) - - return { - 'link_id': code.link_id.id, - 'create_date': datetime.date.today(), - 'ip': route_values['ip'], - 'country_id': country.id, - } + return self.create(click_values) diff --git a/addons/mass_mailing/controllers/main.py b/addons/mass_mailing/controllers/main.py index 440def20462..3c75c7934a3 100644 --- a/addons/mass_mailing/controllers/main.py +++ b/addons/mass_mailing/controllers/main.py @@ -107,7 +107,12 @@ class MassMailController(http.Controller): # which mass_mailing doesn't depend on country_code = request.session.get('geoip', False) and request.session.geoip.get('country_code', False) - request.env['link.tracker.click'].sudo().add_click(code, request.httprequest.remote_addr, country_code, stat_id=stat_id) + request.env['link.tracker.click'].sudo().add_click( + code, + ip=request.httprequest.remote_addr, + country_code=country_code, + mail_stat_id=stat_id + ) return werkzeug.utils.redirect(request.env['link.tracker'].get_url_from_code(code), 301) @http.route('/mailing/blacklist/check', type='json', auth='none') diff --git a/addons/mass_mailing/data/mass_mailing_demo.xml b/addons/mass_mailing/data/mass_mailing_demo.xml index 215cb88bd7a..666d8050cac 100644 --- a/addons/mass_mailing/data/mass_mailing_demo.xml +++ b/addons/mass_mailing/data/mass_mailing_demo.xml @@ -210,49 +210,37 @@ - - - - + - - - - + - - - - + - + - - - - + diff --git a/addons/mass_mailing/models/link_tracker.py b/addons/mass_mailing/models/link_tracker.py index c7077191dab..2b20b007a4b 100644 --- a/addons/mass_mailing/models/link_tracker.py +++ b/addons/mass_mailing/models/link_tracker.py @@ -18,22 +18,27 @@ class LinkTrackerClick(models.Model): mass_mailing_id = fields.Many2one('mail.mass_mailing', string='Mass Mailing') mass_mailing_campaign_id = fields.Many2one('mail.mass_mailing.campaign', string='Mass Mailing Campaign') - @api.model - def add_click(self, code, ip, country_code, stat_id=False): - res = super(LinkTrackerClick, self).add_click(code, ip, country_code, stat_id=stat_id) - if stat_id: - stat_sudo = self.env['mail.mail.statistics'].sudo().browse(stat_id) - stat_sudo.set_opened() - stat_sudo.set_clicked() - return res + def _prepare_click_values_from_route(self, **route_values): + click_values = super(LinkTrackerClick, self)._prepare_click_values_from_route(**route_values) + + if click_values.get('mail_stat_id'): + stat_sudo = self.env['mail.mail.statistics'].sudo().browse(route_values['mail_stat_id']).exists() + if not stat_sudo: + click_values['mail_stat_id'] = False + else: + if not click_values.get('mass_mailing_campaign_id'): + click_values['mass_mailing_campaign_id'] = stat_sudo.mass_mailing_campaign_id.id + if not click_values.get('mass_mailing_id'): + click_values['mass_mailing_id'] = stat_sudo.mass_mailing_id.id - def _get_click_values_from_route(self, route_values): - click_values = super(LinkTrackerClick, self)._get_click_values_from_route(route_values) - if route_values['stat_id']: - mail_stat = self.env['mail.mail.statistics'].browse(route_values['stat_id']) - click_values['mail_stat_id'] = mail_stat.id - if mail_stat.mass_mailing_campaign_id: - click_values['mass_mailing_campaign_id'] = mail_stat.mass_mailing_campaign_id.id - if mail_stat.mass_mailing_id: - click_values['mass_mailing_id'] = mail_stat.mass_mailing_id.id return click_values + + @api.model + def add_click(self, code, **route_values): + click = super(LinkTrackerClick, self).add_click(code, **route_values) + + if click and click.mail_stat_id: + click.mail_stat_id.set_opened() + click.mail_stat_id.set_clicked() + + return click diff --git a/addons/test_mass_mailing/tests/__init__.py b/addons/test_mass_mailing/tests/__init__.py index 4283fdd0861..652f56e3078 100644 --- a/addons/test_mass_mailing/tests/__init__.py +++ b/addons/test_mass_mailing/tests/__init__.py @@ -4,6 +4,7 @@ from . import test_blacklist from . import test_blacklist_mixin from . import test_composer +from . import test_link from . import test_mail_auto_blacklist from . import test_mail_channel from . import test_mass_mailing diff --git a/addons/test_mass_mailing/tests/test_link.py b/addons/test_mass_mailing/tests/test_link.py new file mode 100644 index 00000000000..99819944d82 --- /dev/null +++ b/addons/test_mass_mailing/tests/test_link.py @@ -0,0 +1,65 @@ +# -*- coding: utf-8 -*- +# Part of Odoo. See LICENSE file for full copyright and licensing details. + +from odoo.tests.common import users +from odoo.addons.test_mass_mailing.tests import common + + +class TestLinkTracker(common.MassMailingCase): + + def setUp(self): + super(TestLinkTracker, self).setUp() + + self.link = self.env['link.tracker'].create({ + 'url': 'https://www.example.com' + }) + + self.click = self.env['link.tracker.click'].create({ + 'link_id': self.link.id, + 'ip': '100.00.00.00', + 'country_id': self.env.ref('base.fr').id, + }) + + def test_add_link(self): + code = self.link.code + self.assertEqual(self.link.count, 1) + + # click from a new IP should create a new entry + click = self.env['link.tracker.click'].sudo().add_click( + code, + ip='100.00.00.01', + country_code='BEL' + ) + self.assertEqual(click.ip, '100.00.00.01') + self.assertEqual(click.country_id, self.env.ref('base.be')) + self.assertEqual(self.link.count, 2) + + # click from same IP (even another country) does not create a new entry + click = self.env['link.tracker.click'].sudo().add_click( + code, + ip='100.00.00.01', + country_code='FRA' + ) + self.assertEqual(click, None) + self.assertEqual(self.link.count, 2) + + @users('marketing') + def test_add_link_mail_stat(self): + mailing = self.env['mail.mass_mailing'].create({'name': 'Test Mailing'}) + code = self.link.code + self.assertEqual(self.link.count, 1) + stat = self.env['mail.mail.statistics'].create({'mass_mailing_id': mailing.id}) + self.assertFalse(stat.opened) + self.assertFalse(stat.clicked) + + # click from a new IP should create a new entry and update stat when provided + click = self.env['link.tracker.click'].sudo().add_click( + code, + ip='100.00.00.01', + country_code='BEL', + mail_stat_id=stat.id + ) + self.assertEqual(self.link.count, 2) + self.assertEqual(click.mass_mailing_id, mailing) + self.assertTrue(stat.opened) + self.assertTrue(stat.clicked) From 7222f0ab53e4c60bc00e54f130e9700b921d9e3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Mon, 29 Oct 2018 15:34:43 +0000 Subject: [PATCH 3/4] [CLN] link_tracker: clean some code Purpose of this commit is to remove some unnecessary code in link tracker. * remove unnecessary code / parameters: * remove unused field click_date on click as create_date is already used for that purpose; This commit is linked to task ID 1904277 and PR #28242. --- addons/link_tracker/models/link_tracker.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/addons/link_tracker/models/link_tracker.py b/addons/link_tracker/models/link_tracker.py index 7045e3ec3d2..7c2b3c45f70 100644 --- a/addons/link_tracker/models/link_tracker.py +++ b/addons/link_tracker/models/link_tracker.py @@ -194,7 +194,7 @@ class LinkTracker(models.Model): return {'Error': "This filter doesn't exist."} @api.model - def get_url_from_code(self, code, context=None): + def get_url_from_code(self, code): code_rec = self.env['link.tracker.code'].sudo().search([('code', '=', code)]) if not code_rec: @@ -236,7 +236,6 @@ class LinkTrackerClick(models.Model): _description = "Link Tracker Click" link_id = fields.Many2one('link.tracker', 'Link', required=True, ondelete='cascade') - click_date = fields.Date(string='Create Date') ip = fields.Char(string='Internet Protocol') country_id = fields.Many2one('res.country', 'Country') From 6842d5cbd83da260748fbabdac955c9153d16a5b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Wed, 2 Jan 2019 12:16:18 +0000 Subject: [PATCH 4/4] [IMP] link_tracker: make code field required on link.tracker.code model Indeed it makes no sense to have void codes. This commit is linked to task ID 1904277 and PR #28242. --- addons/link_tracker/models/link_tracker.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/addons/link_tracker/models/link_tracker.py b/addons/link_tracker/models/link_tracker.py index 7c2b3c45f70..645171c8279 100644 --- a/addons/link_tracker/models/link_tracker.py +++ b/addons/link_tracker/models/link_tracker.py @@ -211,7 +211,7 @@ class LinkTrackerCode(models.Model): _name = "link.tracker.code" _description = "Link Tracker Code" - code = fields.Char(string='Short URL Code', store=True) + code = fields.Char(string='Short URL Code', required=True, store=True) link_id = fields.Many2one('link.tracker', 'Link', required=True, ondelete='cascade') _sql_constraints = [