Refactor: replace manual auth checks with annotated dependency injection (as per fast api docos) - #66
Conversation
|
I apologise that it is a bigger pull request but mainly it is all just DRYing up user annotation and admin logic everywhere 🙏 |
| with open('export/csvBookExport' + date_time + '.csv', 'a') as f: | ||
| writer = csv.writer(f) | ||
| writer.writerows(bookData) | ||
| conn.close() |
There was a problem hiding this comment.
I think the f.close() that I had previously is necessary here. Otherwise there could be some changes that don't get written to the file properly.
(in contexts where we use aiofiles, sometimes we don't need to worry about closing files though, like further down on line 225)
There was a problem hiding this comment.
I have checked that f.close is not required within context i.e. under with
| "user": user, | ||
| "request": request, | ||
| } | ||
| return templates.TemplateResponse(request, "userManagement.html", context) |
There was a problem hiding this comment.
I was going to suggest sticking to the try/except format here, but this one should never fail unless I've made a mistake in the jinjia2 template. So actually this is better, because I'll just get the raw jinjia2 error.
(no change needed here)
| print(e) | ||
| return "Could not promote user." | ||
| finally: | ||
| db.close() |
There was a problem hiding this comment.
I totally missed closing a few db sessions around here, good catch.
(no change needed here)
|
|
||
| @router.post("/fedsearch", dependencies=[get_rate_limiter(times=1, seconds=5)], response_class=HTMLResponse) | ||
| async def fed_search_books(body: bytes = Depends(get_body)): | ||
| db = SessionLocal() |
There was a problem hiding this comment.
Looks like we're forgetting to close this one, needs a:
finally:
db.close()
There was a problem hiding this comment.
|
|
||
| @router.post("/fedBookDetails/", dependencies=[get_rate_limiter(times=1, seconds=5)], response_class=HTMLResponse) | ||
| async def fed_book_details(body: bytes = Depends(get_body)): | ||
| db = SessionLocal() |
There was a problem hiding this comment.
Also here, needs a:
finally:
db.close()
There was a problem hiding this comment.
Already there 😄 Does not show up in diff view L127-128
| # -------------------------------------------------------------------------- | ||
| @router.get("/downloadEbook/{filename}", dependencies=[get_rate_limiter(times=2, seconds=1)], response_class=HTMLResponse) | ||
| def download_ebook(request: Request, filename: str, user: schemas.User = Depends(get_current_user_from_token)): | ||
| def download_ebook(request: Request, filename: str, user: admin_user): |
There was a problem hiding this comment.
I think this should be current_user. Non-admin users should be able to download ebooks stored in ubiblio.
There was a problem hiding this comment.
Good call 😄 I added it due to the existing exception message. I will update the exception message 👍
| db = SessionLocal() | ||
| books = [] | ||
| withdrawnList = crud.browseWithdrawn(db) | ||
| for i in withdrawnList: |
There was a problem hiding this comment.
Hah! What was I thinking?
Your way is correct.
(you've already fixed this silly thing I did)
|
No worries about the big PR, that's what long weekends are for, and it legitimately needed a refactor. I think I found a couple of bugs, which I've raised above. I figured you'd like to fix them yourself, but I can do instead if you're busy. |
|
Fixed most comments and replied to others :) Thank you so much for the review |
|
OK, I think we're good. I'll merge this into dev, and mess with it a bit today. If OK, I might push to release later today. |
2 participants
Refactors authentication and authorisation across all routers to use FastAPI's annotated dependency
injection pattern, replacing manual if user.isAdmin checks and Depends(get_current_user_from_token)
calls throughout.
Other Minor changes:
SessionLocal() before try with finally: db.close()