Master portal imp dve #17022

Icallhimtest · 2017-05-16T17:27:27Z

Odoo task: https://www.odoo.com/web#id=28689&view_type=form&model=project.task&menu_id=

Odoo pad: https://pad.odoo.com/p/openerp-project.task-V6R9WO4185

xmo-odoo · 2017-05-17T15:39:38Z

addons/auth_signup/controllers/main.py

@@ -38,8 +38,7 @@ def web_auth_signup(self, *args, **kw):
                if request.env["res.users"].sudo().search([("login", "=", qcontext.get("login"))]):
                    qcontext["error"] = _("Another user is already registered using this email address.")
                else:
-                    _logger.error("%s", e)
-                    qcontext['error'] = _("Could not create a new account.")
+                    qcontext['error'] = "%s", e


xmo-odoo · 2017-05-22T08:01:20Z

addons/auth_signup/models/res_partner.py

+            if partner.user_ids:
+                res[partner.id] = url_encode({'auth_login': partner.user_ids[0].login})
+            else:
+                continue


That doesn't seem very useful...

xmo-odoo · 2017-05-22T08:04:16Z

addons/website_portal_sale/models/account_invoice.py

+            url = '/mail/view?model=%s&res_id=%s' % (self._name, inv.id)
+            auth_param = inv.partner_id.url_get_user_login()
+            if auth_param:
+                url = url + '&' + auth_param[inv.partner_id.id]


Is there a reason for not using url_encode?

it's done above your "That doesn't seem very useful..." comment ^^

Well yeah but not here

Not sure I catch your drift.

Would you like me to create a {model: self._name, res_id: inv.id} dict and url_encode that ?

Or do you want to do the encoding of url_get_user_login() here and not in its method ?

Or both ?

Or both ?

Both sounds good, auth_param can just return the relevant information unencoded and the caller properly encodes everything at once.

xmo-odoo · 2017-05-22T08:04:42Z

addons/website_portal_sale/models/sale_order.py

+            url = '/mail/view?model=%s&res_id=%s&access_token=%s' % (self._name, so.id, so.access_token)
+            auth_param = so.partner_id.url_get_user_login()
+            if auth_param:
+                url = url + '&' + auth_param[so.partner_id.id]


See question above wrt url_encode.

xmo-odoo · 2017-05-22T08:05:42Z

addons/website_portal_sale/models/sale_order.py

+            self.env.cr.execute(query, (('draft', 'sent'),))
+            order_ids = self.env.cr.fetchall()
+
+            for order_id, in order_ids: # order_ids is a list of tuples e.g.: [(1,), (2,), ...]


I find that using a list literal is often clearer for single-element iterable unpacking e.g.

for [order_id] in order_ids:

xmo-odoo · 2017-05-22T08:09:53Z

addons/website_portal_sale/models/sale_order.py

+                default_value = default_compute(self)
+
+                query = 'UPDATE "%s" SET "%s"=%%s WHERE id = %s' % (
+                    self._table, column_name, order_id)


Why is the order_id not a query parameter?

Might be a good place to use executemany though I don't know if it works on Odoo's cursors.

You prefer this ?

query = 'UPDATE "%s" SET "%s"=%%s WHERE id = %%s' % ( self._table, column_name) self.env.cr.execute(query, (default_value, order_id))

I've copied this from elsewhere in the code, I'm not sure why we even do the query formatting in 2 steps.

Is there a reason we shouldn't just do ?

query = 'UPDATE "%s" SET "%s"=%s WHERE id = %s' % ( self._table, column_name, default_value, order_id) self.env.cr.execute(query)

Are the parameters from cr.execute() going through a safe_eval or so ?

Are the parameters from cr.execute() going through a safe_eval or so ?

No but query parameters are only for values, you can't use them for db identifier (table or column names for instance) you have to inject those by hand.

Here you'd be able to even hard-code them but I don't know what we usually do /cc @odony

Sorry, forgot to reply to this part. Making identifiers parameters is indeed not necessary, except for abstract classes like mail.thread where the code might need to refer to different tables. Elsewhere it makes the code noisy and error-prone. People tend to confuse the different kinds of parameters (identifiers who need careful injection vs value parameters that must be passed as query parameters to the execute method), and c/p mistakes. That was the case here. So let's keep things as simple as possible.
And have people learn the API inside out asasp ;-)

PS: @Icallhimtest out of curiosity, where did you originally copy this from? That place might need to be fixed as well ;-)

xmo-odoo · 2017-06-12T07:47:28Z

addons/website/models/res_config.py

+        }
+
+    def set_auth_signup_uninvited(self):
+        set_param = self.env['ir.config_parameter'].set_param


What's the point of this?

You forgot to ask the same question for get_param just above.

It's to avoid having a long line next (>100 characters). Okay, we uselessly define an extra variable, but it's more readable this way imo.

Why not just put a line break in the function call e.g.

self.env['ir.config_parameter'].set_param( 'auth_signup.allow_uninvited', repr(self.auth_signup_uninvited == 'b2c'))

?

thanks for the review of critical importance ! :D

odony

The modified signature for get_access_action make little sense to me. It's a general method available on BaseModel, and now suddenly it takes an optional parameter that may or may not be used to bypass credentials checks somehow.
That kind of business logic belongs to a controller, not a generic method on BaseModel, IMO.

odony · 2017-06-12T13:12:16Z

addons/website/models/res_config.py

+    @api.model
+    def get_default_auth_signup_uninvited(self, fields):
+        # we use safe_eval on the result, since the value of the parameter is a nonempty string
+        allow_uninvited = safe_eval(


Please fix this. As rule, you must not use safe_eval/eval unless its power is truly needed, i.e. when you need to evaluate arbitrary expressions within a sandbox. And then, with great care.
Whether your code is safe does not matter, it's a rule :-)
Not to mention that other parts of the code already use literal_eval (and even that is questionable, because frankly, value.lower() != "true" is quite enough for our purpose).

duly noted, I know a couple a places where I'll change this too.

odony · 2017-06-12T13:16:56Z

addons/auth_signup/models/res_partner.py

+        """
+        res = dict.fromkeys(self.ids, {})
+
+        allow_signup = safe_eval(self.env['ir.config_parameter'].get_param('auth_signup.allow_uninvited', 'False'))


Ditto. Seriously, do not import safe_eval unless you have a really really strong case for it. And then, be sure to have it thoroughly reviewed, because everything matters:

whether methods using safe_eval are public/private

all parameters taken by those methods

ACLs protecting the value/expression that will be evaluated

controllers that may access the methods, and their security

etc..

odony · 2017-06-12T13:24:58Z

addons/website_quote/models/sale_order.py

-            'target': 'self',
-            'res_id': self.id,
-        }
+        if self.template_id and (self.access_token == access_token or self.env.context.get('force_website')):


use consteq

odony · 2017-06-12T13:25:46Z

addons/website_portal_sale/models/sale_order.py

            try:
                self.check_access_rule('read')
            except exceptions.AccessError:
-                pass
+                if self.access_token == access_token:


Don't do sensitive string comparison with ==
Import consteq (from odoo.tools import consteq) then use consteq(correct_token, provided_token), to prevent timing attacks.

odony · 2017-06-12T13:35:27Z

addons/website_portal_sale/models/sale_order.py

+
+            Overridden here because we skip generating unique access tokens
+            for potentially tons of existing sales orders that won't need it.
+            We do, however, generate it for draft or quotation sales orders.


On the contrary, I think we do need to generate it for all existing sales orders, for security reasons. Please make sure it is the case.

odony · 2017-06-12T13:44:50Z

addons/website_portal_sale/models/sale_order.py

+
+                query = 'UPDATE "%s" SET "%s"=%%s WHERE id = %s' % (
+                    self._table, column_name, order_id)
+                self.env.cr.execute(query, (default_value,))


Looks like a lot of unnecessary roundtrips to the database.
If the goal is to generate a new random token for each row, how about a single query:

UPDATE res_users SET access_token = md5(md5(random()::varchar || id::varchar) || clock_timestamp()::varchar)::uuid::varchar WHERE access_token IS NULL;

It does not respect the UUID spec but is sufficiently random for our purposes, I think, and will be very fast even with hundreds of thousands of orders.

This commit duplicates the General settings' User Signup options in the Website configuration, with a clear description of the options. We also remove the unnecessary usage of safe_eval in auth_signup's res_config.py. As stored values are repr values of a boolean field just comparing the string values is sufficient. There is no need to use safe_eval as it should be used only when necessary.

Since 5425316 errors when signing up will only display two messages: * "Another user is already registered using this email address." or * "Could not create a new account." While Odoo creates a multitude of other comprehensible error messages such as * "Passwords do not match; please retype them." * "Signup token '%s' is no longer valid" This commit now separate UserError and AssertionError from SignupErrors. Those are still hidden in a general message (see 5425316 for reasons) while the other ones are fully displayed. This commit also improves translations of messages.

When you receive an url with parameters * auth_signup_token: uuid * auth_login: login those will be stored in the session and used * when the user will want to sign up in order to be linked to the right partner; * when he logs in so he's sure to log in with the right account + autofill is nice This commit only adds the support, future commits will support its use.

In order to be able to identify what action to return based on the access_user. Typically portal users will need to be redirected to the front-end, while internal users will need to be redirected to the back-end.

Mail now can handles a generic access_token in /mail/view route. Mail does not do anything with it. Addons can override the controller and add their specific management of this token according to some specific business logic. Sale order emails now contains the access token to grant access from the notification email url without logging in. Sale portal now allow customers to log in using an access token without having to use Online Quote. If the user doesn't have an account yet and signup is allowed (B2C) an extra parameter is added to the url to link the correct partner to the user upon signup. If the user already has an account an extra parameter is added to auto-fill the user's login if he wants to login from that session. Account is also updated to prepare accepting access tokens. However the complete implementation of accounting customer portal will be done in another task coming soon.

* allow new signup on invalid token * relabel signup buttons * send a welcome email upon signup with a signup token. This way, should the token somehow be usurped by someone else, the original partner's email address will be notified. (before the usurper can change the email)

We make the breadcrumbs of the portal uniform and remove them when we access the document with an access token without being logged in (for sales orders).

C3POdoo added the RD research & development, internal work label May 17, 2017

xmo-odoo reviewed May 17, 2017

View reviewed changes

Icallhimtest force-pushed the master-portal-imp-dve branch 5 times, most recently from ca4d80f to 6d25a17 Compare May 22, 2017 08:10

xmo-odoo requested changes May 22, 2017

View reviewed changes

Icallhimtest force-pushed the master-portal-imp-dve branch from 6d25a17 to 1ce47ad Compare May 22, 2017 09:11

Icallhimtest force-pushed the master-portal-imp-dve branch from 1ce47ad to a0b52c5 Compare June 9, 2017 13:55

xmo-odoo reviewed Jun 12, 2017

View reviewed changes

Icallhimtest force-pushed the master-portal-imp-dve branch from 9bd598c to 349de6d Compare June 12, 2017 08:30

odony requested changes Jun 12, 2017

View reviewed changes

Icallhimtest force-pushed the master-portal-imp-dve branch 3 times, most recently from d55d16b to 8f96c37 Compare June 19, 2017 13:46

Icallhimtest force-pushed the master-portal-imp-dve branch 7 times, most recently from 213e9f9 to b2432e7 Compare June 23, 2017 09:47

Icallhimtest force-pushed the master-portal-imp-dve branch from b2432e7 to 8c3638b Compare July 3, 2017 13:40

tde-banana-odoo mentioned this pull request Jul 5, 2017

Master portal imp dve tde #18004

Closed

Icallhimtest added 5 commits July 6, 2017 12:53

[IMP] models: add access_user to get_access_action

e0a1549

In order to be able to identify what action to return based on the access_user. Typically portal users will need to be redirected to the front-end, while internal users will need to be redirected to the back-end.

Icallhimtest added 2 commits July 6, 2017 12:53

[IMP] website_portal,*: clean breadcrumbs

505686e

We make the breadcrumbs of the portal uniform and remove them when we access the document with an access token without being logged in (for sales orders).

tde-banana-odoo force-pushed the master-portal-imp-dve branch from 8c3638b to 505686e Compare July 6, 2017 11:19

tde-banana-odoo merged commit 505686e into odoo:master Jul 6, 2017

tde-banana-odoo deleted the master-portal-imp-dve branch July 6, 2017 11:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Master portal imp dve #17022

Master portal imp dve #17022

Icallhimtest commented May 16, 2017

xmo-odoo May 17, 2017

xmo-odoo May 22, 2017

Icallhimtest May 22, 2017

xmo-odoo May 22, 2017

Icallhimtest May 22, 2017

xmo-odoo May 22, 2017

Icallhimtest May 22, 2017 •

edited

Loading

xmo-odoo May 22, 2017

xmo-odoo May 22, 2017

xmo-odoo May 22, 2017

Icallhimtest May 22, 2017

xmo-odoo May 22, 2017

Icallhimtest May 22, 2017

xmo-odoo May 22, 2017 •

edited

Loading

odony Jun 12, 2017

xmo-odoo Jun 12, 2017

Icallhimtest Jun 12, 2017

xmo-odoo Jun 12, 2017 •

edited

Loading

Icallhimtest Jun 12, 2017

Icallhimtest Jun 12, 2017

odony left a comment

odony Jun 12, 2017

Icallhimtest Jun 12, 2017

odony Jun 12, 2017

odony Jun 12, 2017

odony Jun 12, 2017

odony Jun 12, 2017

odony Jun 12, 2017

Master portal imp dve #17022

Master portal imp dve #17022

Conversation

Icallhimtest commented May 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Icallhimtest May 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xmo-odoo May 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xmo-odoo Jun 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odony left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Icallhimtest May 22, 2017 •

edited

Loading

xmo-odoo May 22, 2017 •

edited

Loading

xmo-odoo Jun 12, 2017 •

edited

Loading