split authcode into verification/response (rebase on remove user feature)

[imgurbot12]

Re implementation of #102 with rebase from #118.

I think the removal of the user-check simplifies some of the issues I originally encountered with implementing the separation of authorization-code validation/request since now the developer can assume the response will always be an error that does not need to be specially handled if validate_authorization_request returns anything other than an AuthorizationState object.

The code is relatively clean and allows for more direct and clear branches to handle each possibility when processing the authorization request:

Old Version (before PR):

@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)

New Version (after PR):

@app.get("/oauth/authorize")
async def authorize(
    request: Request, oauth: AuthServer = Depends(get_auth_server)
) -> Response:
    """
    oauth2 authorization endpoint using aioauth
    """
    # validate initial request and return error response (if supplied)
    oauthreq = await to_request(request)
    response = await oauth.validate_authorization_request(oauthreq)
    if isinstance(response, OAuthResponse):
        return to_response(response)
    # redirect to login if user information is missing
    user = request.session.get("user", None)
    request.session["oauth"] = response
    if user is None:
        return RedirectResponse("/login")
    # otherwise redirect to approval
    return RedirectResponse("/approve")

[aliev]

Overall, the solution looks interesting, but I’m a bit concerned that validate_authorization_request can now return two different objects: Response (in case of an error) or AuthorizationState (if validation is successful). This makes the library's API less explicit. Let me experiment with this code a bit more.

[imgurbot12]

Overall, the solution looks interesting, but I’m a bit concerned that validate_authorization_request can now return two different objects: Response (in case of an error) or AuthorizationState (if validation is successful). This makes the library's API less explicit. Let me experiment with this code a bit more.

Please do :)

It's definitely not ideal but I wasn't sure how to avoid it with aioauth's preference of turning errors into response objects automatically. I didn't want to break the standard of how all the other calls work to handle it any other way. I don't dislike that aioauth does this either, in fact I think its probably a good thing that it pre-packages the response since as you know there are certain http header requirements in order to follow the rfc. That and it makes handling simpler in most cases.

[imgurbot12]

I should note for the record that, whilst the removal request.user does simplify this PR and provide a basis for a better design, I'm actually still on the fence if this change is positive considering the trade-offs.

Validating the request twice in a single call using the standard create_authorization_response method isn't that big of a deal in my mind and I realize this compromises on a lot of the core principles for the library which is likely why this implementation is as tricky as it is. I'm realizing now trying to update the unit-tests as well that this is breaking more things than I would've liked even after all the improvements.

That being said, I'm not totally against the changes I propose here either. I just don't want this new PR to feel like a push from me to get these changes made, its only really a proposal of possibility.

[aliev]

I should note for the record that, whilst the removal request.user does simplify this PR and provide a basis for a better design, I'm actually still on the fence if this change is positive considering the trade-offs.

Validating the request twice in a single call using the standard create_authorization_response method isn't that big of a deal in my mind and I realize this compromises on a lot of the core principles for the library which is likely why this implementation is as tricky as it is. I'm realizing now trying to update the unit-tests as well that this is breaking more things than I would've liked even after all the improvements.

That being said, I'm not totally against the changes I propose here either. I just don't want this new PR to feel like a push from me to get these changes made, its only really a proposal of possibility.

I understand, that's why I started v2, so it would be easier for us to discuss new changes. I'm very grateful for your ideas.

Actually, I really like the idea with a separate state and I would like to use it. Give me more time to think about this and find more options. We seem to be very close to a compromise solution.