[jmaupetit]

Purpose

Drive migrations acting as the user rely on the short-lived ProConnect token, so those waiting in a busy queue can fail once it expires.

Proposal

The new "user_exchanged_token"'s DRIVE_AUTH_MODE exchanges it with Menshen (RFC 8693) at launch for a Drive token whose lifetime is set by Menshen, stored apart from the OIDC tokens. Celery tasks use that token and exchange again if it expires while the ProConnect session can still be refreshed.

[rage2000]

Thanks for this! One design question about the Menshen call.

exchange() goes through menshen_client, which sends the request with its own httpx2.Client, while every other call in the backend (Drive, the ProConnect refresh, Resana) goes through RetryingSession and handles requests exceptions. Hence the translation of httpx errors into requests ones in exchange(), which leads to the issue on L800. Other consequences:

• limited_retries(max_attempts=1) in ensure_drive_token does not apply to the Menshen call, and neither do the DRIVE_RETRY_* settings nor the retry/give-up logs. During a migration this is partly compensated, since the translated ConnectionError is retried by the enclosing Drive call.
• The translation layer has to be maintained: each new httpx exception type must be mapped by hand.
• Two HTTP stacks (connection pools, timeouts, test mocking).

Was there a reason to go through the client's own transport (e.g. a recommendation from the Menshen team, or plans to use introspect / revoke later)? If not, I see a few options:

1. Drop menshen_client, send the RFC 8693 request through self.session

Same pattern as DriveUserTokenBackend._refresh: form POST with Basic auth, raise_for_status(), except HTTPError / except RequestException, then read access_token and expires_in from the JSON.

• Pros: one HTTP stack, real requests errors carrying their response, no translation layer. Retries, limited_retries and logs as everywhere else. No httpx2 / menshen_client dependency, no cached client.
• Cons: we own the Menshen specifics (endpoint path, form fields, response format): if Menshen changes them, we change our code instead of bumping the client.

2. Keep the current design, fix the translation

Attach a response to the translated HTTPError, declare httpx2.

• Pros: smallest change, the official client is used as is.
• Cons: HTTPError expects a requests.Response; passing the httpx2 one works by duck typing (status_code, text, json()) but breaks as soon as a caller relies on something specific to requests. The translation layer and the two stacks stay, and the Menshen call itself is still not retried or capped.

3. Use menshen_client for the format, RetryingSession for the transport

Only use the client's HTTP-free public API (Configuration.token_url, TokenExchangeRequest, TokenType, TokenExchangeResponse) and send the request ourselves:

config = _menshen_config()
request = TokenExchangeRequest(
    subject_token=subject_token,
    subject_token_type=TokenType.ACCESS_TOKEN,
    audience=settings.DRIVE_TOKEN_EXCHANGE_AUDIENCE or None,
    scope=settings.DRIVE_TOKEN_EXCHANGE_SCOPE or None,
)
response = self.session.post(
    config.token_url,
    data={key: value for key, value in asdict(request).items() if value is not None},
    auth=(config.client_id, config.client_secret),
    timeout=config.requests_timeout,
    retry=True,
)
response.raise_for_status()
exchanged = TokenExchangeResponse(**response.json())

• Pros: Menshen's URL, fields and response format still come from the official client, so a protocol change is followed by bumping it. The transport is ours: real requests errors, retries, limited_retries, logs, no direct httpx2 import.
• Cons: we reproduce the client's private _form_data (the None filter) and its Basic auth, so a future transport-level change in the client (extra header, signing) would not be picked up. httpx2 stays installed, unused by our code.

4. Upstream: let menshen_client accept an injected session/transport, or expose request building and response parsing apart from sending

Since menshen lives in suitenumerique too, this would make option 3 official instead of relying on a private helper.

• Pros: cleanest long-term, benefits other services in the suite.
• Cons: needs a client release first. It can be combined with 2 or 3 in the meantime.

I would lean towards 3 for this PR, possibly with an upstream issue for 4. What do you think?

[rage2000]

httpx2 is imported directly in drive_backend.py and in the tests, but only comes in transitively through menshen_client. If we keep importing it, it should be declared here too (depending on the outcome of the general comment, it may not be needed at all).

[rage2000]

This HTTPError has no response attached (response=None), and some callers read it.

During a migration the exchange runs inside _BearerTokenAuth, so when the exchanged token expires and Menshen answers a 4xx that is not invalid_grant (e.g. a refused client), this error is raised from within do_create_item. _is_server_error and _is_duplicate_id_conflict handle a missing response, but the logger.warning right after reads http_error.response.status_code and .text (L394-396):

AttributeError: 'NoneType' object has no attribute 'status_code'

which hides the actual Menshen error.

A test for this case could set an expired exchanged token on the user, make the Menshen client raise an HTTPStatusError with a 403, and call create_folder(): it should raise an HTTPError, not an AttributeError.

The fix depends on how we want to call Menshen, see my general comment.

[rage2000]

A ResponseParsingError raised by this call (Menshen answering an unexpected body, or a misconfigured server URL returning something else) is not a RequestException, so it lands in the except Exception below and is reported as DriveTokenRequired, so the user is asked to log in again, which cannot help. Could it be mapped to DestinationUnavailable, or to a dedicated error?

[martigan]

It could be in a utils file or a shared folder as tribu will use it no?

[martigan]

make the get_token method not private

[martigan]

why not raising from error here?

[martigan]

I think the name is not generic enough

it could be used with different destination (I have tribu in mind)
maybe destination_exchanged_token

[martigan]

dito