Fixing http error codes in security tutorials #5044
Conversation
|
📝 Docs preview for commit 27bf09c at: https://62ab6bfb9fe0210cb141f466--fastapi.netlify.app |
|
I'm sorry for the close and reopen. I wanted to revert this back to a draft PR |
There was a problem hiding this comment.
I am in agreement that some of the codes are wrong codes used in the tutorials and that it should be fixed. However, I am not sure that referencing to an Enum value would be beneficial for in the docs. Using int, it is inline with the signature of the init of HTTPException and might be easier to understand for people not experienced with FastAPI (which by definition are the majority of the people going through the documentation in the first place).
Also made comments where I disagree with your suggested code.
| @@ -119,7 +121,7 @@ async def login_for_access_token(form_data: OAuth2PasswordRequestForm = Depends( | |||
| raise HTTPException( | |||
| status_code=status.HTTP_401_UNAUTHORIZED, | |||
| detail="Incorrect username or password", | |||
| headers={"WWW-Authenticate": "Bearer"}, | |||
| headers={"WWW-Authenticate": "Basic"}, | |||
There was a problem hiding this comment.
This made me think for a bit, but I think I agree with you. Although we are using helper classes with OAuth2 stuff in them, it is basically a basic authentication. On the other hand, the username/password are following the Oauth2 standards (e.g. with the form) and not with base64 encoding (as is required by the Basic Authentication standard). So, it can be both ways I guess?
There was a problem hiding this comment.
Hmm I don't know. Somehow it's neither of the two options. I found this on this topic. I guess there is no simple soultion. Maybe just leave the WWW-Authenticate out for simplicities sake or leave as is?
Bearer is definitely false in my opinion.
There was a problem hiding this comment.
When using basic auth, the browser prompts you with a username/passwd form. This might not even work, as the browser will probably put the credentials into the header instead of the body. I didn't test this out yet
|
I think the error code constants make the code much more readable and show intent more clearly. Especially because new people are reading through the docs I think it's important to make constants the default. Also I'm not sure but I think in the docs the usage of constants and ints is rather inconsistent. |
docs_src/security/tutorial005.py
Outdated
| @@ -139,15 +134,21 @@ async def get_current_active_user( | |||
| current_user: User = Security(get_current_user, scopes=["me"]) | |||
| ): | |||
| if current_user.disabled: | |||
| raise HTTPException(status_code=400, detail="Inactive user") | |||
| raise HTTPException( | |||
| status_code=status.HTTP_400_BAD_REQUEST, detail="Inactive user" | |||
There was a problem hiding this comment.
Yes, 403 would be more appropriate imo.
Co-authored-by: Irfanuddin Shafi Ahmed <irfanudeen08@gmail.com>
|
I don't have time at this moment to write tests and apply all your suggestions. I will complete the PR once I have time again |
|
📝 Docs preview for commit 72cdde5 at: https://62ea4cd99d56ce1019f2dfbe--fastapi.netlify.app |
|
📝 Docs preview for commit 42536db at: https://63146aa3d8aeff43e433fcb1--fastapi.netlify.app |
The docs still have to be updated so the yellow marker is at the correct places.
Before I do this: Can somebody review if everything is more or less fine like this.
And I just noticed that tests still have to be written due to the error code changes
This PR addresses #4975