Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@microsoft-github-policy-service agree

based on the flow of BaseBearerTokenAuthenticationProvider.authenticate_request it's responsible for checking if the current request has authentication, if not then it kicks of the process. By that same logic it would also be responsible for making sure the authentication it already has isn't expired before pushing it down the line. Unless I'm misunderstanding something?

I believe your understanding is right, what I'm trying to get more information on is how does your application get into a situation with a request information pre-authenticated with an expired token?

Thank you for your patience in this discussion. Our application is a graphical one that users often leave open for extended periods of time, sometimes up to 8 hours. The authentication token that we use has an expiration time of about 1 hour.

During the application’s lifecycle, users make multiple requests at variable intervals. They authenticate when the application is launched and expect to interact with it until they close it, usually at the end of their shift, at which point the cache is cleared of the refresh token and auth token. Given this, there are instances where the application has a request pre-authenticated with a token that could expire before the request is completed.

To address this, we’re considering implementing a token refresh strategy. This would involve checking the token’s expiry before each request and refreshing it if it’s near its expiration. If the refresh request fails, we would then prompt the user to re-authenticate. This approach would ensure that our application remains user-friendly and secure, as it would always use valid tokens and wouldn’t require users to relaunch the application.

I hope this clarifies the situation. Please let me know if you have any further questions or need additional information.

Thanks for the additional context. I'm not sure why/how the application pre-authenticates the requests without sending them? Wouldn't it be easier to let the client authenticate the requests with a recent token as the request is being sent out?

this application is making use of the Graph SDK. The first request of the application is sent through without authentication, at which point it hits this method, where it's seen to be missing the authentication header it follows the process for the user to authenticate. From then on any requests made using that client by default utilizes that authentication token until it expires.
Hopefully this helps to clarify!

The specific method in question is:
HttpxRequestAdapter.send_async

async def send_async(
        self,
        request_info: RequestInformation,
        parsable_factory: ParsableFactory,
        error_map: Dict[str, ParsableFactory],
    ) -> Optional[ModelType]:
        """Excutes the HTTP request specified by the given RequestInformation and returns the
        deserialized response model.
        Args:
            request_info (RequestInformation): the request info to execute.
            parsable_factory (ParsableFactory): the class of the response model
            to deserialize the response into.
            error_map (Dict[str, ParsableFactory]): the error dict to use in
            case of a failed request.

        Returns:
            ModelType: the deserialized response model.
        """
        parent_span = self.start_tracing_span(request_info, "send_async")
        try:
            if not request_info:
                parent_span.record_exception(REQUEST_IS_NULL)
                raise REQUEST_IS_NULL

            response = await self.get_http_response_message(request_info, parent_span)

            response_handler = self.get_response_handler(request_info)
            if response_handler:
                parent_span.add_event(RESPONSE_HANDLER_EVENT_INVOKED_KEY)
                return await response_handler.handle_response_async(response, error_map)

            await self.throw_failed_responses(response, error_map, parent_span, parent_span)
            if self._should_return_none(response):
                return None
            root_node = await self.get_root_parse_node(response, parent_span, parent_span)
            if root_node is None:
                return None
            _deserialized_span = self._start_local_tracing_span("get_object_value", parent_span)
            value = root_node.get_object_value(parsable_factory)
            parent_span.set_attribute(DESERIALIZED_MODEL_NAME_KEY, value.__class__.__name__)
            _deserialized_span.end()
            return value
        finally:
            parent_span.end()

yes, and the way this is built is to:

1. always request the token from the authentication library
2. request and set the token only right before the request is sent to the service

If we end up at this stage with a request that contains an expired or revoked token, it can only mean one of those cases:

1. the client OS time settings are wrong (let's exclude that for the time being)
2. continuous access evaluation challenge like password reset, account being locked, device moved to a geo-fenced area... (should result in claims value being set, and the token being removed already)
3. request was pre-authenticate by the client application (header "manually set", applications should not be doing that)
4. the access token provider, through the authentication library its using (azure identity for the implementation we provide) returned an access token that was already expired. This would indicate an issue with token caching and should be fixed there, instead of being patched for here.

I hope that clarifies the design a little bit. I'm trying to narrow down which scenario your application falls in so we can address the issue at its root. Does it make sense? Do you have more context to share on that?

example code snippet, my guess is the issue would be at it sharing the request information.

class AsyncValidateCommand(AsyncAPICommand):
    """
    Class for a command to validate something using the ##### API.
    """

    _type: ValidateType
    """The type of validation to perform."""

    _request_information: RequestInformation
    """Information about the API request."""

    _upn: str
    """The user principal name (UPN) to validate."""

    def __init__(
        self,
        client: GraphServiceClient,
        _type: ValidateType,
        upn: str,
        lock: Lock | None = None,
    ) -> None:

        super().__init__(client, lock)

        if not upn:
            raise ValueError("'upn' must be not empty value")

        self._type = _type
        self._upn = upn
        self._request_information = RequestInformation()
        self._request_information.url_template = "{+baseurl}/api/..."
        self._request_information.http_method = Method.GET

    @abstractmethod
    async def _send(
        self,
        factory: ParsableFactory,
        error_mapping: Dict[str, ParsableFactory],
    ) -> Optional[Parsable]:
        """
        Sends the API request and returns the response.

        Args:
            factory (ParsableFactory): A factory for creating Parsable objects from the response.
            error_mapping (Dict[str, ParsableFactory]):
            A mapping from HTTP status codes to factories for creating
            Parsable objects from the error response.

        Returns:
            Optional[Parsable]: The response from the API, or None if there was an error.
        """

    async def execute(self) -> Optional[ValidationResponse]:
        """
        Executes the validation command.

        Returns:
            Optional[ValidationResponse]: The response from the validation API,
            or None if there was an error.
        """

        body = Validation(
            UPN=self._upn,
            ValidateType=self._type,
        )

        self._request_information.set_content_from_parsable(
            self._client.request_adapter,
            "application/json",
            body,
        )

        factory = ValidationResponse()

        error_mapping: Dict[str, ParsableFactory] = {
            "4XX": ValidationError,
            "5XX": ValidationError,
        }

        try:
            async with self.lock:
                return await self._send(factory, error_mapping)
        finally:
            self._is_executed = True

even with this I'd still argue that have it remove or raise an exception at the same point in this library would be useful. Though that's an enhancement instead of a bug.

in this snippet, is the send method being called multiple times for the same AsyncValidateCommand instance?

a subclass yes. this is inherited into the different commands (ex: AsyncValidateUserExits, AsyncValidateRiskDismissal, etc.) and those instances are stored and fetched at a later time.
ex:

class AsyncValidateRiskDismissalCommand(AsyncValidateCommand):
    """
    Class for a command to validate if risks were dismissed for a specific user to the
    #### API.
    """

    def __init__(self, client: GraphServiceClient, upn: str, lock: Lock | None = None) -> None:
        super().__init__(client, ValidateType.VALIDATE_RISK_DISMISSAL, upn, lock)

    async def _send(
        self,
        factory: ParsableFactory,
        error_mapping: Dict[str, ParsableFactory],
    ) -> Optional[ValidationResponse]:
        """
        Sends the API request and returns the response.

        Args:
            factory (ParsableFactory): A factory for creating Parsable objects from the response.
            error_mapping (Dict[str, ParsableFactory]): A mapping from HTTP status codes to
            factories for creating Parsable objects from the error response.

        Returns:
            Optional[ValidationResponse]: The response from the API, or None if there was an error.
        """

        return await self._client.request_adapter.send_async(
            self._request_information,
            factory,
            error_mapping,
        )

why wouldn't the code in that class simply remove the header in the send method?
Alternatively it could be the request information object in the send method. This would probably increase memory pressure but would alleviate any potential race condition or issue as mentioned above.

It could, and I'm adjusting it for such a case, removing the header if the token is expired. Though that would still leave me wandering if this PR could be tweaked so instead of check if the token is expired - since that can vary from definition of the access token - to instead if it sees any authentication header it's cleared?

One of the requirements we have is to use the access token if it's already present, since this can be a user override for a single request (think elevation of privileges kind of scenario), so we can't systematically remove what comes in there.
Another requirement we have is to clear the token when already present if a claims is also present (auth challenge because of CAE kind of situation).
After reviewing your code another time, I think the impact is lower than I originally estimated since it'll only trigger when:

1. an access token is already present
2. claims are NOT present

This is effectively only going to happen if the client application forcibly set a token on the request (or is re-using a request), which means the impact should be low (most of the times the code will be bypassed).
I'd like @samwelkanda opinion on this analysis before we perform further review as he is our resident Python expert.
Also, from an http semantics perspective, the bearer value can be something else than a JWT, so:

1. the parsing needs to be resilient and not blow up if it fails
2. we should probably leave the token in place if we can't decode it, since the user is probably doing something we didn't anticipate.

Then comes the question of the consistency across languages, the implementations are currently aligned, would you be willing to do similar work in the other languages?

I think all your points make sense! Thank you for the consideration! Just to clarify, are you asking if I am willing to (for lack of a better term) translate the adjusted, after making sure it's consistent with your (pl.) requirements, process?

not exactly, my ask was if you're willing to replicate the change in other kiota abstractions libraries for other languages once this one is merged?

I'd be willing to do the best I can, my breadth of languages is wide but I'm not certain I know all the languages supported by Kiota.

This pull request has conflicting changes, the author must resolve the conflicts before this pull request can be merged.