Marzban icon indicating copy to clipboard operation
Marzban copied to clipboard

UserResponse: admin field added

Open Alirezaja1384 opened this issue 10 months ago • 6 comments

Details:

  • User responses ship with admin info now.
  • CRUD's user queries: Admin left join.

Alirezaja1384 avatar Apr 16 '24 21:04 Alirezaja1384

Fixes #910

Alirezaja1384 avatar Apr 16 '24 21:04 Alirezaja1384

nice pr but you returning all admin data in this case this can cause data leak , users can see there admin data with "/sub/{token}/info" request we just need username or id

M03ED avatar Apr 17 '24 14:04 M03ED

nice pr but you returning all admin data in this case this can cause data leak , users can see there admin data with "/sub/{token}/info" request we just need username or id

Thanks for your time. I wasn't aware of this (I usually use separate schemas for users and admin, so I didn't expect this 😅). In my opinion, the user shouldn't even be able to get the admin's username. A separated schema like AdminUserResponse or DetailedUserResponse with the admin field might be the way to go. Do you agree?

Alirezaja1384 avatar Apr 18 '24 13:04 Alirezaja1384

And since the non-sudo admins have no access to other admins' users, there would not be any risk of data leak cause it's the data that they should/and already have access to.

Alirezaja1384 avatar Apr 18 '24 13:04 Alirezaja1384

i think its better to have separated schema for /info request with less data some data like note sub_updated_at sub_last_user_agent online_at excluded_inbounds inbounds are unnecessary

M03ED avatar Apr 18 '24 13:04 M03ED

Since I don't like the pieces of code I wrote in the last commit, I chose to explain the changes. I know there are much better ways to handle this, but as far as I know, all of them require changes in User and UserResponse. Since UserResponse is highly integrated in every single piece of code (Which I don't like, but it is), any change that affects it can lead to unexpected bugs. So I chose to handle it in a way that has minimal side effects. Any suggestion is welcome as usual.

Alirezaja1384 avatar Apr 21 '24 19:04 Alirezaja1384

Hello, thanks for your efforts. just tested it, and it returns users list empty. Screenshot_2024-07-03_00-25-03

SaintShit avatar Jul 02 '24 20:07 SaintShit

Hello, thanks for your efforts. just tested it, and it returns users list empty. Screenshot_2024-07-03_00-25-03

Sounds to be fixed now. The join led to the exclusion of users who didn't have a db admin.

Alirezaja1384 avatar Jul 12 '24 12:07 Alirezaja1384

Thank you @Alirezaja1384, changes are great. I might change the way "admin" field is returning in future.

SaintShit avatar Jul 14 '24 08:07 SaintShit