🐛 Fix cached dependencies when using a dependency in Security() and other places (e.g. Depends()) with different OAuth2 scopes
#2945
Conversation
Codecov Report
@@ 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 us how you rate us. Have a feature suggestion? Share it here. |
|
📝 Docs preview for commit d8b63cdaebaf9c37b195bfa0bca54f3b9b0c8fa2 at: https://604c78b2648d6ca4039cc914--fastapi.netlify.app |
laggardkernel
changed the title
|
I've also considered implementing
|
laggardkernel
force-pushed
the
bugfix/cache-key
branch
from
d8b63cd to
2c18368
Compare
|
Updated once after tweaking the related test. Also rebased the branch to current master in this force push. |
tiangolo
changed the title
Security() and other places (e.g. Depends()) with different OAuth2 scopes
|
📝 Docs preview for commit 3399de4 at: https://6304d5c087148506d673b746--fastapi.netlify.app |
|
Thanks for the detailed explanation and for your contribution @laggardkernel! 🙇 🚀 🍰 |
|
Hi guys, from fastapi import FastAPI, Depends, Security
from fastapi.security import SecurityScopes
def get_obj(
required_scopes: SecurityScopes,
):
print(f'get_obj called with scopes: "{required_scopes.scope_str}"')
return 'object'
def check_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'])],
)
def get_obj_with_scopes(
obj=Depends(get_obj),
):
return obj
@app.put(
'/',
dependencies=[Security(check_user_role_for_obj, scopes=['edit_scope'])],
)
def put_obj_with_scopes(
obj=Depends(get_obj),
):
return obj
@app.get(
'/child_obj',
dependencies=[Security(check_user_role_for_obj, scopes=['read_child_scope'])],
)
def get_obj_with_scopes():
return 'child object'Here is output from calling get_obj_with_scopes endpoint: as you can see get_obj was called twice.... I am just throwing it for consideration. |
|
Using a database session inside a |
|
Hi @tiangolo, |
|
To understand dependency cache, we may start from what features are added by FastAPI upon Starlette.
In the view/router function, we parse the
The introduction of dependency ( async def sum_c(a: int = 0, b: int = 0):
return a + b
@app.get("/sum")
async def calc_sum(c: int = Depends(sum_c)):
return cIn the above example, the view func doesn't accept query param To avoid duplicated, high cost calculation in some
nparley qmorek |
|
Hi @laggardkernel, could you eplain what you mean by:
|
|
I took example from: https://fastapi.tiangolo.com/advanced/security/oauth2-scopes/#dependency-tree-and-scopes from typing import Dict
from datetime import datetime, timedelta
from typing import List, Union
from fastapi import Depends, FastAPI, HTTPException, Security, status
from fastapi.security import (
OAuth2PasswordBearer,
OAuth2PasswordRequestForm,
SecurityScopes,
)
from jose import JWTError, jwt
from pydantic import BaseModel, ValidationError
# to get a string like this run:
# openssl rand -hex 32
SECRET_KEY = "09d25e094faa6ca2556c818166b7a9563b93f7099f6f0f4caa6cf63b88e8d3e7"
ALGORITHM = "HS256"
ACCESS_TOKEN_EXPIRE_MINUTES = 30
fake_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"
}
]
}
class Token(BaseModel):
access_token: str
token_type: str
class TokenData(BaseModel):
username: Union[str, None] = None
scopes: List[str] = []
class User(BaseModel):
username: str
email: Union[str, None] = None
full_name: Union[str, None] = None
disabled: Union[bool, None] = None
class UserInDB(User):
cleartext_password: str
oauth2_scheme = OAuth2PasswordBearer(
tokenUrl="token",
scopes={"me": "Read information about the current user.", "items": "Read items."},
)
app = FastAPI()
def verify_password(plain_password, db_password):
return plain_password == db_password
def get_user(db, username: str):
users_db = db['users']
if username in users_db:
user_dict = users_db[username]
return UserInDB(**user_dict)
def authenticate_user(fake_db, username: str, password: str):
user = get_user(fake_db, username)
if not user:
return False
if not verify_password(password, user.cleartext_password):
return False
return user
def create_access_token(data: dict, expires_delta: Union[timedelta, None] = None):
to_encode = data.copy()
if expires_delta:
expire = datetime.utcnow() + expires_delta
else:
expire = datetime.utcnow() + timedelta(minutes=15)
to_encode.update({"exp": expire})
encoded_jwt = jwt.encode(to_encode, SECRET_KEY, algorithm=ALGORITHM)
return encoded_jwt
def get_db() -> Dict:
print('get_db called')
return fake_dict_db
async def get_current_user(
security_scopes: SecurityScopes,
db: Dict = Depends(get_db),
token: str = Depends(oauth2_scheme),
):
print('required scopes are:', security_scopes.scope_str)
if security_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")
if username is None:
raise credentials_exception
token_scopes = payload.get("scopes", [])
token_data = TokenData(scopes=token_scopes, username=username)
except (JWTError, ValidationError):
raise credentials_exception
user = get_user(db, username=token_data.username)
if user is None:
raise credentials_exception
for scope in security_scopes.scopes:
if scope not in token_data.scopes:
raise HTTPException(
status_code=status.HTTP_401_UNAUTHORIZED,
detail="Not enough permissions",
headers={"WWW-Authenticate": authenticate_value},
)
return user
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")
return current_user
@app.post("/token", response_model=Token)
async def login_for_access_token(
db: Dict = Depends(get_db),
form_data: OAuth2PasswordRequestForm = Depends(),
):
user = authenticate_user(db, form_data.username, form_data.password)
if not user:
raise HTTPException(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)
async def read_users_me(current_user: User = Depends(get_current_active_user)):
return current_user
@app.get("/users/me/items/")
async def read_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/")
async def read_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: 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 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, the @app.get("/users/me/items/")
async def read_own_items(
db: Dict = Depends(get_db),
current_user: User = Security(get_current_active_user, scopes=["items"])
):
async def get_current_active_user(
current_user: User = Security(get_current_user, scopes=["me"])
):
async def get_current_user(
security_scopes: SecurityScopes,
db: Dict = Depends(get_db),
token: str = Depends(oauth2_scheme),
):
confirmed by printing dependency_cacheTwo different cache keys
@tiangolo It seems making |
|
@tiangolo I have a solution but not sure if this contradicts the original security design.
|
|
@tiangolo would you be able to have a look on this please? |
|
This issue of scopes affecting |
|
I am also affected by this issue and opened a discussion (#6024).
@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 to |
|
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 for This approach also limits the amount of refactoring needed because you are still using |
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. |
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. |
To make
.security_scopescontribute to.cache_key, parametersecurity_scopesmust be passed toDependant.__init__(). We forgot to do it in this way. Reassignment after initdependant_instance.security_scopes = ["dummy"]doesn't change the.cache_keyanymore. Main related logic is atget_sub_dependant().This makes
Securitywith different scopes (OAuth2 scopes) share the same cache. If we haveSecurity(func)andSecurity(func, scopes=['scope1'])in a dependency tree. Only one of them will be really called.The bug could be traced back to bff5dbb on 6/6/19 when the
dependency_cacheis first introduced. Seems a serious bug for anyone usingSecurityin dependencies, better fix it soon.