Missing Reviewers group on PR breakes SQL Database

[sgabenov]

Description

We have 2 instances of gitea server, where we testing enabling LDAP Grout sync with organizations and teams. After we have enabled this config, when we open a PR we got 500 Error. On the SQL Database side logs show this error. DB restarts in cycle all the time on both instances.

How To reproduce this issue:

1. Create local Team1 in Organization1
2. Add LDAP_User1 to Local_Team1 (Users from LDAP already imported to gitea)
3. In Repo1 from Organization1 create a PR to master branch
4. Enable LDAP groups in gitea configuration, create new LDAP_Team2 in Organization1
5. Map LDAP group so LDAP_User1 is added to newly created LDAP_Team2
6. Check that LDAP_User1 is added to Local_Team1 and LDAP_Team2
7. Delete Local_Team1
8. Try to open PR from step "3", get 500 error

db_1      | 2023-03-27 06:51:01.641 UTC [7117] ERROR:  canceling statement due to user request
db_1      | 2023-03-27 06:51:01.641 UTC [7117] STATEMENT:  SELECT "id", "user_id", "repo_id", "status", "source", "issue_id", "commit_id", "comment_id", "updated_by", "created_unix", "updated_unix" FROM "notification" WHERE (user_id = $1) AND "status" IN ($2,$3) ORDER BY updated_unix DESC LIMIT 20
db_1      | 2023-03-27 07:35:02.300 UTC [1] LOG:  received smart shutdown request
db_1      | 2023-03-27 07:35:02.330 UTC [1] LOG:  background worker "logical replication launcher" (PID 33) exited with exit code 1
db_1      | 
db_1      | PostgreSQL Database directory appears to contain a database; Skipping initialization
db_1      | 
db_1      | 2023-03-27 07:35:41.908 UTC [1] LOG:  starting PostgreSQL 12.1 (Debian 12.1-1.pgdg100+1) on x86_64-pc-linux-gnu, compiled by gcc (Debian 8.3.0-6) 8.3.0, 64-bit
db_1      | 2023-03-27 07:35:41.908 UTC [1] LOG:  listening on IPv4 address "0.0.0.0", port 5432
db_1      | 2023-03-27 07:35:41.909 UTC [1] LOG:  listening on IPv6 address "::", port 5432
db_1      | 2023-03-27 07:35:41.911 UTC [1] LOG:  listening on Unix socket "/var/run/postgresql/.s.PGSQL.5432"
db_1      | 2023-03-27 07:35:41.931 UTC [27] LOG:  database system was interrupted; last known up at 2023-03-27 07:30:18 UTC
db_1      | 2023-03-27 07:35:41.979 UTC [27] LOG:  database system was not properly shut down; automatic recovery in progress
db_1      | 2023-03-27 07:35:41.982 UTC [27] LOG:  redo starts at 28/EFBF96E0
db_1      | 2023-03-27 07:35:41.993 UTC [27] LOG:  invalid record length at 28/EFCEBE90: wanted 24, got 0

db_1      | 2023-03-27 07:35:41.994 UTC [27] LOG:  redo done at 28/EFCEBE48
db_1      | 2023-03-27 07:35:42.038 UTC [1] LOG:  database system is ready to accept connections

The config of LDAP mapping

{
  "CN=emb,OU=Groups,OU=CO_Users,DC=my_org,DC=com": {
    "sandbox": [
      "ldap_embedded"
    ]
  },
  "CN=Leads,OU=Groups,OU=CO_Users,DC=my_org,DC=com": {
    "antiq": [
      "ldap_leads"
    ],
    "applications": [
      "ldap_leads"
    ],
    "libraries": [
      "ldap_leads"
    ],
    "autoqa": [
      "ldap_leads"
    ],
    "devops": [
      "ldap_leads"
    ],
    "docker": [
      "ldap_leads"
    ],
    "emb_conan": [
      "ldap_leads"
    ],
    "emb_devsec": [
      "ldap_leads"
    ],
    "emb_mirrors": [
      "ldap_leads"
    ],
    "release_activity": [
      "ldap_leads"
    ]
  },
  "CN=emb_devops,OU=Groups,OU=CO_Users,DC=my_org,DC=com": {
    "antiq": [
      "ldap_emb_devops"
    ],
    "applications": [
      "ldap_emb_devops"
    ],
    "libraries": [
      "ldap_emb_devops"
    ],
    "autoqa": [
      "ldap_emb_devops"
    ],
    "devops": [
      "ldap_emb_devops"
    ],
    "docker": [
      "ldap_emb_devops"
    ],
    "emb_conan": [
      "ldap_emb_devops"
    ],
    "emb_devsec": [
      "ldap_emb_devops"
    ],
    "emb_mirrors": [
      "ldap_emb_devops"
    ],
    "release_activity": [
      "ldap_emb_devops"
    ]
  },
  "CN=edt_devops,OU=Groups,OU=CO_Users,DC=my_org,DC=com": {
    "devops": [
      "ldap_edt_devops"
    ]
  },
  "CN=emb_kml,OU=Groups,OU=CO_Users,DC=my_org,DC=com": {
    "applications": [
      "ldap_emb_mail"
    ],
    "libraries": [
      "ldap_emb_mail"
    ],
    "emb_conan": [
      "ldap_emb_mail"
    ],
    "emb_mirrors": [
      "ldap_emb_mail"
    ],
    "release_activity": [
      "ldap_emb_mail"
    ]
  },
  "CN=dev_focus,OU=Groups,OU=CO_Users,DC=my_org,DC=com": {
    "applications": [
      "ldap_emb_focus"
    ],
    "libraries": [
      "ldap_emb_focus"
    ],
    "emb_conan": [
      "ldap_emb_focus"
    ],
    "emb_mirrors": [
      "ldap_emb_focus"
    ],
    "release_activity": [
      "ldap_emb_focus"
    ]
  },
  "CN=emb_antiq,OU=Groups,OU=CO_Users,DC=my_org,DC=com": {
    "antiq": [
      "ldap_emb_antiq"
    ],
    "libraries": [
      "ldap_emb_antiq"
    ],
    "emb_conan": [
      "ldap_emb_antiq"
    ],
    "emb_mirrors": [
      "ldap_emb_antiq"
    ]
  },
  "CN=emb_skm,OU=Groups,OU=CO_Users,DC=my_org,DC=com": {
    "applications": [
      "ldap_emb_squadus"
    ],
    "libraries": [
      "ldap_emb_squadus"
    ],
    "emb_conan": [
      "ldap_emb_squadus"
    ],
    "emb_mirrors": [
      "ldap_emb_squadus"
    ]
  },
  "CN=emb_qa,OU=Groups,OU=CO_Users,DC=my_org,DC=com": {
    "applications": [
      "ldap_emb_qa"
    ],
    "libraries": [
      "ldap_emb_qa"
    ],
    "autoqa": [
      "ldap_emb_qa"
    ],
    "release_activity": [
      "ldap_emb_qa"
    ]
  }
}

Gitea Version

1.19.0

Can you reproduce the bug on the Gitea demo site?

Yes

Log Gist

No response

Screenshots

No response

Git Version

No response

Operating System

No response

How are you running Gitea?

Docker

Database

PostgreSQL

[sgabenov]

Feels like deleting old Teams and creating new teams breaks the PR structure. Even if new team has all necessery access the PR is somehow asociates with old teams. Seems like a bug

[melmus]

I have the same issue too

[aralex]

Agree! It's a real problem!

[sgabenov]

I think the real problem is that PSs (Reviewers for PR) are mapped by TeamID in SQL database, so when you delete group "Team1" and add users with same privileges to another team, the PRs are still looking for previous TeamID in SQL.

1. This problem should not show 500 error, this should be handled and give proper reply, like "The TeamID in not found in DB, please check that some group is not deleted"
2. If reviewers groop is missing, it still should show the PRs (opened and already closed). At the moment it is 500 Error
3. There should be a possibility to restore access to PRs after the original Team is deleted. I can see, that even if i create a new Team with same name, the ID of this team is different and gitea checks on TeamID. This means i will loose access to PRs if i deleted the original team and could not revert it without editing DB, which is risky
4. It feels like DB structure should be modified to avoid this kind of problems. There should be linked tables. And you should not store it in one table "team"

[sillyguodong]

How To reproduce this issue:

Firstly, I tried to follow the steps you reported, but did not reproduce the "500 interval error".

I think the real problem is that PSs (Reviewers for PR) are mapped by TeamID in SQL database

If I assgin a team to a PR Reviewers, then I delete the team, and try to visit the PR, the backend will return 500.
So I think this issue is not related to LDAP, could you help me modify the title of the issue?

To fix the bug, the following issues need to be resloved:

1. Now that the organization's administrator has confirmed to delete the team, we should remove the PR/team binding (the row which review_team_id = ${team_id} and issue_id = ${pr's issue_id} in table review) when deleting team as well.
2. For undeleted binding rows in in table review: When we try to get reviewers for PR, if we find that PR binds a team that doesn't exist, we ignore it. (Or maybe we can delete these rows when executing migrations)
3. For comment: I think it should be preserved. We can append "deleted" to the team name after the team is deleted.