From b1d1f4db825732ba0f571d4758d234ccb79ee08a Mon Sep 17 00:00:00 2001 From: Li Chaoran Date: Sat, 29 Jul 2023 18:00:44 +0800 Subject: [PATCH] frontend: group alias need 1:1 mapping to group When i was developing group supported in OIDC, i found some wired hehavior in group activated in #2825, So i submitted this PR, and @praiskup also give advice a TODO so i include the fix as well This PR try to fix: #2825 1. add unique contraint to fas_name and add migration script generated by alembic revision --autogenerate 2. I think macro fas_group_href canbe replaced by config.GROUP_INFO.link.forma which is more configurable 3. When creating a new alias for a group, if the group already has one alias, popup a flash and redirect to the already existed group Signed-off-by: Li Chaoran --- ...743d_add_unique_constraint_to_fas_group.py | 24 ++++++++++++++++ frontend/coprs_frontend/coprs/models.py | 3 +- .../coprs/templates/_helpers.html | 7 +---- .../coprs/templates/coprs/show/group.html | 2 +- .../templates/groups/user_fas_groups.html | 4 +-- .../coprs/views/groups_ns/groups_general.py | 28 +++++++++++-------- 6 files changed, 46 insertions(+), 22 deletions(-) create mode 100644 frontend/coprs_frontend/alembic/versions/daa62cd0743d_add_unique_constraint_to_fas_group.py diff --git a/frontend/coprs_frontend/alembic/versions/daa62cd0743d_add_unique_constraint_to_fas_group.py b/frontend/coprs_frontend/alembic/versions/daa62cd0743d_add_unique_constraint_to_fas_group.py new file mode 100644 index 000000000..7da6fdae7 --- /dev/null +++ b/frontend/coprs_frontend/alembic/versions/daa62cd0743d_add_unique_constraint_to_fas_group.py @@ -0,0 +1,24 @@ +""" +add unique constraint to fas_group + +Revision ID: daa62cd0743d +Revises: ba6ac0936bfb +Create Date: 2023-08-01 09:52:01.522171 +""" + +from alembic import op + + +# revision identifiers, used by Alembic. +revision = 'daa62cd0743d' +down_revision = 'ba6ac0936bfb' +branch_labels = None +depends_on = None + + +def upgrade(): + op.create_unique_constraint(None, 'group', ['fas_name']) + + +def downgrade(): + op.drop_constraint(None, 'group', type_='unique') diff --git a/frontend/coprs_frontend/coprs/models.py b/frontend/coprs_frontend/coprs/models.py index 038534b47..f33cbede6 100644 --- a/frontend/coprs_frontend/coprs/models.py +++ b/frontend/coprs_frontend/coprs/models.py @@ -2206,8 +2206,7 @@ class Group(db.Model, helpers.Serializer): id = db.Column(db.Integer, primary_key=True) name = db.Column(db.String(127)) - # TODO: add unique=True - fas_name = db.Column(db.String(127)) + fas_name = db.Column(db.String(127), unique=True) @property def at_name(self): diff --git a/frontend/coprs_frontend/coprs/templates/_helpers.html b/frontend/coprs_frontend/coprs/templates/_helpers.html index 317d82e74..3956dd6f2 100644 --- a/frontend/coprs_frontend/coprs/templates/_helpers.html +++ b/frontend/coprs_frontend/coprs/templates/_helpers.html @@ -304,7 +304,7 @@

{{ g.user.name | capitalize}}

{{ g.user.coprs_count }} My projects - {% if config.FAS_LOGIN or config.LDAP_URL %} + {% if config.FAS_LOGIN or config.LDAP_URL or config.OIDC_LOGIN %} {{ user.user_groups|length }} My groups @@ -458,11 +458,6 @@

News - Read All

{{- copr_url('coprs_ns.copr_package', package.copr, package_name=package.name) -}} {% endmacro %} -{%- macro fas_group_href(name) -%} -https://accounts.fedoraproject.org/group/{{name}} -{%- endmacro -%} - - {% macro repo_file_href(copr, repo, arch=None) %} {%- if not arch %} {{- owner_url('coprs_ns.generate_repo_file', diff --git a/frontend/coprs_frontend/coprs/templates/coprs/show/group.html b/frontend/coprs_frontend/coprs/templates/coprs/show/group.html index e7185d45c..6679298ab 100644 --- a/frontend/coprs_frontend/coprs/templates/coprs/show/group.html +++ b/frontend/coprs_frontend/coprs/templates/coprs/show/group.html @@ -1,7 +1,7 @@ {% extends "coprs/group_show.html" %} {% block title %}Project List{% endblock %} {% block header %}Project List{% endblock %} -{% from "_helpers.html" import render_pagination, fas_group_href %} +{% from "_helpers.html" import render_pagination %} {% block breadcrumbs %}
  1. diff --git a/frontend/coprs_frontend/coprs/templates/groups/user_fas_groups.html b/frontend/coprs_frontend/coprs/templates/groups/user_fas_groups.html index afb4abaea..e35192f4b 100644 --- a/frontend/coprs_frontend/coprs/templates/groups/user_fas_groups.html +++ b/frontend/coprs_frontend/coprs/templates/groups/user_fas_groups.html @@ -1,5 +1,5 @@ {% extends "layout.html" %} -{% from "_helpers.html" import fas_group_href, initialize_datatables %} +{% from "_helpers.html" import initialize_datatables %} {% block title %}Add a Group{% endblock %} {% block header %}Add a Group{% endblock %} @@ -37,7 +37,7 @@

    {% for team in teams %} - + {{ team }} diff --git a/frontend/coprs_frontend/coprs/views/groups_ns/groups_general.py b/frontend/coprs_frontend/coprs/views/groups_ns/groups_general.py index 43194250e..ddae34a8e 100644 --- a/frontend/coprs_frontend/coprs/views/groups_ns/groups_general.py +++ b/frontend/coprs_frontend/coprs/views/groups_ns/groups_general.py @@ -10,6 +10,7 @@ from coprs.logic.coprs_logic import CoprsLogic, PinnedCoprsLogic from coprs.logic.users_logic import UsersLogic from coprs import app +from coprs.oidc import oidc_enabled from ... import db from ..misc import login_required @@ -21,6 +22,15 @@ @groups_ns.route("/activate/", methods=["GET", "POST"]) @login_required def activate_group(fas_group): + msg_fmt = "Group {} is activated in the System under the alias {}" + + group = UsersLogic.get_group_by_fas_name(fas_group).first() + if group: + # can't have more than one alias for a group + flask.flash(msg_fmt.format(group.fas_name, group.name), "success") + return flask.redirect(url_for( + "groups_ns.list_projects_by_group", group_name=group.name)) + form = ActivateFasGroupForm() if form.validate_on_submit(): @@ -31,22 +41,18 @@ def activate_group(fas_group): if fas_group not in flask.g.user.user_teams: raise InsufficientRightsException( - "User '{}' doesn't have access to fas group {}" + "User '{}' doesn't have access to group {}" .format(flask.g.user.username, fas_group)) alias = form.name.data - group = UsersLogic.get_group_by_fas_name_or_create( - fas_group, alias) - - db.session.add(group) + group = UsersLogic.create_group_by_fas_name(fas_group, alias) + # db.session.add(group) is done by UsersLogic.create_group_by_fas_name() + db.session.flush() db.session.commit() - flask.flash( - "FAS group {} is activated in the Copr under the alias {} " - .format(fas_group, alias) - ) + flask.flash(msg_fmt.format(group.fas_name, group.name), "success") return flask.redirect(url_for( - "groups_ns.list_projects_by_group", group_name=alias)) + "groups_ns.list_projects_by_group", group_name=group.name)) else: return flask.render_template( @@ -85,7 +91,7 @@ def list_projects_by_group(group_name, page=1): @groups_ns.route("/list/my") @login_required def list_user_groups(): - if not (app.config['FAS_LOGIN'] or app.config['LDAP_URL']): + if not (app.config['FAS_LOGIN'] or app.config['LDAP_URL'] or oidc_enabled(app.config)): raise ObjectNotFound("Fedora Accounts or LDAP groups not enabled") teams = flask.g.user.user_teams