Codecov Report

Merging#2945 (3399de4) intomaster (982911f) willnot change coverage.
The diff coverage is100.00%.

@@            Coverage Diff            @@##            master     #2945   +/-   ##=========================================  Coverage   100.00%   100.00%           =========================================  Files          533       533             Lines        13746     13755    +9     =========================================+ Hits         13746     13755    +9

Help us with your feedback. Take ten seconds to tell ushow you rate us. Have a feature suggestion?Share it here.

📝 Docs preview for commit d8b63cdaebaf9c37b195bfa0bca54f3b9b0c8fa2 at:https://604c78b2648d6ca4039cc914--fastapi.netlify.app

I've also considered implementingDependant.cache_key as aproperty. But current fix is enough

• it keeps the designed made by the author to passscopes during__init__()
• Dependant is a class for internal use, andDependant.__init__() is only called few times in the source code

Updated once after tweaking the related test. Also rebased the branch to current master in this force push.

📝 Docs preview for commit3399de4 at:https://6304d5c087148506d673b746--fastapi.netlify.app

Thanks for the detailed explanation and for your contribution@laggardkernel! 🙇 🚀 🍰

Hi guys,
it looks that changes from this PR broke my privs checking mechanism.
Simplest code to demonstrate it is below:

fromfastapiimportFastAPI,Depends,Securityfromfastapi.securityimportSecurityScopesdefget_obj(required_scopes:SecurityScopes,):print(f'get_obj called with scopes: "{required_scopes.scope_str}"')return'object'defcheck_user_role_for_obj(required_scopes:SecurityScopes,obj=Depends(get_obj),):print(f'check_user_role_for_obj called for{obj} with scopes:, "{required_scopes.scope_str}"')return'object role'app=FastAPI()@app.get('/',dependencies=[Security(check_user_role_for_obj,scopes=['read_scope'])],)defget_obj_with_scopes(obj=Depends(get_obj),):returnobj@app.put('/',dependencies=[Security(check_user_role_for_obj,scopes=['edit_scope'])],)defput_obj_with_scopes(obj=Depends(get_obj),):returnobj@app.get('/child_obj',dependencies=[Security(check_user_role_for_obj,scopes=['read_child_scope'])],)defget_obj_with_scopes():return'child object'

Here is output from calling get_obj_with_scopes endpoint:

INFO:     127.0.0.1:58537 - "GET / HTTP/1.1" 200 OKget_obj called with scopes: "read_scope"check_user_role_for_obj called for object with scopes:, "read_scope"get_obj called with scopes: ""

as you can see get_obj was called twice....
I understand that this change was necessary as I was also suprised that I can not use different scopes for the same dependencies, but thought that it is by desing ;]
Maybe my understanding of this is not correct, but in such caseget_obj itself is only used withDepends, so no specific scopes are requested for it, so maybe it should be cached?
Especially that I don't even need/use required_scopes: SecurityScopes inget_obj function, so why differentiate cache key by it? Maybe it should depend if get_obj has SecurityScopes in its parameters list?

I am just throwing it for consideration.
As for now I will fix my FastAPI version to 0.79.1 and maybe I need to rethink how my privs model is designed.
Good Job guys!

Using a database session inside aSecurity andDepends also will open two sessions after this PR. Thus if the security method if returning a sqlalchemy the object return from the two methods will be attached to different sessions.

Hi@tiangolo,
maybe my case is kind specific to how I designed my privileges model, but creating multiple db sessions may lead to unexpected errors... I am not sure if I fully understand how dependency cache is working, but maybe it should be divided to 2 parts first scopes dependant and another not?
Can you please have a look on this?

To understand dependency cache, we may start from what features are added by FastAPI upon Starlette.

• When a request is received from the client at the server, it's received as bytes
• The ASGI server parse the bytes toenviron, recv(), send() (ASGI standard)
• Starlette abstract theenviron dict andrecv() as aRequest, which is a more developer-friendly Python object.

In the view/router function, we parse theRequest (e.g. it's query param, json body, form body, etc), get the info we want, and compose aResponse object.

FastAPI is capable to let developers declare the query param, or form, or json body as arguments of the view function. It helps us use the more finer part ofRequest directly without using the wholeRequest.

The introduction of dependency (Depends,Security) makesFastAPI more powerful. We can declare some composite value based on the primary data (query param, json body, form element, etc). e.g.

asyncdefsum_c(a:int=0,b:int=0):returna+b@app.get("/sum")asyncdefcalc_sum(c:int=Depends(sum_c)):returnc

In the above example, the view func doesn't accept query parama orb, but a composite param namedc, which is the sum ofa + b.

To avoid duplicated, high cost calculation in someDepends,FastAPI caches the result.

• ForDepends, the cache key is based on the func wrapped byDepends
• ForSecurity (subclass ofDepends), the cache key is based on the wrapped func and paramscopes. (Formerlyscopes are ignored cause a bug in the implementation, and this PR fixed it.)

nparley qmorekFor your questions, you may separate db session initialization out of existingSecuritys as a standaloneDepends, and re-used it as an argument in the existing multipleSecuritys.
This makes the newDepends, namely db session initialization process, has its own cache key, being executed only once, and not affected byscopes ofSecurity anymore. (Nope. I forgot thatscopes is inherited in the dependency chain. TwoDepends under two differentSecurity have different cache key.)

Hi@laggardkernel,
thank you very much for your answer.

could you eplain what you mean by:

you may separate db session initialization out of existing Securitys as a standalone Depends, and re-used it as an argument in the existing multiple Securitys.

I took example from:https://fastapi.tiangolo.com/advanced/security/oauth2-scopes/#dependency-tree-and-scopes
and modified it slightly (paswords are in cleartext, but it is not important here) with db dependency as in:
https://github.com/tiangolo/full-stack-fastapi-postgresql/blob/master/%7B%7Bcookiecutter.project_slug%7D%7D/backend/app/app/api/deps.py
and
https://github.com/tiangolo/full-stack-fastapi-postgresql/blob/master/%7B%7Bcookiecutter.project_slug%7D%7D/backend/app/app/api/api_v1/endpoints/items.py

fromtypingimportDictfromdatetimeimportdatetime,timedeltafromtypingimportList,UnionfromfastapiimportDepends,FastAPI,HTTPException,Security,statusfromfastapi.securityimport (OAuth2PasswordBearer,OAuth2PasswordRequestForm,SecurityScopes,)fromjoseimportJWTError,jwtfrompydanticimportBaseModel,ValidationError# to get a string like this run:# openssl rand -hex 32SECRET_KEY="09d25e094faa6ca2556c818166b7a9563b93f7099f6f0f4caa6cf63b88e8d3e7"ALGORITHM="HS256"ACCESS_TOKEN_EXPIRE_MINUTES=30fake_dict_db= {"users": {"johndoe": {"username":"johndoe","full_name":"John Doe","email":"johndoe@example.com","cleartext_password":"password","disabled":False,        },"alice": {"username":"alice","full_name":"Alice Chains","email":"alicechains@example.com","cleartext_password":"password","disabled":True,        },    },"items": [        {"name":"First"        },        {"name":"Second"        }    ]}classToken(BaseModel):access_token:strtoken_type:strclassTokenData(BaseModel):username:Union[str,None]=Nonescopes:List[str]= []classUser(BaseModel):username:stremail:Union[str,None]=Nonefull_name:Union[str,None]=Nonedisabled:Union[bool,None]=NoneclassUserInDB(User):cleartext_password:stroauth2_scheme=OAuth2PasswordBearer(tokenUrl="token",scopes={"me":"Read information about the current user.","items":"Read items."},)app=FastAPI()defverify_password(plain_password,db_password):returnplain_password==db_passworddefget_user(db,username:str):users_db=db['users']ifusernameinusers_db:user_dict=users_db[username]returnUserInDB(**user_dict)defauthenticate_user(fake_db,username:str,password:str):user=get_user(fake_db,username)ifnotuser:returnFalseifnotverify_password(password,user.cleartext_password):returnFalsereturnuserdefcreate_access_token(data:dict,expires_delta:Union[timedelta,None]=None):to_encode=data.copy()ifexpires_delta:expire=datetime.utcnow()+expires_deltaelse:expire=datetime.utcnow()+timedelta(minutes=15)to_encode.update({"exp":expire})encoded_jwt=jwt.encode(to_encode,SECRET_KEY,algorithm=ALGORITHM)returnencoded_jwtdefget_db()->Dict:print('get_db called')returnfake_dict_dbasyncdefget_current_user(security_scopes:SecurityScopes,db:Dict=Depends(get_db),token:str=Depends(oauth2_scheme),):print('required scopes are:',security_scopes.scope_str)ifsecurity_scopes.scopes:authenticate_value=f'Bearer scope="{security_scopes.scope_str}"'else:authenticate_value=f"Bearer"credentials_exception=HTTPException(status_code=status.HTTP_401_UNAUTHORIZED,detail="Could not validate credentials",headers={"WWW-Authenticate":authenticate_value},    )try:payload=jwt.decode(token,SECRET_KEY,algorithms=[ALGORITHM])username:str=payload.get("sub")ifusernameisNone:raisecredentials_exceptiontoken_scopes=payload.get("scopes", [])token_data=TokenData(scopes=token_scopes,username=username)except (JWTError,ValidationError):raisecredentials_exceptionuser=get_user(db,username=token_data.username)ifuserisNone:raisecredentials_exceptionforscopeinsecurity_scopes.scopes:ifscopenotintoken_data.scopes:raiseHTTPException(status_code=status.HTTP_401_UNAUTHORIZED,detail="Not enough permissions",headers={"WWW-Authenticate":authenticate_value},            )returnuserasyncdefget_current_active_user(current_user:User=Security(get_current_user,scopes=["me"])):ifcurrent_user.disabled:raiseHTTPException(status_code=400,detail="Inactive user")returncurrent_user@app.post("/token",response_model=Token)asyncdeflogin_for_access_token(db:Dict=Depends(get_db),form_data:OAuth2PasswordRequestForm=Depends(),):user=authenticate_user(db,form_data.username,form_data.password)ifnotuser:raiseHTTPException(status_code=400,detail="Incorrect username or password")access_token_expires=timedelta(minutes=ACCESS_TOKEN_EXPIRE_MINUTES)access_token=create_access_token(data={"sub":user.username,"scopes":form_data.scopes},expires_delta=access_token_expires,    )return {"access_token":access_token,"token_type":"bearer"}@app.get("/users/me/",response_model=User)asyncdefread_users_me(current_user:User=Depends(get_current_active_user)):returncurrent_user@app.get("/users/me/items/")asyncdefread_own_items(db:Dict=Depends(get_db),current_user:User=Security(get_current_active_user,scopes=["items"])):return {'items':db['items'],'current_user':current_user    }@app.get("/status/")asyncdefread_system_status(current_user:User=Depends(get_current_user)):return {"status":"ok"}

if you run this example code and call endpoint: /users/me/items/

you will see in console:

get_db calledget_db calledrequired scopes are: items me

so as you see get_db dependency is called twice...

Maybe my modifications are not correct, but it consistent with project template creted by tiangolo:https://github.com/tiangolo/full-stack-fastapi-postgresql
so in /users/me/items/ endpoint I added db dependency Depends(get_db) as I need to get items from fake db...

I am aware that it was created before this bug was discovered, so may also not be correct in this matter, but I would love to know how to do this correctly....

@qmorek Sorry, my former solution is wrong. I forgot something, thescopes is inherited by children in the dependency tree. In your example,

@app.get("/users/me/items/")asyncdefread_own_items(db:Dict=Depends(get_db),current_user:User=Security(get_current_active_user,scopes=["items"])):asyncdefget_current_active_user(current_user:User=Security(get_current_user,scopes=["me"])):asyncdefget_current_user(security_scopes:SecurityScopes,db:Dict=Depends(get_db),token:str=Depends(oauth2_scheme),):

Depends(get_db) param inread_own_items() andDepend(get_db) fromget_current_user() <- get_current_active_user() <- read_own_items() has different cache keys. The later inherits scopes"items" and"me" as part of the cache key. It makesget_db() executed twice during one request.

get_db called: 2dependency_cache: {(<function get_db at 0x10eeebf70>, ()): {'items': [{'name': 'First'},                                                    {'name': 'Second'}],                                          'users': {'alice': {'cleartext_password': 'password',                                                              'disabled': True,                                                              'email': 'alicechains@example.com',                                                              'full_name': 'Alice '                                                                           'Chains',                                                              'username': 'alice'},                                                    'johndoe': {'cleartext_password': 'password',                                                                'disabled': False,                                                                'email': 'johndoe@example.com',                                                                'full_name': 'John '                                                                             'Doe',                                                                'username': 'johndoe'}}}, (<function get_db at 0x10eeebf70>, ('items', 'me')): {'items': [{'name': 'First'},                                                                 {'name': 'Second'}],                                                       'users': {'alice': {'cleartext_password': 'password',                                                                           'disabled': True,                                                                           'email': 'alicechains@example.com',                                                                           'full_name': 'Alice '                                                                                        'Chains',                                                                           'username': 'alice'},                                                                 'johndoe': {'cleartext_password': 'password',                                                                             'disabled': False,                                                                             'email': 'johndoe@example.com',                                                                             'full_name': 'John '                                                                                          'Doe',                                                                             'username': 'johndoe'}}}}

@tiangolo It seems makingscopes part of cache key may have a side effect because of the inheritance and merging ofscopes in the dependency tree.
But this very inheritance also makes sense when we do this out of security concerns. What's your idea?

@tiangolo I have a solution but not sure if this contradicts the original security design.

• scopes is still passed down in the dependency tree
• butscopes is taken into cache key if the child node is stillSecurity while notDepends in the dependency tree

@tiangolo would you be able to have a look on this please?

This issue of scopes affectingDepends cache_key bit me as well in#9513. Curious what the solution to this could be as it doesn't seem intended. 🤔

I am also affected by this issue and opened a discussion (#6024).

@tiangolo I have a solution but not sure if this contradicts the original security design.

* `scopes` is still passed down in the dependency tree* but `scopes` is taken into cache key if the child node is still `Security` while not `Depends` in the dependency tree

@laggardkernel This is the way I wanted to implement a fix when poking with the cache code. I could also make a PR if we get some indication from@tiangolo that this approach is acceptable. Also we could be on the conservative side and introduce a new parameter toSecurity to allow dropping the scopes of non-securityDepends in cache keys. I am however not sure of how this parameter should be passed down the dependency chain.

I created#9790 to fix this issue.@laggardkernel Let me know what you think of it.

Is there a timeline for this to be PR to be merged? This bug is causing a few issues with running FastAPI in production because it halves our available db connections - every request requires two db sessions.

@DurandA we have been doing some more testing on this and have found that if you set up the DB using middleware you don't have this issue. This is because although you still have two entries in the cache forget_db they both point to the same db connection - e.g:

@app.middleware("http")async def db_session_middleware(request: Request, call_next):    response = Response("Internal server error", status_code=500)    try:        request.state.db = SessionLocal()        response = await call_next(request)    finally:        request.state.db.close()    return response

def get_db(request: Request):    return request.state.db

This approach also limits the amount of refactoring needed because you are still usingget_db as a dependancy.

@DurandA we have been doing some more testing on this and have found that if you set up the DB using middleware you don't have this issue.

Thank you for sharing your solution. IMHO, this is clean enough to be put in production while waiting for an update on this. The only downside I see is that a session is established regardless of whether a DB session is required by the request.

The only downside I see is that a session is established regardless of whether a DB session is required by the request.

Hi@brian-montgomery. You provided an interesting solution to this problem in a comment here but it somehow disappeared. Was there any particular issue with your proposal?

We continue to discuss the mentioned problem (about dependency caching) caused by this PR.
Could you take a look and give your opinion?
#6024 (comment)

@laggardkernel ,@qmorek ,@nyte-owl ,@Barnyclark