fief-dev / fief-python

Fief client for Python
https://docs.fief.dev/integrate/python/
MIT License
11 stars 4 forks source link

Tenant prefix is added twice when using relative URL #16

Closed frankie567 closed 5 months ago

frankie567 commented 5 months ago

Discussed in https://github.com/orgs/fief-dev/discussions/320

Originally posted by **timorobin** January 8, 2024 I am self-hosting fief as a GCP cloud run service and integrating into my fastapi backend. When I configure the client to use a new tenant I've created manually in the fief app, I enter my client id, client secret, get redirected to the fief ui, log in as the new user I just made, and then get redirected back to the fastapi-generated docs. All goes well, but the issue is when I call an authenticated endpoint, I get a JWK error. After some debugging I figured out that when the `_get_endpoint_url` method was called with `absolute=False` the relative path returned would include the tenant name first, while the httpx client we build has a base url with the tenant name as well. This resulted in the request being `https://
///.well-known/jwks.json` and similar for the get user info request. If I specify the base url without the tenant, openid config we get is scoped for the main fief admin 'tenant' not the tenant I just created. I fixed this subclassing the `FiefAsync` and adding an optional `tenant_name` attribute. So when I init the class, I pass in the base url, without any tenant name involved and I manually prefixed the authorize and token url attributes with the tenant name. I also overwrote the `_get_openid_configuration_request` to account for the tenant name attribute. This seems to have fixed my issues, but is there anything I'm not realizing?
ocontant commented 5 months ago

Environment: Development Test Environment (no sensitive data involved)

@frankie567 I faced the same bug when configuring a test CLI from the documentation.

Issue Description:

Encountered a bug while setting up a test CLI based on the provided documentation, leading to an authentication error due to an incorrect handling of tenant information in the redirect_uri, or a dependency using the fief_client base_url attribute.

Steps to Reproduce:

Received a "client not found" error during initial setup.
Identified that the fief_client's authorize() method was not including the tenant in the redirect_uri, resulting in requests being routed to the default admin tenant.
Modified the base_url attribute in the fief_client object constructor to include the tenant in the URI.
Successfully logged in via the web interface, but encountered a None return for the access_code in the CLI, after the web authentication flow completed.

Debugging Information:

Debug Trace

From vscode Debug

Exception has occurred: FiefRequestError       (note: full exception trace is shown but execution is paused at: login)
[404] - {"detail":"Not Found"}
  File "Sandbox/test_jwt/Fief-jwt-auth/cli.py", line 44, in login (Current frame)
    fief_auth.authorize()
  File "Sandbox/test_jwt/Fief-jwt-auth/cli.py", line 68, in <module>
    cli()
fief_client.client.FiefRequestError: [404] - {"detail":"Not Found"}

From container Debug log:

2024-02-03 22:00:43 2024-02-03 15:00:43.185 | INFO     | uvicorn.protocols.http.httptools_impl:send:496 - 192.168.65.1:54493 - "GET /test-auth/authorize?response_type=code&client_id=s7RPgOnwDdc6FTuPM31wgStTEPYA4dkQpr4gsKnVE4I&redirect_uri=http%3A%2F%2Flocalhost%3A51562%2Fcallback&scope=openid+offline_access&code_challenge=bh7-S7KVPW2ynd_spAYaHeqwzxK8qQzEW-2J9DLk9Qw&code_challenge_method=S256 HTTP/1.1" 302 - {}
2024-02-03 22:00:43 2024-02-03 15:00:43.207 | INFO     | uvicorn.protocols.http.httptools_impl:send:496 - 192.168.65.1:54493 - "GET /test-auth/consent HTTP/1.1" 302 - {}
2024-02-03 22:00:43 2024-02-03 15:00:43.326 | INFO     | uvicorn.protocols.http.httptools_impl:send:496 - 192.168.65.1:54494 - "POST /test-auth/test-auth/api/token HTTP/1.1" 404 - {}
2024-02-03 22:48:49 2024-02-03 15:48:49.413 | INFO     | uvicorn.protocols.http.httptools_impl:send:496 - 192.168.65.1:54639 - "GET /test-auth/.well-known/openid-configuration HTTP/1.1" 200 - {}

The fief-server method get_authorize_client() include get_current_tenant() dependency in the method . get_authorize_client() is a dependency of get_authorize_redirect_uri(). get_authorize_redirect_uri is called by auth_callback(), which is called by authorized().

image
async def get_authorize_client(
    client_id: str | None = Query(None),
    tenant: Tenant = Depends(get_current_tenant),  # <--------- Possibly the line responsible for doubling the tenant URI value.
    repository: ClientRepository = Depends(get_workspace_repository(ClientRepository)),
) -> Client:
    if client_id is None:
        raise AuthorizeException(
            AuthorizeError.get_invalid_client(_("client_id is missing"))
        )

    client = await repository.get_by_client_id_and_tenant(client_id, tenant.id)

    if client is None:
        raise AuthorizeException(AuthorizeError.get_invalid_client(_("Unknown client")))

    return client

Suspected Root Cause:

The issue appears to stem from the fief_server's handling of the base_url, specifically the duplication of the tenant name in the API endpoint path. EDITED TO REMOVE WRONG ROOT CAUSE

frankie567 commented 5 months ago

@ocontant No, the bug is actually caused by this part:

https://github.com/fief-dev/fief-python/blob/a68cdeb168066b6c787ef7c8fbaa9c57cae36882/fief_client/client.py#L255-L258

To handle Docker better, we manipulate the URL returned by our openid_configuration endpoint: we remove the host to keep only the path. So when Fief returns us this:

https://example.fief.dev/.well-known/jwks.json

We change it to:

/.well-known/jwks.json

So far so good, but it breaks with a sub-tenant with a prefix:

https://example.fief.dev/my-tenant/.well-known/jwks.json
/my-tenant/.well-known/jwks.json

Since the client base URL is https://example.fief.dev/my-tenant/, we end up calling:

https://example.fief.dev/my-tenant/my-tenant/.well-known/jwks.json
ocontant commented 5 months ago

This is the flow as I understand it, but i might be missing steps and details. From step 4, I'm more doubtful 1: Phase Initialization: Client need to know about the tenant, because at this point of the flow, the server cannot provide it for us. 2: Phase Authorize: Client initiate connection to a remote defined in the client as base_url to connect to the Authorize() endpoint. _IF tenant is not included, the clientid cannot be found, because we hit the default admin tenant 3: Phase Token Validation: The server accepted the Authorize credentials and return us an access_code in a JWT signed by a JWKS. 4: We need to decode the JWT using the JWKS to extract the access code, hence using the .well_known URI to access the JWKS. Does the well_known URI returned with the JWT ? 5: We manipulated the well_known URI for the docker context.

From that flow, I understand 2 contexts.

  1. Client initiate request, that require an initial knowledge of how to communicate with the remote.
  2. Client request following an initial request, for which the server can provide knowledge of how to communicate with the server.

Question: 1- Do we need to duplicate API endpoint per tenant? (adding f"{base_url}/{tenant}/{endpoint}") 2- Would it be better to keep all the logic on the default endpoint URI (f"{base_url}/{endpoint}"), and add logic to the endpoint to validate if {tenant} is None: _tenant="default" else _tenant=tenant, and apply the logic based on the tenant context for each endpoint.

In the current context, you need to parse the {base_url} before reconstructing the new url, to keep only the host part.

from urllib.parse import urlparse
parsed_url = urlparse(base_url)
oid_config_uri=f"{parsed_url.scheme}://{parsed_url.hostname}/{parsed_oid_config}

Alternatively: We could add a tenant attribute to the class and assign the tenant value to it when we instantiate the class. The tenant value would be concatenate to the base_url when necessary for the context where the client initiate the flow.

frankie567 commented 5 months ago

Well, the main challenge is the following: in the OpenID specification, auth servers should expose a ./well-known/openid-configuration endpoint. This endpoint should give all the URL and parameters required to perform OAuth2 correctly. Here is one example:

https://example.fief.dev/.well-known/openid-configuration

Thanks to this, you can use any OpenID compatible client, feed it with this URL, and everything should work nicely.


Now, we introduce tenants. Those are logically-separated set of users, with their own set of parameters. So, they need their ./well-known/openid-configuration endpoint as well. Problem is, if we want to be compatible with OpenID clients, we can't rely on headers or other technique to set the tenant: it has to appear in the URL. That's why tenants have a path prefix:

https://example.fief.dev/sub/.well-known/openid-configuration


For the current bug, we just need to be a bit more smarter and remove the base URL, including the prefix if there is one. I'll issue a fix.