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.

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

Do we support multi group alias for one group or not? #2825

Closed
pkking opened this issue Jul 28, 2023 · 2 comments
Closed

Do we support multi group alias for one group or not? #2825

pkking opened this issue Jul 28, 2023 · 2 comments

Comments

@pkking
Copy link
Contributor

pkking commented Jul 28, 2023

Opened the issue because im confused by these code:

        if fas_group not in flask.g.user.user_teams:
            raise InsufficientRightsException(
                "User '{}' doesn't have access to fas 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)
        db.session.commit()

        flask.flash(
            "FAS group {} is activated in the Copr under the alias {} "
            .format(fas_group, alias)
        )
        return flask.redirect(url_for(
            "groups_ns.list_projects_by_group", group_name=alias))

The code above will allow user submit more than one alias for a group, but if the group already has a alias, it just will always return it instead of create another alias, so the code behave is wired:

  1. if we allow multi alias for one group, the maybe we should make a new method get_group_by_fas_name_and_alias_or_create to both match the alias and group name
  2. if just one alias is allowed, the redirect should be using group.name instead of alias
@praiskup
Copy link
Member

There's just 1:1 mapping, and if form.validate_on_submit() does some checking. There's though a race condition that we should fix, perhaps by installing unique key

fas_name = db.Column(db.String(127))

@pkking
Copy link
Contributor Author

pkking commented Jul 29, 2023

Otherwise, the form.validate_on_submit() just check if the alias already existed, but if we try to add another alias for a group, it will try to redirect to /group/g/<new alias>/coprs which will return 404, i think we should just redirect to /group/g/<existed alias>/coprs.

@FrostyX FrostyX moved this from Needs triage to In Progress in CPT Kanban Jul 31, 2023
pkking added a commit to pkking/copr that referenced this issue Jul 31, 2023
When i was developing group supported in OIDC, i found some wired hehavior in group activated in fedora-copr#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: fedora-copr#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 <[email protected]>
pkking added a commit to pkking/copr that referenced this issue Aug 1, 2023
When i was developing group supported in OIDC, i found some wired hehavior in group activated in fedora-copr#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: fedora-copr#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 <[email protected]>
pkking added a commit to pkking/copr that referenced this issue Aug 1, 2023
When i was developing group supported in OIDC, i found some wired hehavior in group activated in fedora-copr#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: fedora-copr#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 <[email protected]>
pkking added a commit to pkking/copr that referenced this issue Aug 2, 2023
When i was developing group supported in OIDC, i found some wired hehavior in group activated in fedora-copr#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: fedora-copr#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 <[email protected]>
pkking added a commit to pkking/copr that referenced this issue Aug 2, 2023
When i was developing group supported in OIDC, i found some wired hehavior in group activated in fedora-copr#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: fedora-copr#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 <[email protected]>
pkking added a commit to pkking/copr that referenced this issue Aug 2, 2023
When i was developing group supported in OIDC, i found some wired hehavior in group activated in fedora-copr#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: fedora-copr#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 <[email protected]>
pkking added a commit to pkking/copr that referenced this issue Aug 2, 2023
When i was developing group supported in OIDC, i found some wired hehavior in group activated in fedora-copr#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: fedora-copr#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 <[email protected]>
pkking added a commit to pkking/copr that referenced this issue Aug 3, 2023
When i was developing group supported in OIDC, i found some wired hehavior in group activated in fedora-copr#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: fedora-copr#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 <[email protected]>
@nikromen nikromen moved this from In Progress to Done in CPT Kanban Aug 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants