From 7b8616e9d6b7dbba9ecc16e586cb64d38f2f4c99 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Brunner?= Date: Tue, 1 Oct 2024 16:51:44 +0200 Subject: [PATCH 1/3] Add display name in the user for better OpenID Connect support --- admin/tests/test_user.py | 4 ++++ commons/c2cgeoportal_commons/models/static.py | 11 +++++++++++ doc/integrator/authentication_oidc.rst | 3 ++- geoportal/c2cgeoportal_geoportal/__init__.py | 5 +++-- geoportal/c2cgeoportal_geoportal/lib/oidc.py | 6 +++++- .../geoportal/CONST_config-schema.yaml | 3 +++ geoportal/c2cgeoportal_geoportal/views/login.py | 7 ++++--- geoportal/tests/functional/test_login.py | 10 ++++++---- 8 files changed, 38 insertions(+), 11 deletions(-) diff --git a/admin/tests/test_user.py b/admin/tests/test_user.py index 7e1bde47e0..41b7b2e5b6 100644 --- a/admin/tests/test_user.py +++ b/admin/tests/test_user.py @@ -166,6 +166,7 @@ def test_submit_update(self, pw_gen_mock, smtp_mock, dbsession, test_app, users_ ("item_type", "user"), ("id", user.id), ("username", "new_name_withéàô"), + ("display_name", "New name withéàô"), ("email", "new_mail@valid.net"), ("settings_role_id", roles[2].id), ("__start__", "roles:sequence"), @@ -271,6 +272,7 @@ def test_submit_new(self, pw_gen_mock, smtp_mock, dbsession, test_app, users_tes ("item_type", "user"), ("id", ""), ("username", "new_user"), + ("display_name", "New user"), ("email", "valid@email.net"), ("settings_role_id", roles[2].id), ), @@ -284,6 +286,7 @@ def test_submit_new(self, pw_gen_mock, smtp_mock, dbsession, test_app, users_tes ).group(1) assert user.username == "new_user" + assert user.display_name == "New user" assert user.email == "valid@email.net" assert user.settings_role_id == roles[2].id assert user.password is not None and len(user.password) @@ -315,6 +318,7 @@ def test_invalid_email(self, test_app): "item_type": "user", "id": "", "username": "invalid_email", + "display_name": "Invalid email", "email": "new_mail", "role_name": "secretary_2", "is_password_changed": "false", diff --git a/commons/c2cgeoportal_commons/models/static.py b/commons/c2cgeoportal_commons/models/static.py index 5cd5f05054..c6b2aa0294 100644 --- a/commons/c2cgeoportal_commons/models/static.py +++ b/commons/c2cgeoportal_commons/models/static.py @@ -142,6 +142,15 @@ class User(Base): # type: ignore } }, ) + display_name: Mapped[str] = mapped_column( + Unicode, + info={ + "colanderalchemy": { + "title": _("Display name"), + "description": _("Name displayed in the application."), + } + }, + ) _password: Mapped[str] = mapped_column( "password", Unicode, nullable=False, info={"colanderalchemy": {"exclude": True}} ) @@ -263,8 +272,10 @@ def __init__( # nosec roles: list[Role] | None = None, expire_on: datetime | None = None, deactivated: bool = False, + display_name: str | None = None, ) -> None: self.username = username + self.display_name = display_name or username self.password = password self.tech_data = {} self.email = email diff --git a/doc/integrator/authentication_oidc.rst b/doc/integrator/authentication_oidc.rst index 593d64d809..4e9ed707cd 100644 --- a/doc/integrator/authentication_oidc.rst +++ b/doc/integrator/authentication_oidc.rst @@ -48,7 +48,8 @@ If we want to use OpenID Connect as an authentication provider, we need to set t url: client_id: user_info_fields: - username: name # Default value + username: sub # Default value + display_name: name # Default value email: email # Default value With that the user will be create in the database at the first login, and the access right will be set in the GeoMapFish database. diff --git a/geoportal/c2cgeoportal_geoportal/__init__.py b/geoportal/c2cgeoportal_geoportal/__init__.py index 7b684b9092..8a6bc6f12e 100644 --- a/geoportal/c2cgeoportal_geoportal/__init__.py +++ b/geoportal/c2cgeoportal_geoportal/__init__.py @@ -363,7 +363,8 @@ def get_user_from_request( ) request.user_ = oidc.DynamicUser( - username=user_info["username"], + username=user_info["sub"], + display_name=user_info["username"], email=user_info["email"], settings_role=( DBSession.query(Role).filter_by(name=user_info["settings_role"]).first() @@ -376,7 +377,7 @@ def get_user_from_request( ], ) else: - request.user_ = DBSession.query(User).filter_by(email=user_info["email"]).first() + request.user_ = DBSession.query(User).filter_by(username=user_info["sub"]).first() for user in DBSession.query(User).all(): _LOG.error(user.username) else: diff --git a/geoportal/c2cgeoportal_geoportal/lib/oidc.py b/geoportal/c2cgeoportal_geoportal/lib/oidc.py index 955eb0b3db..072030c482 100644 --- a/geoportal/c2cgeoportal_geoportal/lib/oidc.py +++ b/geoportal/c2cgeoportal_geoportal/lib/oidc.py @@ -51,6 +51,7 @@ class DynamicUser(NamedTuple): """ username: str + display_name: str email: str settings_role: main.Role | None roles: list[main.Role] @@ -89,6 +90,7 @@ class OidcRememberObject(TypedDict): refresh_token: str | None refresh_token_expires: str | None username: str | None + display_name: str | None email: str | None settings_role: str | None roles: list[str] @@ -144,6 +146,7 @@ def remember( ).isoformat() ), "username": None, + "display_name": None, "email": None, "settings_role": None, "roles": [], @@ -173,7 +176,8 @@ def remember( ) for field_, default_field in ( - ("username", "name"), + ("username", "sub"), + ("display_name", "name"), ("email", "email"), ("settings_role", None), ("roles", None), diff --git a/geoportal/c2cgeoportal_geoportal/scaffolds/update/{{cookiecutter.project}}/geoportal/CONST_config-schema.yaml b/geoportal/c2cgeoportal_geoportal/scaffolds/update/{{cookiecutter.project}}/geoportal/CONST_config-schema.yaml index e2acbb6a80..8763f55a15 100644 --- a/geoportal/c2cgeoportal_geoportal/scaffolds/update/{{cookiecutter.project}}/geoportal/CONST_config-schema.yaml +++ b/geoportal/c2cgeoportal_geoportal/scaffolds/update/{{cookiecutter.project}}/geoportal/CONST_config-schema.yaml @@ -243,6 +243,9 @@ mapping: type: map mapping: username: + type: str + default: sub + display_name: type: str default: name email: diff --git a/geoportal/c2cgeoportal_geoportal/views/login.py b/geoportal/c2cgeoportal_geoportal/views/login.py index 49645bfd4d..4c7f8e24b1 100644 --- a/geoportal/c2cgeoportal_geoportal/views/login.py +++ b/geoportal/c2cgeoportal_geoportal/views/login.py @@ -329,7 +329,7 @@ def _user(self, user: static.User | None = None) -> dict[str, Any]: if user is not None: result.update( { - "username": user.username, + "username": user.display_name, "email": user.email, "roles": [{"name": r.name, "id": r.id} for r in user.roles], } @@ -652,7 +652,8 @@ def oidc_callback(self) -> pyramid.response.Response: models.DBSession.add(user) else: user = oidc.DynamicUser( - username=remember_object["username"], + username=remember_object["sub"], + display_name=remember_object["username"], email=remember_object["email"], settings_role=( models.DBSession.query(main.Role).filter_by(name=remember_object["settings_role"]).first() @@ -681,7 +682,7 @@ def oidc_callback(self) -> pyramid.response.Response: # TODO respect the user interface... json.dumps( { - "username": user.username, + "username": user.display_name, "email": user.email, "is_intranet": is_intranet(self.request), "functionalities": self._functionality(), diff --git a/geoportal/tests/functional/test_login.py b/geoportal/tests/functional/test_login.py index 0fada98bec..516ee12fab 100644 --- a/geoportal/tests/functional/test_login.py +++ b/geoportal/tests/functional/test_login.py @@ -320,6 +320,7 @@ def __init__(self, name, functionalities): class U: username = "__test_user" + display_name = "Test User" is_password_changed = True email = "info@example.com" settings_role = None @@ -332,7 +333,7 @@ def __init__(self, role="__test_role", functionalities=None): request.user = U() login = Login(request) expected = { - "username": "__test_user", + "username": "Test User", "email": "info@example.com", "is_intranet": False, "login_type": "local", @@ -350,7 +351,7 @@ class F: del request.response.headers["Vary"] login = Login(request) expected = { - "username": "__test_user", + "username": "Test User", "email": "info@example.com", "is_intranet": False, "login_type": "local", @@ -392,6 +393,7 @@ def __init__(self, name, functionalities): class U: username = "__test_user" + display_name = "Test User" is_password_changed = True email = "info@example.com" @@ -407,13 +409,13 @@ def __init__(self, role="__test_role", functionalities=None): self.assertEqual( login.loginuser(), { + "username": "Test User", "email": "info@example.com", "functionalities": {}, "is_intranet": False, "login_type": "local", "roles": [{"id": 123, "name": "__test_role"}], "two_factor_enable": False, - "username": "__test_user", }, ) @@ -422,13 +424,13 @@ def __init__(self, role="__test_role", functionalities=None): self.assertEqual( login.loginuser(), { + "username": "Test User", "email": "info@example.com", "functionalities": {}, "is_intranet": True, "login_type": "local", "roles": [{"id": 123, "name": "__test_role"}], "two_factor_enable": False, - "username": "__test_user", }, ) From c501a05d728568635a85daccacf785fa927529a5 Mon Sep 17 00:00:00 2001 From: "geo-ghci-int[bot]" <146321879+geo-ghci-int[bot]@users.noreply.github.com> Date: Wed, 2 Oct 2024 15:07:51 +0000 Subject: [PATCH 2/3] Add Alembic upgrade script From the artifact of the previous workflow run --- ...dd_display_name_in_the_user_for_better_.py | 65 +++++++++++++++++++ 1 file changed, 65 insertions(+) create mode 100644 commons/c2cgeoportal_commons/alembic/static/267b4c1bde2e_add_display_name_in_the_user_for_better_.py diff --git a/commons/c2cgeoportal_commons/alembic/static/267b4c1bde2e_add_display_name_in_the_user_for_better_.py b/commons/c2cgeoportal_commons/alembic/static/267b4c1bde2e_add_display_name_in_the_user_for_better_.py new file mode 100644 index 0000000000..02ab45da09 --- /dev/null +++ b/commons/c2cgeoportal_commons/alembic/static/267b4c1bde2e_add_display_name_in_the_user_for_better_.py @@ -0,0 +1,65 @@ +# Copyright (c) 2024, Camptocamp SA +# All rights reserved. + +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are met: + +# 1. Redistributions of source code must retain the above copyright notice, this +# list of conditions and the following disclaimer. +# 2. Redistributions in binary form must reproduce the above copyright notice, +# this list of conditions and the following disclaimer in the documentation +# and/or other materials provided with the distribution. + +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED +# WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE +# DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR +# ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES +# (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; +# LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND +# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS +# SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +# The views and conclusions contained in the software and documentation are those +# of the authors and should not be interpreted as representing official policies, +# either expressed or implied, of the FreeBSD Project. + +""" +Add display name in the user for better OpenID Connect support. + +Revision ID: 267b4c1bde2e +Revises: aa41e9613256 +Create Date: 2024-10-02 14:33:59.185133 +""" + +import sqlalchemy as sa +from alembic import op +from c2c.template.config import config +from sqlalchemy.dialects import postgresql + +# revision identifiers, used by Alembic. +revision = "267b4c1bde2e" +down_revision = "aa41e9613256" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + """Upgrade.""" + config["schema"] + staticschema = config["schema_static"] + + # ### commands auto generated by Alembic - please adjust! ### + op.add_column("user", sa.Column("display_name", sa.Unicode(), nullable=False), schema=staticschema) + # ### end Alembic commands ### + + +def downgrade() -> None: + """Downgrade.""" + config["schema"] + staticschema = config["schema_static"] + + # ### commands auto generated by Alembic - please adjust! ### + op.drop_column("user", "display_name", schema=staticschema) + # ### end Alembic commands ### From b6812fc4d5e7a65f231ce9a2f798f3bb3ced0ce2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Brunner?= Date: Wed, 2 Oct 2024 17:26:11 +0200 Subject: [PATCH 3/3] Fill the display_name with the username --- ...c1bde2e_add_display_name_in_the_user_for_better_.py | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/commons/c2cgeoportal_commons/alembic/static/267b4c1bde2e_add_display_name_in_the_user_for_better_.py b/commons/c2cgeoportal_commons/alembic/static/267b4c1bde2e_add_display_name_in_the_user_for_better_.py index 02ab45da09..79ea773d88 100644 --- a/commons/c2cgeoportal_commons/alembic/static/267b4c1bde2e_add_display_name_in_the_user_for_better_.py +++ b/commons/c2cgeoportal_commons/alembic/static/267b4c1bde2e_add_display_name_in_the_user_for_better_.py @@ -47,19 +47,15 @@ def upgrade() -> None: """Upgrade.""" - config["schema"] staticschema = config["schema_static"] - # ### commands auto generated by Alembic - please adjust! ### - op.add_column("user", sa.Column("display_name", sa.Unicode(), nullable=False), schema=staticschema) - # ### end Alembic commands ### + op.add_column("user", sa.Column("display_name", sa.Unicode(), nullable=True), schema=staticschema) + op.execute(f'UPDATE {staticschema}."user" SET display_name = username') + op.alter_column("user", "display_name", nullable=False, schema=staticschema) def downgrade() -> None: """Downgrade.""" - config["schema"] staticschema = config["schema_static"] - # ### commands auto generated by Alembic - please adjust! ### op.drop_column("user", "display_name", schema=staticschema) - # ### end Alembic commands ###