connexion icon indicating copy to clipboard operation
connexion copied to clipboard

Fail on invalid token when checking multiple OR security schemes

Open RobbeSneyders opened this issue 1 year ago • 3 comments

Fixes #1767

RobbeSneyders avatar Nov 02 '23 21:11 RobbeSneyders

Coverage Status

coverage: 94.236% (-0.001%) from 94.237% when pulling 4711dc241c9551f5d6b1ed83a8ca75d492203b4c on bugfix/or-security into d972eae8642be6a3c1dfb92e1cb091726d1f5225 on main.

coveralls avatar Nov 02 '23 22:11 coveralls

I'm not sure whether this prohibits some valid options. If you have 2 different OAuth2 providers for example (which I believe is valid), both using the Authorization: Bearer header/format, this will no longer be possible.

Hmm interesting, I didn't think of that.

So I'm thinking we need to make it optional to "stop early" in the security checking, e.g. by making a special exception that basically wraps the real exception you want to throw. Then we can catch that specific one and fail early in that case, otherwise we continue and evaluate at the end.

I think we can maybe still generalize it in the following way: "If a known security token is included in the request, it should be valid against at least one of the schemes".

Maybe an implementation like this could work (pseudo):

valid_tokens = {}
invalid_tokens = defaultdict(list)

token_info = NO_VALUE
for func in auth_funcs:
    token_name = auth_func.token_name
    try:
        token_info = func(request)
        while asyncio.iscoroutine(token_info):
            token_info = await token_info
        if token_info is not NO_VALUE:
            valid_tokens[token_name] = token_info
    except Exception as err:
        invalid_tokens[token_name].append(error)

if not valid_tokens:
        logger.info("... No auth provided. Aborting with 401.")
        raise OAuthProblem(detail="No authorization token provided")
    
for token, errors in invalid_tokens.items():
    if token in valid_tokens:
        continue
    else:
        cls._raise_most_specific(errors)

# TODO: Combine valid token_infos like with multiple AND schemes

RobbeSneyders avatar Nov 03 '23 10:11 RobbeSneyders

That sounds like a valid approach. I don't know how multiple security schemes can be handled properly here though, as the auth_funcs can be a verify_multiple_schemes function, so can be mapped to multiple token names

Ruwann avatar Nov 07 '23 11:11 Ruwann