Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fail on invalid token when checking multiple OR security schemes #1778

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

RobbeSneyders
Copy link
Member

Fixes #1767

@coveralls
Copy link

Coverage Status

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

Copy link
Member

@Ruwann Ruwann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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.

@RobbeSneyders
Copy link
Member Author

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

@Ruwann
Copy link
Member

Ruwann commented Nov 7, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow fast fail when mutliple security handlers are provided
3 participants