feat: separate authcode validation and response generation

[imgurbot12]

addresses: #101

This is only a quick edit for a potential solution.
I'd be happy to discuss strategy here or in the related issue.

[shawn-monadical]

Consider this: what if all the information needed to create the authorization response was copied from the request to the AuthorizationState in the validate_authorization_request function (i.e. all the necessary query params like client_id, redirect_uri, state, nonce, etc)? And then in create_authorization_response function, instead of getting that data from the request, it would get it from the AuthorizationState instead.

This would be useful for situations where the authorization process spans multiple requests (like if you are presenting the user with a login page for example). This way the request which completes the authorization process wouldn't necessarily have to be the same request that initiated the authorization process.

However I realize this would be a big change since all the response type classes would have to be updated to support that. What does everyone else think?

[imgurbot12]

I'm open to the idea. The current implementation was an attempt to make the needed changes with the smallest footprint possible, so I suppose it'd be up to @aliev on what he thinks is best.

[aliev]

Thank you, folks, for your ideas and contributions! Sorry for the delay in responding.

Like @shawn-monadical mentioned, these changes are significant and should probably be added in a major version of the package.

How about considering the following workaround?

@router.get("/authorize")
async def authorize(
    request: Request,
    query: Query = Depends(),
    storage: SQLAlchemyStorage = Depends(get_sqlalchemy_storage),
):
    oauth2_storage = Storage(storage=storage)
    authorization_server = AuthorizationServer(storage=oauth2_storage)
    oauth2_request: OAuth2Request = await to_oauth2_request(request, settings)
    oauth2_response = await authorization_server.create_authorization_response(
        oauth2_request
    )

    if oauth2_response.status_code == HTTPStatus.UNAUTHORIZED:
        # TODO redirect user here.
        ...
    return await to_fastapi_response(oauth2_response)

here: https://github.com/aliev/aioauth-fastapi/blob/master/aioauth_fastapi_demo/oauth2/endpoints.py#L59-L61

But in that case, we need to maintain the state to redirect the user back after successful authentication (it seems like you covered this case in this PR). Please give me a little more time to do the research and code review.

[imgurbot12]

Alright, so funny enough after making the necessary changes I'm actually gonna argue that this PR and the design required to split create_authorization_response into a validate/response-generation is BAD, and this actually makes things more complicated than they are helpful.

The primary reason is it requires the user to be responsible for a lot more manual state checking rather than just letting aioauth handle it itself. Because aioauth wraps its errors and simply returns them as error responses, its very easy to simply return an error, and check the status of it for specific cases (like not being logged in) vs splitting them in two. Take a look at the comparison in the fastapi example before and after.

before changes:

@app.get("/oauth/authorize")
async def authorize(
    request: Request, oauth: AuthServer = Depends(get_auth_server)
) -> Response:
    """
    oauth2 authorization endpoint using aioauth
    """
    oauthreq = await to_request(request)
    response = await oauth.create_authorization_response(oauthreq)
    if response.status_code == HTTPStatus.UNAUTHORIZED:
        request.session["oauth"] = oauthreq
        return RedirectResponse("/login")
    return to_response(response)

after changes:

@app.get("/oauth/authorize")
async def authorize(
    request: Request, oauth: AuthServer = Depends(get_auth_server)
) -> Response:
    """
    oauth2 authorization endpoint using aioauth
    """
    oauthreq = await to_request(request)
    auth_state = await oauth.validate_authorization_request(oauthreq)
    if isinstance(auth_state, OAuthResponse):
        return to_response(auth_state)
    if "user" not in request.session:
        request.session["oauth"] = auth_state
        return RedirectResponse("/login")
    response = await oauth.create_authorization_response(auth_state)
    return to_response(response)

what you would think you gain in being more intentional about the operation actually just kinda makes a mess of things.

It's also not intentionally clear the developer should make sure that aioauth.Request.user is set either until they get an error with create_authoriation_response since there's no part of the design that indicates the user is not already set or needs to be set which creates this hidden design flow.

Perhaps that's due to a poor design decision on my part but so far I'm actually not seeing the benefit to splitting the two functions knowing that its possible to operate just fine without doing so. Maybe it would be worth it to highlighting in the documentation how to handle the standard case of not being logged-in immediately somewhere or something.

What are everyone's thoughts? 🤔

[thearchitector]

at least as its presented here, i agree that the former is a lot clearer than the latter. imo as soon as you start throwing in isinstance stuff because of return type unions things get hairy.

i think the tricky thing is that, at a surface level, inspecting the "created auth response" breaks the (at least my) mental model of the step 1, step 2, step 3 handling linearity that the specs illustrate; i would expect any of my customization (as a library user) to happen in those various steps rather than hidden away.

but honestly it may just come down to better references/examples and documentation around exactly what is happening and where its happening regarding flows, esp if this lib intends to obfuscate/present a lot of the specifics of the oauth/oidc specs in a friendly way

[thearchitector]

thinking about this more, actually, im not entirely convinced by what i said

since i'm trying to adhere strictly to the OIDC spec, im finding myself basically having to do what you've suggested here anyway:

1. always start with request.user = UnauthenticatedUser(), since we should not check the user before validating the req
2. call create_authorization_response in order to run request validation, knowing it will yield a 401
3. trying to log in the user via whatever means
   • if successful, re-calling create_authorization_response in order to actually generate the auth code
   • if not, redirecting to login page

it would be slightly better if I used AuthorizationMiddleware and set user accordingly first, then redirected to login later if 401 ... but still feels out of compliance

[imgurbot12]

call create_authorization_response in order to run request validation, knowing it will yield a 401

I think this is where i have the hangup with decision to split it, because the request validation can obviously error if the request is not valid which means it either has to:

1. raise an exception (which u have to process and likely convert to a standard http response)
2. return a pre-packaged error response for you

In either cause u have to handle the error. So you're already comparing the output of the result of the validation function anyways in all circumstances.

Oauth error responses by standard have to have certain headers and follow a basic error response format for redirects and such, so in aioauths case they make it super easy by pre-packaging the exception as a valid response that u can just return mindlessly in most cases.

Similarly, when you're redirecting to a user login u have to save the oauth-data passed to the authorize endpoint since the oauth response needs to be passed back afterwards. The way i see it, you have two options, its either:

1. the cached request (which is how you would need to do it now)
2. some intermediate state of processed request (which is how this PR handles it)

In most cases I think its simpler to just hold onto the original oauth-request information to call create_authorization_response again later without having to worry about juggling state or deal with any weird unions in the output like I demo in the fastapi-example for this PR.

Leaving it to one call also automatically covers the case where the user has already logged in, like if you're choosing to keep an active session for faster approvals across multiple apps or just providing a "stay logged in option".

[thearchitector]

Leaving it to one call also automatically covers the case where the user has already logged in, like if you're choosing to keep an active session for faster approvals across multiple apps or just providing a "stay logged in option".

right, yeah. the crux of it is that pretty much all the logic is behind that 1 function call. to be really strict about the flow of stuff probably means sacrificing a lot of friendliness there, having to split stuff up, and would definitely require a pretty massive redesign that would move away from the "hide the spec specifics" approach

[aliev]

Sorry for the delay in responding. It took some time to fully "refresh" my memory of the project's codebase. I’ve created a PR that might serve as a trade-off. Please let me know what you think.