dpgaspar / Flask-AppBuilder

Simple and rapid application development framework, built on top of Flask. includes detailed security, auto CRUD generation for your models, google charts and much more. Demo (login with guest/welcome) - http://flaskappbuilder.pythonanywhere.com/
BSD 3-Clause "New" or "Revised" License
4.63k stars 1.35k forks source link

fix: updating user using FAB security api breaking user password hash #2179

Open Always-prog opened 8 months ago

Always-prog commented 8 months ago

Description

Updating a user using the FAB security API breaks the user’s password hash. This is because in the pre_update function of the user model, the item parameter is the user model. The check at this line is always true because the user already has a password. https://github.com/dpgaspar/Flask-AppBuilder/blob/59db85df13e5484ae24a7a9365986a15a7d9eb1f/flask_appbuilder/security/sqla/apis/user/api.py#L71-L72

I fixed it by moving checking password change to put endpoint of the user model.

Testing instructions

  1. Enable FAB_ADD_SECURITY_API in the config
  2. Get API token for making requests
    
    import requests
    import json

url = "http://localhost:8088/api/v1/security/login"

payload = json.dumps({ "password": "admin", "provider": "db", "refresh": True, "username": "admin" })

response = requests.request("POST", url, headers=headers, data=payload)

3. Change a role/first_name/last_name or any field in user model
```python
target_user_id = 2
url = f"http://localhost:8088/api/v1/security/users/{target_user_id}"

payload = json.dumps({
  "roles": [
    3,
    4
  ]
})
headers = {
  'Authorization': 'Bearer <token>',
  'Content-Type': 'application/json'
}

response = requests.request("PUT", url, headers=headers, data=payload)

After 3 step, target user is not able to login with her old password

ADDITIONAL INFORMATION

codecov[bot] commented 8 months ago

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 48.36%. Comparing base (59db85d) to head (bc6e81a). Report is 30 commits behind head on master.

Files Patch % Lines
flask_appbuilder/security/sqla/apis/user/api.py 0.00% 2 Missing :warning:

:exclamation: There is a different number of reports uploaded between BASE (59db85d) and HEAD (bc6e81a). Click for more details.

HEAD has 7 uploads less than BASE | Flag | BASE (59db85d) | HEAD (bc6e81a) | |------|------|------| |python|8|1|
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2179 +/- ## =========================================== - Coverage 79.31% 48.36% -30.96% =========================================== Files 72 72 Lines 8974 8699 -275 =========================================== - Hits 7118 4207 -2911 - Misses 1856 4492 +2636 ``` | [Flag](https://app.codecov.io/gh/dpgaspar/Flask-AppBuilder/pull/2179/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Daniel+Vaz+Gaspar) | Coverage Δ | | |---|---|---| | [python](https://app.codecov.io/gh/dpgaspar/Flask-AppBuilder/pull/2179/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Daniel+Vaz+Gaspar) | `48.36% <0.00%> (-30.96%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Daniel+Vaz+Gaspar#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Always-prog commented 8 months ago

I'll look why test cases failing

18191518054 commented 1 month ago

Is there a solution?