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

Rest API invalid output compared to UI #27694

Closed
sgabenov opened this issue Oct 19, 2023 · 6 comments · Fixed by #30291
Closed

Rest API invalid output compared to UI #27694

sgabenov opened this issue Oct 19, 2023 · 6 comments · Fixed by #30291
Assignees
Labels
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented topic/api Concerns mainly the API topic/authentication type/bug

Comments

@sgabenov
Copy link

Description

Hi all,

The Gitea REST API {{baseUrl}}/repos/:owner/:repo/branch_protections/:name returns invalid output compared to the data that is shown in UI.
For instance, we have a repo with protected branch "master". Initially we have added several users for "merge_whitelist_usernames" with user "vasiliy.chipizhin" included into this list. Then this user was retired and we have deleted his account from "merge_whitelist_usernames". It was done several month ago and we have already restarted gitea instance since that time. In Gitea UI i do not see this user account in the whitelist section. But when i use REST API to get list of users, i do see this user account "vasiliy.chipizhin" in API respoce

Postman responce:
"vasiliy.chipizhin" account is presented
postman

Gitea UI:
"vasiliy.chipizhin" account is absent

master-1
master-2

Gitea Version

1.20.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

@lng2020

This comment was marked as outdated.

@lng2020

This comment was marked as outdated.

@sgabenov
Copy link
Author

The auth source is LDAP. Users are created automatically by cron sync with ldap

@lng2020

This comment was marked as outdated.

@lng2020 lng2020 added issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented topic/authentication labels Oct 23, 2023
@silverwind silverwind added the topic/api Concerns mainly the API label Mar 3, 2024
@edwardzhanged
Copy link
Contributor

That's possible. When the backend of API and web both return the same user IDs, API just looks up the usernames in db with those IDs and then returns them but the web frontend adds the para is_active=1 and then queries them. So a possible reproduction procedure: deactivate the user but not delete the user from the whitelist. Will fix it later. (with some tests)

I can't seem to find any is_active=1 parameter in the frontend logic. Here's what I've discovered:

  • Frontend logic: It renders the merge_whitelist_users from Users, while users are queried from getUsersWithAccessMode with a readAccess condition. Then, it renders the whitelisted user corresponding to the userIds from the protected_branch table.
  • Backend logic: It directly queries the protected_branch table and uses GetUserNamesByIDs to fill the whitelist usernames into the API response.

I suspect the root cause may be that LDAP downgraded the user's permission to 0 (Can LDAP do that?), or the admin manually changed the user's permission to unread. ( @sgabenov, could you confirm this please?) . This might be causing the frontend to fail to retrieve all the users.

I'm not certain if this issue should be labeled as a bug. According to GitLab documentation about deactivated user

When you deactivate a user, their projects, groups, and history remain.

However, it might be better for the backend API to find the whitelist users with a readAccess condition as well. This would unify the logic for both the backend and frontend.

Happy to provide a pr for that.

@lunny
Copy link
Member

lunny commented Apr 3, 2024

That's possible. When the backend of API and web both return the same user IDs, API just looks up the usernames in db with those IDs and then returns them but the web frontend adds the para is_active=1 and then queries them. So a possible reproduction procedure: deactivate the user but not delete the user from the whitelist. Will fix it later. (with some tests)

I can't seem to find any is_active=1 parameter in the frontend logic. Here's what I've discovered:

* Frontend logic:  It renders the `merge_whitelist_users` from `Users`, while users are queried from `getUsersWithAccessMode` with a readAccess condition. Then, it renders the whitelisted user corresponding to the userIds from the protected_branch table.

* Backend logic:  It directly queries the protected_branch table and uses `GetUserNamesByIDs` to fill the whitelist usernames into the API response.

I suspect the root cause may be that LDAP downgraded the user's permission to 0 (Can LDAP do that?), or the admin manually changed the user's permission to unread. ( @sgabenov, could you confirm this please?) . This might be causing the frontend to fail to retrieve all the users.

I'm not certain if this issue should be labeled as a bug. According to GitLab documentation about deactivated user

When you deactivate a user, their projects, groups, and history remain.

However, it might be better for the backend API to find the whitelist users with a readAccess condition as well. This would unify the logic for both the backend and frontend.

Happy to provide a pr for that.

Thank you for the contribution.

silverwind pushed a commit that referenced this issue Apr 17, 2024
…0291)

Add some logic in `convert.ToBranchProtection` to return only the names
associated with readAccess instead of returning all names. This will
ensure consistency in behavior between the frontend and backend.
Fixes: #27694

---------

Co-authored-by: techknowlogick <techknowlogick@gitea.com>
Co-authored-by: wenzhuo.zhang <wenzhuo.zhang@geely.com>
Co-authored-by: Giteabot <teabot@gitea.io>
GiteaBot added a commit to GiteaBot/gitea that referenced this issue Apr 17, 2024
…-gitea#30291)

Add some logic in `convert.ToBranchProtection` to return only the names
associated with readAccess instead of returning all names. This will
ensure consistency in behavior between the frontend and backend.
Fixes: go-gitea#27694

---------

Co-authored-by: techknowlogick <techknowlogick@gitea.com>
Co-authored-by: wenzhuo.zhang <wenzhuo.zhang@geely.com>
Co-authored-by: Giteabot <teabot@gitea.io>
silverwind pushed a commit that referenced this issue Apr 17, 2024
…0291) (#30544)

Backport #30291 by @edwardzhanged

Add some logic in `convert.ToBranchProtection` to return only the names
associated with readAccess instead of returning all names. This will
ensure consistency in behavior between the frontend and backend.
Fixes: #27694

Co-authored-by: Edward Zhang <45360012+edwardzhanged@users.noreply.github.com>
Co-authored-by: techknowlogick <techknowlogick@gitea.com>
Co-authored-by: wenzhuo.zhang <wenzhuo.zhang@geely.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented topic/api Concerns mainly the API topic/authentication type/bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants