[14.0][IMP] auth_oidc: Logout the user from the auth provider #666

wluyima · 2024-06-11T17:12:21Z

Added a mechanism to logout the user from the auth provider when

… form

OCA-git-bot · 2024-06-11T17:12:28Z

Hi @sbidoul,
some modules you are maintaining are being modified, check this out!

ap-wtioit

Thanks for your efforts to make this module better. Could you run the OCA pre-commit or incoperate the suggestions from the log output: https://github.com/OCA/server-auth/actions/runs/9479497854/job/26118147534?pr=666

ap-wtioit · 2024-06-12T11:54:20Z

auth_oidc/controllers/main.py

+            p = request.env["auth.oauth.provider"].sudo().browse(user.oauth_provider_id.id)
+            if p.logout_endpoint:
+                r = request.env['ir.config_parameter'].sudo().get_param('web.base.url') + redirect
+                logout_url = p.logout_endpoint + "?client_id=" + p.client_id + "&post_logout_redirect_uri=" + r


the logout_endpoint may already contain query parameters:

end_session_endpoint REQUIRED. URL at the OP to which an RP can perform a redirect to request that the End-User be logged out at the OP. This URL MUST use the https scheme and MAY contain port, path, and query parameter components.

https://openid.net/specs/openid-connect-rpinitiated-1_0.html

So it would be better to not add a hardcocded questionmark here but use a more dynamic solution to adding parameters.

Also it seems suspicious that the redirect url r is not encoded here. (But i have not tested it yet)

Yes r should be encoded when added as post_logout_redirect_uri. (e.g. when redirect is /web/login?db=devel&something=different it would possibly end up as parameter something on the logout_endpoint)

Makes sense, I will update the PR

ap-wtioit · 2024-06-12T11:57:50Z

auth_oidc/models/auth_oauth_provider.py

@@ -46,6 +46,10 @@ class AuthOauthProvider(models.Model):
        string="Token URL", help="Required for OpenID Connect authorization code flow."
    )
    jwks_uri = fields.Char(string="JWKS URL", help="Required for OpenID Connect.")
+    logout_endpoint = fields.Char(
+        string="Logout URL",
+        help="Required for OpenID Connect to logout the user in the authorization provider upon logout in the client."


I guess this is expected to be the same value as end_session_endpoint from the provider discovery metadata, can you add a remark to the documentation that this is the value to be used.

Sounds good, I could also rename the property to end_session_endpoint

wluyima · 2024-06-13T11:54:58Z

auth_oidc/controllers/main.py

+        if user.oauth_provider_id.id:
+            p = request.env["auth.oauth.provider"].sudo().browse(user.oauth_provider_id.id)
+            if p.logout_endpoint:
+                r = request.env['ir.config_parameter'].sudo().get_param('web.base.url') + redirect


@ap-wtioit is redirect expected to always be relative to the base URL? If full URLs are supported then the line above would need to be updated to be optional if redirect starts with http or https

i think only relative redirects are used in odoo but there is no technical limit for that and the redirect paramter is sent directly to werkzeug: https://github.com/odoo/odoo/blob/14.0/addons/web/controllers/main.py#L1319

so yes that's a good catch and web.base.url should only be used when the url is not starting with a protocol (i guess http(s) will suffice), also there is the possibility that the redirect is relative to /web/session/logout (e.g. ../login, or destroy) but i think this is also not used currently (and i tested it only locally for checking security)

You mentioned destroy, I see it also logs out the user, do we need to override it too to logout the user in the IDP?

i can only see destroy being used in one Odoo Enterprise application "project_timesheet_synchro / Synchronization with the external timesheet application" and that code upon login does not support the things added by auth_oidc so i would skip this usecase here.

So is this good to go?

I'd like to test it again (most likely tomorrow) but yes this looks good to me now.

wluyima · 2024-06-13T13:26:45Z

auth_oidc/controllers/main.py

+            p = request.env["auth.oauth.provider"].sudo().browse(user.oauth_provider_id.id)
+            if p.logout_endpoint:
+                r = redirect
+                if r.find('http') == -1 and r.find('https') == -1:


I have sneaked in support for full URLs

Please use not r.startswith("http:") and not r.startswith("https:") here. the check you used would skip adding the base url for all redirects containing http.

ap-wtioit · 2024-06-18T11:35:13Z

Reviewed again, did you see my comment regarding oca-precommit? The check is still failing.

wluyima · 2024-06-18T15:11:29Z

@ap-wtioit when do you plan to have this merged?

Apply formatting changes Apply formatting changes

ap-wtioit · 2024-06-19T13:40:57Z

@ap-wtioit when do you plan to have this merged?

to me it looks good now. to get it merged we need a second approval, ideally from a PSC (Project Steering Committee member) or the maintainer of the module. then it can be merged (by the maintainer or PSC).

wluyima · 2024-06-20T10:21:19Z

@sbidoul do you mind becoming the second reviewer/approver on this PR?

sbidoul · 2024-06-27T08:22:48Z

I will look into it. Not had time yet.

wluyima · 2024-06-27T17:26:39Z

I see that this was added to the 14.0 milestone, it would be great to forward port the changes to later milestones too i.e. 15, 16 and 17

sbidoul

Hi,

Thanks for this contrib.

I made a few comments, the most important being the URL parsing logic that should use urllib.parse for robustness. I think this is important is such a security sensitive module.

Could you also add a test? The existing tests should give inspiration.

Head up about forward porting: I tested this on 16.0 and it will require some work to adapt, because Odoo now forces a local redirect (requests.redirect has a local=True argument):

https://github.com/odoo/odoo/blob/e889bd2c993a456d3077beaa3271f73e3510665e/addons/web/controllers/session.py#L85-L87

sbidoul · 2024-07-12T09:59:04Z

auth_oidc/models/auth_oauth_provider.py

@@ -46,6 +46,12 @@ class AuthOauthProvider(models.Model):
        string="Token URL", help="Required for OpenID Connect authorization code flow."
    )
    jwks_uri = fields.Char(string="JWKS URL", help="Required for OpenID Connect.")
+    logout_endpoint = fields.Char(


I'd suggest naming this field end_session_endpoint to match the field in the openid configuration document. This will make the code easier to understand.

sbidoul · 2024-07-12T10:01:41Z

auth_oidc/controllers/main.py

+                        request.env["ir.config_parameter"]
+                        .sudo()
+                        .get_param("web.base.url")
+                        + r


Let's use urllib.parse.urljoin instead of + here. It will be better in case web.base.url ends with a /.

sbidoul · 2024-07-12T10:20:34Z

auth_oidc/controllers/main.py

+                logout_base_url = p.logout_endpoint
+                params = {}
+                if "?" in p.logout_endpoint:
+                    url_parts = p.logout_endpoint.split("?")


I'm not too comfortable with parsing a URL this way. What if, for instance, there is more than 1 ? in the url. It should not happen of course, but... It would be more robust to use urllib.parse.urlparse and parse_qs.

sbidoul · 2024-07-12T10:21:37Z

auth_oidc/controllers/main.py

+                    params = url_decode(url_parts[1])
+                params["client_id"] = p.client_id
+                params["post_logout_redirect_uri"] = r
+                logout_url = f"{logout_base_url}?{url_encode(params)}"


It might be nice to extract this url manipulation logic in a little class method that could be unit tested.

Is there documentation on how to run the test suite?

@wluyima This part of the odoo documentation should get you started. Let me know if you hit any issue.

Thanks @sbidoul is there a way I can find documentation to run Odoo 14.0 locally from source? I was following the 15.0 documentation and I can't seem to install the packages in the requirements.txt file, I get this error when I try to install the packages, do you have any clues? I'm using Python 3.12.4. I'm not sure if is not compatible, is there documentation on Odoo and Python versions compatibility?

For Odoo 14 I generally use python 3.8. It will for sure not work with 3.12.

I recommend installing the Odoo dependencies in a virtual environment:

python3.8 -m venv ./venv source ./venv/bin/activate pip install -r https://raw.githubusercontent.com/odoo/odoo/14.0/requirements.txt

But if you don't use Odoo 14, why do you do this PR to the 14.0 branch?

Thanks @sbidoul! I did say we're using Odoo 14 which is why this PR is issued against the 14.0 branch and I'm installing the dependencies in a virtual environment but I was using python 3.12, I'll try with python 3.8 and see if it works. I was actually able to later find the documentation for Odoo 14 here.

sbidoul · 2024-07-12T10:23:26Z

auth_oidc/controllers/main.py

+    def logout(self, redirect="/web/login"):
+        user = request.env["res.users"].sudo().browse(request.session.uid)
+        if user.oauth_provider_id.id:
+            p = (


A little detail: let's use explicit variable names?

sbidoul · 2024-07-12T10:32:58Z

auth_oidc/models/auth_oauth_provider.py

+        string="Logout URL",
+        help="Required for OpenID Connect to logout the user in the authorization provider "
+        "upon logout in the client, should be the value of end_session_endpoint specified by "
+        "the authorization provider",


Shall we mention that when this field is set, logging out of Odoo also causes a complete logout of the identify provider?

sbidoul · 2024-07-12T11:02:50Z

auth_oidc/controllers/main.py

+
+class OpenIDLogout(Session):
+    @http.route("/web/session/logout", type="http", auth="none")
+    def logout(self, redirect="/web/login"):


Can you add a comment or docstring referencing the spec that this implements?
https://openid.net/specs/openid-connect-rpinitiated-1_0.html#RPLogout

sbidoul · 2024-07-17T06:21:42Z

auth_oidc/controllers/main.py

+            )
+            if provider.end_session_endpoint:
+                red_url = redirect
+                if not red_url.startswith("http:") and not red_url.startswith("https:"):


This test is now unnecessary, I think, because urljoin will do the right thing.

sbidoul · 2024-07-17T06:22:09Z

auth_oidc/controllers/main.py

+                .browse(user.oauth_provider_id.id)
+            )
+            if provider.end_session_endpoint:
+                red_url = redirect


Suggested change

red_url = redirect

post_logout_redirect_uri = redirect

wluyima · 2024-07-19T13:29:32Z

@sbidoul tests have been added

wluyima · 2024-08-01T13:26:54Z

@sbidoul @ap-wtioit tests were added, did you get a chance to take a look?

wluyima added 4 commits June 10, 2024 13:12

Added a mechanism to logout the user from the auth provider

4884777

Skip IDP logout if user has no IDP account

a4360a4

Fixed IDP redirect url to include base URL and logout URL to provider…

1344d3a

… form

Skip IDP logout of no logout URL is configured for the provider

7e817fa

Fixed logout for user with no IDP account

b7e96b9

ap-wtioit suggested changes Jun 12, 2024

View reviewed changes

Added support for params in logout URL and encoded post redirect URL

29e295b

wluyima commented Jun 13, 2024

View reviewed changes

wluyima added 2 commits June 13, 2024 14:56

Updated logout URL's help tip to mention it maps to end_session_endpoint

5c30529

Added support for full URLs as redirects

9401071

wluyima commented Jun 13, 2024

View reviewed changes

wluyima added 2 commits June 18, 2024 20:49

Allow redirect URLs containing http or https but do not start with them

6bd6a6b

Apply formatting changes

bb63584

Apply formatting changes Apply formatting changes

wluyima force-pushed the idp_logout branch from bf23dd2 to bb63584 Compare June 18, 2024 18:42

ap-wtioit approved these changes Jun 19, 2024

View reviewed changes

gurneyalex added this to the 14.0 milestone Jun 21, 2024

wtaferner approved these changes Jul 11, 2024

View reviewed changes

OCA-git-bot added the approved label Jul 11, 2024

sbidoul requested changes Jul 12, 2024

View reviewed changes

OCA-git-bot removed the approved label Jul 12, 2024

sbidoul changed the title ~~Logout the user from the auth provider~~ [14.0][IMP] auth_oidc: Logout the user from the auth provider Jul 12, 2024

wluyima added 2 commits July 15, 2024 14:41

Renamed logout_endpoint to end_session_endpoint

134417b

Switched to using urllib.parse.urljoin

abe4570

Switched to using urllib to parse, create urls and query params

ae387c4

sbidoul reviewed Jul 17, 2024

View reviewed changes

wluyima added 3 commits July 18, 2024 14:57

Added initial test for oidc logout

5acdf4a

Added more tests for oidc logout

59421ce

Cleaned up test to verify individual redirect url components

d59f8ce

Use url_root instead of web base url

8fc3df6

[14.0][IMP] auth_oidc: Logout the user from the auth provider #666

Are you sure you want to change the base?

[14.0][IMP] auth_oidc: Logout the user from the auth provider #666

Conversation

wluyima commented Jun 11, 2024

OCA-git-bot commented Jun 11, 2024

ap-wtioit 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

wluyima Jun 13, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ap-wtioit commented Jun 18, 2024

wluyima commented Jun 18, 2024

ap-wtioit commented Jun 19, 2024

wluyima commented Jun 20, 2024

sbidoul commented Jun 27, 2024

wluyima commented Jun 27, 2024

sbidoul 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

sbidoul Jul 16, 2024 • 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

wluyima commented Jul 19, 2024

wluyima commented Aug 1, 2024

wluyima Jun 13, 2024 •

edited

Loading

sbidoul Jul 16, 2024 •

edited

Loading