vladimirs-git / fortigate-api

Python package for configuring Fortigate (Fortios) devices using REST API
Apache License 2.0
61 stars 18 forks source link

Logic simplification and unnecessary loop elimination for csrf token #29

Closed Christiandus closed 3 months ago

Christiandus commented 4 months ago

Currently the get token from cookies function has the following loop:

Current code ``` while True: # fortios < v7 cookie_name = "ccsrftoken" if cookies := [o for o in session.cookies if o and o.name == cookie_name]: break # fortios >= v7 cookie_name += "_" if cookies := [o for o in session.cookies if o and o.name.startswith(cookie_name)]: break raise ValueError("Invalid login credentials. Cookie 'ccsrftoken' is missing.") token = str(cookies[0].value).strip('"') return token ```

In my opinion the logic could be significantly simplified and the loop removed. I've tested it on 7.2.8 and it works flawlessly :) Let me know what you think! (Of course the tests would need to be adjusted as they currently check for the underscore as well)

My suggestion ``` cookie_prefix = "ccsrftoken" if cookies := [o for o in session.cookies if o and o.name.startswith(cookie_prefix)]: token = str(cookies[0].value).strip('"') return token else: raise ValueError("Invalid login credentials. Cookie 'ccsrftoken' is missing.") ```
vladimirs-git commented 4 months ago

Thank you for your interest in this project. Regarding your suggestion to remove the following ValueError tests: L103 L104

These tests aim to exclude cookies not related to authentication. I suppose that some cookies starting with ccsrftoken might be intended for other purposes besides authentication and I prefer to do them to eliminate unpredictable behavior. Perhaps my fears are unfounded.

Christiandus commented 4 months ago

When I checked the cookies set by the FGT there were only 2 cookies. The name of the other cookies starts with APSCOOKIE. As you're only reading cookies from the FGT response (and not other websites) I don't see any problem in simplifying the logic, in the end it's up to you though.

vladimirs-git commented 3 months ago

fixed in 2.0.2