Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 17 commits into
base: 14.0
Choose a base branch
from
+166 −0
Open

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

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
4884777
Added a mechanism to logout the user from the auth provider
wluyima Jun 10, 2024
a4360a4
Skip IDP logout if user has no IDP account
wluyima Jun 11, 2024
1344d3a
Fixed IDP redirect url to include base URL and logout URL to provider…
wluyima Jun 11, 2024
7e817fa
Skip IDP logout of no logout URL is configured for the provider
wluyima Jun 11, 2024
b7e96b9
Fixed logout for user with no IDP account
wluyima Jun 12, 2024
29e295b
Added support for params in logout URL and encoded post redirect URL
wluyima Jun 13, 2024
5c30529
Updated logout URL's help tip to mention it maps to end_session_endpoint
wluyima Jun 13, 2024
9401071
Added support for full URLs as redirects
wluyima Jun 13, 2024
6bd6a6b
Allow redirect URLs containing http or https but do not start with them
wluyima Jun 18, 2024
bb63584
Apply formatting changes
wluyima Jun 18, 2024
134417b
Renamed logout_endpoint to end_session_endpoint
wluyima Jul 15, 2024
abe4570
Switched to using urllib.parse.urljoin
wluyima Jul 15, 2024
ae387c4
Switched to using urllib to parse, create urls and query params
wluyima Jul 15, 2024
5acdf4a
Added initial test for oidc logout
wluyima Jul 18, 2024
59421ce
Added more tests for oidc logout
wluyima Jul 19, 2024
d59f8ce
Cleaned up test to verify individual redirect url components
wluyima Jul 19, 2024
8fc3df6
Use url_root instead of web base url
wluyima Aug 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions auth_oidc/controllers/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,10 @@

from werkzeug.urls import url_decode, url_encode

from odoo import http
from odoo.addons.auth_oauth.controllers.main import OAuthLogin
from odoo.addons.web.controllers.main import Session
from odoo.http import request

_logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -48,3 +51,28 @@
provider["auth_endpoint"], url_encode(params)
)
return providers


class OpenIDLogout(Session):

@http.route("/web/session/logout", type="http", auth="none")
def logout(self, redirect="/web/login"):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

user = request.env["res.users"].sudo().browse(request.session.uid)
if user.oauth_provider_id.id:
p = request.env["auth.oauth.provider"].sudo().browse(user.oauth_provider_id.id)

Check warning on line 62 in auth_oidc/controllers/main.py

View check run for this annotation

Codecov / codecov/patch

auth_oidc/controllers/main.py#L62 

Added line #L62 was not covered by tests
if p.logout_endpoint:
r = redirect

Check warning on line 64 in auth_oidc/controllers/main.py

View check run for this annotation

Codecov / codecov/patch

auth_oidc/controllers/main.py#L64 

Added line #L64 was not covered by tests
if r.find('http') == -1 and r.find('https') == -1:
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have sneaked in support for full URLs

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

r = request.env['ir.config_parameter'].sudo().get_param('web.base.url') + r
logout_base_url = p.logout_endpoint
params = {}

Check warning on line 68 in auth_oidc/controllers/main.py

View check run for this annotation

Codecov / codecov/patch

auth_oidc/controllers/main.py#L66-L68 

Added lines #L66 - L68 were not covered by tests
if '?' in p.logout_endpoint:
url_parts = p.logout_endpoint.split("?")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

logout_base_url = url_parts[0]
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)}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there documentation on how to run the test suite?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@sbidoul sbidoul Jul 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return super().logout(redirect=logout_url)

Check warning on line 76 in auth_oidc/controllers/main.py

View check run for this annotation

Codecov / codecov/patch

auth_oidc/controllers/main.py#L70-L76 

Added lines #L70 - L76 were not covered by tests
# User has no account with any provider or no logout URL is configured for the provider
return super().logout(redirect=redirect)
5 changes: 5 additions & 0 deletions auth_oidc/models/auth_oauth_provider.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,11 @@ 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(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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"
)

@tools.ormcache("self.jwks_uri", "kid")
def _get_keys(self, kid):
Expand Down
1 change: 1 addition & 0 deletions auth_oidc/views/auth_oauth_provider.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
<field name="validation_endpoint" position="after">
<field name="token_endpoint" />
<field name="jwks_uri" />
<field name="logout_endpoint" />
</field>
</field>
</record>
Expand Down
Loading