Skip to content

Commit

Permalink
frontend: group alias need 1:1 mapping to group
Browse files Browse the repository at this point in the history
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 tries 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

Fixes: #2825
Fixes: #2563
Signed-off-by: Li Chaoran <[email protected]>
  • Loading branch information
pkking authored and praiskup committed Aug 4, 2023
1 parent dfd210f commit bddee02
Show file tree
Hide file tree
Showing 6 changed files with 44 additions and 21 deletions.
Original file line number Diff line number Diff line change
@@ -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 = '7d9f6f921fa0'
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')
3 changes: 1 addition & 2 deletions frontend/coprs_frontend/coprs/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -2217,8 +2217,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):
Expand Down
7 changes: 1 addition & 6 deletions frontend/coprs_frontend/coprs/templates/_helpers.html
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,7 @@ <h3 class="panel-title"> {{ g.user.name | capitalize}} </h3>
<span class="badge">{{ g.user.coprs_count }}</span>
My projects
</a>
{% if config.FAS_LOGIN or config.LDAP_URL %}
{% if config.FAS_LOGIN or config.LDAP_URL or config.OIDC_LOGIN %}
<a href="{{url_for('groups_ns.list_user_groups') }}" class="list-group-item">
<span class="badge"> {{ user.user_groups|length }} </span>
My groups
Expand Down Expand Up @@ -458,11 +458,6 @@ <h3 class="panel-title">News - <a href="{{ config.NEWS_URL }}">Read All</a></h3>
{{- 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',
Expand Down
Original file line number Diff line number Diff line change
@@ -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 %}
<ol class="breadcrumb">
<li>
Expand Down
Original file line number Diff line number Diff line change
@@ -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 %}

Expand Down Expand Up @@ -37,7 +37,7 @@ <h1>
<tbody>
{% for team in teams %}
<tr>
<td><a href="{{ fas_group_href(team) }}">
<td><a href="{{ config.GROUP_INFO.link.format(name=team) }}">
{{ team }}
</a></td>
<td>
Expand Down
25 changes: 15 additions & 10 deletions frontend/coprs_frontend/coprs/views/groups_ns/groups_general.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -21,6 +22,15 @@
@groups_ns.route("/activate/<fas_group>", 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():
Expand All @@ -31,22 +41,17 @@ 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)
group = UsersLogic.create_group_by_fas_name(fas_group, alias)

db.session.add(group)
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(
Expand Down Expand Up @@ -85,7 +90,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
Expand Down

0 comments on commit bddee02

Please sign in to comment.