[FIX] auth_oauth: missing user in signin endpoint
Install auth_oauth and via the /web/login, click the "Log in using Odoo.com" button. You are redirected on odoo.com which ask you for your odoo.com login and password. When the login form on odoo.com is submited, you are redirected back on your local database. The problem is that, in case a new account was created on-the-fly, then the login fails with a cryptic error. The actual error is that `request.env.user._is_internal` fails because `user` is an empty recordset where it should had been the just-authenticated user. The problem is an inconsistent transaction state between the cursor of the request, the cursor used with `auth_oauth` (which created a new user) and the cursor used with `authenticate` (which authenticated the new user). Yes, there are 3 cursors. The newly created user just isn't present in the transaction of the request's cursor. Here is the lifetime of the 3 cursors: * request.env.cr, it begins when the http request enters Odoo, it is commited when a http response exits Odoo. * /auth_oauth/signin, it begins roughly at the beginning of the controller, it is commited once after the user is created (so before the authenticate transaction begins but AFTER the request transaction begun), it is commited again when the controller exits. * authenticate, begins when authenticate is called, is commited when it returns. Because the request transaction started before, it cannot access user created by /auth_oauth/signin. Because the route is `auth='none'`, if system administrators append the `auth_oauth` module via `--load` (cli) or `server_wide_modules` (odoorc) then the controller can be accessed without database. This is the reason for the explicit registry/cursor/environment inside this controller, we needed to make sure we are connected to a database, we cannot rely on request. The new approach used in this work is to benefit from `ensure_db()`, the function that is used by various web `auth='none'` controllers such as /web and /web/login. It makes sure that the database we want to connect to is already present on the request, otherwise it repeats the request but this time connecting it to the database. Using this approach we can have a `auth='none'` controller whose request.env is guaranteed to be connected on the right database. We can avoid to create explicit new registry/cursor/environment within the controller and just use request's ones. Because the /auth_oauth/signin controller now simply use the request transaction, the above point: > Because the request transaction started before, it cannot access user > created by /auth_oauth/signin. just doesn't stand anymore as the user is created within the same transaction. The extra `cr.commit()` must still be present for `authenticate` to see the newly created user. opw-3421701 closes odoo/odoo#138051 X-original-commit: e165568f9795af213c8467a7f17948ed781b0799 Signed-off-by: Olivier Dony (odo) <odo@odoo.com> Co-authored-by: Julien Castiaux <juc@odoo.com>
This commit is contained in:
Abdelouahab (abla)
committed by
Julien Castiaux
Julien Castiaux
co-authored by
Julien Castiaux
Julien Castiaux
parent
388f66368a
commit
5dd1efe3d8
@@ -123,51 +123,53 @@ class OAuthController(http.Controller):
|
||||
@fragment_to_query_string
|
||||
def signin(self, **kw):
|
||||
state = json.loads(kw['state'])
|
||||
|
||||
# make sure request.session.db and state['d'] are the same,
|
||||
# update the session and retry the request otherwise
|
||||
dbname = state['d']
|
||||
if not http.db_filter([dbname]):
|
||||
return BadRequest()
|
||||
ensure_db(db=dbname)
|
||||
|
||||
provider = state['p']
|
||||
context = clean_context(state.get('c', {}))
|
||||
registry = registry_get(dbname)
|
||||
with registry.cursor() as cr:
|
||||
try:
|
||||
env = api.Environment(cr, SUPERUSER_ID, context)
|
||||
db, login, key = env['res.users'].sudo().auth_oauth(provider, kw)
|
||||
cr.commit()
|
||||
action = state.get('a')
|
||||
menu = state.get('m')
|
||||
redirect = werkzeug.urls.url_unquote_plus(state['r']) if state.get('r') else False
|
||||
url = '/web'
|
||||
if redirect:
|
||||
url = redirect
|
||||
elif action:
|
||||
url = '/web#action=%s' % action
|
||||
elif menu:
|
||||
url = '/web#menu_id=%s' % menu
|
||||
request.update_context(**clean_context(state.get('c', {})))
|
||||
try:
|
||||
# auth_oauth may create a new user, the commit makes it
|
||||
# visible to authenticate()'s own transaction below
|
||||
_, login, key = request.env['res.users'].with_user(SUPERUSER_ID).auth_oauth(provider, kw)
|
||||
request.env.cr.commit()
|
||||
|
||||
pre_uid = request.session.authenticate(db, login, key)
|
||||
resp = request.redirect(_get_login_redirect_url(pre_uid, url), 303)
|
||||
resp.autocorrect_location_header = False
|
||||
action = state.get('a')
|
||||
menu = state.get('m')
|
||||
redirect = werkzeug.urls.url_unquote_plus(state['r']) if state.get('r') else False
|
||||
url = '/web'
|
||||
if redirect:
|
||||
url = redirect
|
||||
elif action:
|
||||
url = '/web#action=%s' % action
|
||||
elif menu:
|
||||
url = '/web#menu_id=%s' % menu
|
||||
|
||||
# Since /web is hardcoded, verify user has right to land on it
|
||||
if werkzeug.urls.url_parse(resp.location).path == '/web' and not request.env.user._is_internal():
|
||||
resp.location = '/'
|
||||
return resp
|
||||
except AttributeError:
|
||||
# auth_signup is not installed
|
||||
_logger.error("auth_signup not installed on database %s: oauth sign up cancelled." % (dbname,))
|
||||
url = "/web/login?oauth_error=1"
|
||||
except AccessDenied:
|
||||
# oauth credentials not valid, user could be on a temporary session
|
||||
_logger.info('OAuth2: access denied, redirect to main page in case a valid session exists, without setting cookies')
|
||||
url = "/web/login?oauth_error=3"
|
||||
redirect = request.redirect(url, 303)
|
||||
redirect.autocorrect_location_header = False
|
||||
return redirect
|
||||
except Exception as e:
|
||||
# signup error
|
||||
_logger.exception("OAuth2: %s" % str(e))
|
||||
url = "/web/login?oauth_error=2"
|
||||
pre_uid = request.session.authenticate(dbname, login, key)
|
||||
resp = request.redirect(_get_login_redirect_url(pre_uid, url), 303)
|
||||
resp.autocorrect_location_header = False
|
||||
|
||||
# Since /web is hardcoded, verify user has right to land on it
|
||||
if werkzeug.urls.url_parse(resp.location).path == '/web' and not request.env.user._is_internal():
|
||||
resp.location = '/'
|
||||
return resp
|
||||
except AttributeError: # TODO juc master: useless since ensure_db()
|
||||
# auth_signup is not installed
|
||||
_logger.error("auth_signup not installed on database %s: oauth sign up cancelled.", dbname)
|
||||
url = "/web/login?oauth_error=1"
|
||||
except AccessDenied:
|
||||
# oauth credentials not valid, user could be on a temporary session
|
||||
_logger.info('OAuth2: access denied, redirect to main page in case a valid session exists, without setting cookies')
|
||||
url = "/web/login?oauth_error=3"
|
||||
except Exception:
|
||||
# signup error
|
||||
_logger.exception("Exception during request handling")
|
||||
url = "/web/login?oauth_error=2"
|
||||
|
||||
redirect = request.redirect(url, 303)
|
||||
redirect.autocorrect_location_header = False
|
||||
|
||||
@@ -52,7 +52,7 @@ def clean_action(action, env):
|
||||
return cleaned_action
|
||||
|
||||
|
||||
def ensure_db(redirect='/web/database/selector'):
|
||||
def ensure_db(redirect='/web/database/selector', db=None):
|
||||
# This helper should be used in web client auth="none" routes
|
||||
# if those routes needs a db to work with.
|
||||
# If the heuristics does not find any database, then the users will be
|
||||
@@ -60,7 +60,8 @@ def ensure_db(redirect='/web/database/selector'):
|
||||
# If the db is taken out of a query parameter, it will be checked against
|
||||
# `http.db_filter()` in order to ensure it's legit and thus avoid db
|
||||
# forgering that could lead to xss attacks.
|
||||
db = request.params.get('db') and request.params.get('db').strip()
|
||||
if db is None:
|
||||
db = request.params.get('db') and request.params.get('db').strip()
|
||||
|
||||
# Ensure db is legit
|
||||
if db and db not in http.db_filter([db]):
|
||||
|
||||
@@ -1017,6 +1017,7 @@ class Session(collections.abc.MutableMapping):
|
||||
|
||||
self.should_rotate = True
|
||||
self.update({
|
||||
'db': env.registry.db_name,
|
||||
'login': login,
|
||||
'uid': uid,
|
||||
'context': user_context,
|
||||
|
||||
Reference in New Issue
Block a user