Skip to content

Commit

Permalink
[18.0][MIG] auth_saml: Migration to 18.0
Browse files Browse the repository at this point in the history
  • Loading branch information
BT-dlagin committed Jan 13, 2025
1 parent 1551708 commit df71b9e
Show file tree
Hide file tree
Showing 7 changed files with 124 additions and 44 deletions.
2 changes: 1 addition & 1 deletion auth_saml/__manifest__.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

{
"name": "SAML2 Authentication",
"version": "17.0.1.0.0",
"version": "18.0.1.0.0",
"category": "Tools",
"author": "XCG Consulting, Odoo Community Association (OCA)",
"maintainers": ["vincent-hatakeyama"],
Expand Down
35 changes: 19 additions & 16 deletions auth_saml/controllers/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,7 @@
exceptions,
http,
models,
)
from odoo import (
registry as registry_get,
modules,
)
from odoo.http import request
from odoo.tools.misc import clean_context
Expand Down Expand Up @@ -173,10 +171,9 @@ def _get_saml_extra_relaystate(self):
}
return state

@http.route("/auth_saml/get_auth_request", type="http", auth="none")
@http.route("/auth_saml/get_auth_request", type="http", auth="none", readonly=False)
def get_auth_request(self, pid):
provider_id = int(pid)

provider = request.env["auth.saml.provider"].sudo().browse(provider_id)
redirect_url = provider._get_auth_request(
self._get_saml_extra_relaystate(), request.httprequest.url_root.rstrip("/")
Expand All @@ -191,7 +188,9 @@ def get_auth_request(self, pid):
redirect.autocorrect_location_header = True
return redirect

@http.route("/auth_saml/signin", type="http", auth="none", csrf=False)
@http.route(
"/auth_saml/signin", type="http", auth="none", csrf=False, readonly=False
)
@fragment_to_query_string
def signin(self, **kw):
"""
Expand Down Expand Up @@ -241,16 +240,20 @@ def signin(self, **kw):
url = f"/#action={action}"
elif menu:
url = f"/#menu_id={menu}"
pre_uid = request.session.authenticate(*credentials)

credentials_dict = {
"login": credentials[1],
"token": credentials[2],
"type": "saml_token",
}
pre_uid = request.session.authenticate(dbname, credentials_dict)
resp = request.redirect(_get_login_redirect_url(pre_uid, url), 303)
resp.autocorrect_location_header = False
return resp

except exceptions.AccessDenied:
# saml credentials not valid,
# user could be on a temporary session
# saml credentials not valid, user could be on a temporary session
_logger.info("SAML2: access denied")

url = "/web/login?saml_error=expired"
redirect = werkzeug.utils.redirect(url, 303)
redirect.autocorrect_location_header = False
Expand All @@ -265,25 +268,25 @@ def signin(self, **kw):
redirect.autocorrect_location_header = False
return redirect

@http.route("/auth_saml/metadata", type="http", auth="none", csrf=False)
@http.route(
"/auth_saml/metadata", type="http", auth="none", csrf=False, readonly=False
)
def saml_metadata(self, **kw):
provider = kw.get("p")
dbname = kw.get("d")
valid = kw.get("valid", None)

if not dbname or not provider:
_logger.debug("Metadata page asked without database name or provider id")
return request.not_found(_("Missing parameters"))
raise request.not_found(_("Missing parameters"))

provider = int(provider)

registry = registry_get(dbname)

with registry.cursor() as cr:
with modules.registry.Registry(dbname).cursor() as cr:
env = api.Environment(cr, SUPERUSER_ID, {})
client = env["auth.saml.provider"].sudo().browse(provider)
if not client.exists():
return request.not_found(_("Unknown provider"))
raise request.not_found(_("Unknown provider"))

return request.make_response(
client._metadata_string(
Expand Down
2 changes: 1 addition & 1 deletion auth_saml/models/auth_saml_provider.py
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ def _compute_sp_metadata_url(self):
qs = urllib.parse.urlencode({"p": record.id, "d": self.env.cr.dbname})

record.sp_metadata_url = urllib.parse.urljoin(
base_url, f"/auth_saml/metadata?{qs}"
base_url, (f"/auth_saml/metadata?{qs}")
)

def _get_cert_key_path(self, field="sp_pem_public"):
Expand Down
19 changes: 12 additions & 7 deletions auth_saml/models/res_users.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

import passlib

from odoo import SUPERUSER_ID, _, api, fields, models, registry, tools
from odoo import SUPERUSER_ID, _, api, fields, models, modules, tools
from odoo.exceptions import AccessDenied, ValidationError

from .ir_config_parameter import ALLOW_SAML_UID_AND_PASSWORD
Expand Down Expand Up @@ -48,7 +48,7 @@ def _auth_saml_signin(self, provider: int, validation: dict, saml_response) -> s
if len(user) != 1:
raise AccessDenied()

with registry(self.env.cr.dbname).cursor() as new_cr:
with modules.registry.Registry(self.env.cr.dbname).cursor() as new_cr:
new_env = api.Environment(new_cr, self.env.uid, self.env.context)
# Update the token. Need to be committed, otherwise the token is not visible
# to other envs, like the one used in login_and_redirect
Expand Down Expand Up @@ -76,7 +76,7 @@ def auth_saml(self, provider: int, saml_response: str, base_url: str = None):
# return user credentials
return self.env.cr.dbname, login, saml_response

def _check_credentials(self, password, env):
def _check_credentials(self, credential, env):
"""Override to handle SAML auths.
The token can be a password if the user has used the normal form...
Expand All @@ -85,9 +85,10 @@ def _check_credentials(self, password, env):
"""
try:
# Attempt a regular login (via other auth addons) first.
return super()._check_credentials(password, env)

return super()._check_credentials(credential, env)
except (AccessDenied, passlib.exc.PasswordSizeError):
if not (credential["type"] == "saml_token" and credential["token"]):
raise
passwd_allowed = (
env["interactive"] or not self.env.user._rpc_api_keys_only()
)
Expand All @@ -100,12 +101,16 @@ def _check_credentials(self, password, env):
.search(
[
("user_id", "=", self.env.user.id),
("saml_access_token", "=", password),
("saml_access_token", "=", credential["token"]),
]
)
)
if token:
return
return {
"uid": self.env.user.id,
"auth_method": "saml",
"mfa": "default",
}
raise AccessDenied() from None

@api.model
Expand Down
96 changes: 84 additions & 12 deletions auth_saml/tests/test_pysaml.py
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ def test__compute_sp_metadata_url__provider_has_sp_baseurl(self):
{"p": self.saml_provider.id, "d": self.env.cr.dbname}
)
expected_url = urllib.parse.urljoin(
"http://example.com", f"/auth_saml/metadata?{expected_qs}"
"http://example.com", (f"/auth_saml/metadata?{expected_qs}")
)
# Assert that sp_metadata_url is set correctly
self.assertEqual(self.saml_provider.sp_metadata_url, expected_url)
Expand Down Expand Up @@ -200,7 +200,10 @@ def test_login_no_saml(self):

# Try to log in with a non-existing SAML token
with self.assertRaises(AccessDenied):
self.authenticate(user="[email protected]", password="test_saml_token")
self.user._check_credentials(
{"type": "password", "password": "test_saml_token"},
{"interactive": True},
)

redirect_url = self.saml_provider._get_auth_request()
self.assertIn("http://localhost:8000/sso/redirect?SAMLRequest=", redirect_url)
Expand Down Expand Up @@ -254,7 +257,10 @@ def test_login_with_saml(self):

# We should not be able to log in with the wrong token
with self.assertRaises(AccessDenied):
self.authenticate(user="[email protected]", password=f"{token}-WRONG")
self.user._check_credentials(
{"type": "password", "password": "WRONG_TOKEN"},
{"interactive": True},
)

# User should now be able to log in with the token
self.authenticate(user="[email protected]", password=token)
Expand All @@ -268,8 +274,9 @@ def test_disallow_user_password_when_changing_ir_config_parameter(self):
).value = "False"
# The password should be blank and the user should not be able to connect
with self.assertRaises(AccessDenied):
self.authenticate(
user="[email protected]", password="NesTNSte9340D720te>/-A"
self.user._check_credentials(
{"type": "password", "password": "NesTNSte9340D720te>/-A"},
{"interactive": True},
)

def test_disallow_user_password_new_user(self):
Expand Down Expand Up @@ -332,18 +339,19 @@ def test_disallow_user_password_no_password_set(self):
with self.assertRaises(ValidationError):
user.password = "new password"

def test_disallow_user_password(self):
def test_disallow_user_password_on_option_disable(self):
"""Test that existing user password is deleted when adding an SAML provider when
the disallow option is set."""
self.authenticate(user="[email protected]", password="Lu,ums-7vRU>0i]=YDLa")
# change the option
self.browse_ref(
"auth_saml.allow_saml_uid_and_internal_password"
).value = "False"
# Test that existing user password is deleted when adding an SAML provider
self.authenticate(user="[email protected]", password="Lu,ums-7vRU>0i]=YDLa")
self.add_provider_to_user()
with self.assertRaises(AccessDenied):
self.authenticate(user="[email protected]", password="Lu,ums-7vRU>0i]=YDLa")
self.user._check_credentials(
{"type": "password", "password": "Lu,ums-7vRU>0i]=YDLa"},
{"interactive": True},
)

def test_disallow_user_admin_can_have_password(self):
"""Test that admin can have its password set
Expand Down Expand Up @@ -417,6 +425,70 @@ def test_disallow_user_password_when_changing_settings(self):
).execute()

with self.assertRaises(AccessDenied):
self.authenticate(
user="[email protected]", password="NesTNSte9340D720te>/-A"
self.user._check_credentials(
{"type": "password", "password": "NesTNSte9340D720te>/-A"},
{"interactive": True},
)

def test_saml_metadata_invalid_provider(self):
"""Accessing SAML metadata with an invalid provider ID should return 404."""
response = self.url_open(f"/auth_saml/metadata?p=999999&d={self.env.cr.dbname}")
self.assertEqual(response.status_code, 404)
self.assertIn("Unknown provider", response.text)

def test_saml_metadata_missing_parameters(self):
"""Accessing the SAML metadata endpoint without params should return 404."""
response = self.url_open("/auth_saml/metadata")
self.assertEqual(response.status_code, 404)
self.assertIn("Missing parameters", response.text)

def test_saml_provider_deactivation(self):
"""A deactivated SAML provider should not be usable for authentication."""
self.saml_provider.active = False

redirect_url = self.saml_provider._get_auth_request()
response = self.idp.fake_login(redirect_url)
unpacked_response = response._unpack()

with self.assertRaises(AccessDenied):
self.env["res.users"].sudo().auth_saml(
self.saml_provider.id, unpacked_response.get("SAMLResponse"), None
)

def test_compute_sp_metadata_url_for_new_record(self):
"""Test that sp_metadata_url is set to False for a new (unsaved) provider."""
new_provider = self.env["auth.saml.provider"].new(
{"name": "New SAML Provider", "sp_baseurl": "http://example.com"}
)
new_provider._compute_sp_metadata_url()
self.assertFalse(new_provider.sp_metadata_url)

def test_store_outstanding_request(self):
"""Test that the SAML request ID is stored in the auth_saml.request model."""
reqid = "test-request-id"
self.saml_provider._store_outstanding_request(reqid)

request = self.env["auth_saml.request"].search([("saml_request_id", "=", reqid)])
self.assertTrue(request)
self.assertEqual(request.saml_provider_id.id, self.saml_provider.id)

def test_get_auth_request_redirect_url(self):
"""Test that _get_auth_request returns a valid redirect URL."""
redirect_url = self.saml_provider._get_auth_request()
self.assertIsNotNone(redirect_url)
self.assertIn("SAMLRequest=", redirect_url)

def test_fragment_to_query_string_empty_query(self):
"""Test fragment_to_query_string redirects when no query string is provided."""
response = self.url_open("/auth_saml/signin")
self.assertEqual(response.status_code, 200)
self.assertIn("<script>", response.text)

def test_get_auth_request_valid_provider(self):
"""Test that get_auth_request returns a redirect for a valid provider."""
response = self.url_open(
f"/auth_saml/get_auth_request?pid={self.saml_provider.id}", allow_redirects=False
)
self.assertEqual(response.status_code, 303)
self.assertIn("Location", response.headers)
self.assertIn("SAMLRequest=", response.headers["Location"])
10 changes: 5 additions & 5 deletions auth_saml/views/auth_saml.xml
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,12 @@
<field name="name">auth.saml.provider.list</field>
<field name="model">auth.saml.provider</field>
<field name="arch" type="xml">
<tree decoration-muted="not active">
<list decoration-muted="not active">
<field name="sequence" widget="handle" />
<field name="name" />
<field name="active" widget="boolean_toggle" />
<field name="autoredirect" />
</tree>
</list>
</field>
</record>
<record model="ir.ui.view" id="view_saml_provider_form">
Expand Down Expand Up @@ -168,10 +168,10 @@
name="attribute_mapping_ids"
context="{'default_provider_id': id}"
>
<tree editable="bottom">
<list editable="bottom">
<field name="attribute_name" />
<field name="field_name" widget="selection" />
</tree>
</list>
</field>
<p class="help small">
Mapped attributes are copied from the SAML response at every logon, if available. If multiple values are returned (i.e. a list) then the first value is used.
Expand All @@ -191,7 +191,7 @@
<record model="ir.actions.act_window" id="action_saml_provider">
<field name="name">Providers</field>
<field name="res_model">auth.saml.provider</field>
<field name="view_mode">tree,form</field>
<field name="view_mode">list,form</field>
</record>
<menuitem
id="menu_saml_providers"
Expand Down
4 changes: 2 additions & 2 deletions auth_saml/views/res_users.xml
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,10 @@
<page string="SAML">
<group>
<field name="saml_ids" nolabel="1" colspan="2">
<tree editable="bottom">
<list editable="bottom">
<field name="saml_provider_id" />
<field name="saml_uid" />
</tree>
</list>
</field>
</group>
</page>
Expand Down

0 comments on commit df71b9e

Please sign in to comment.