feat: remove user by aliev · Pull Request #118 · aliev/aioauth

aliev · 2025-01-26T12:07:01Z

After analyzing the code, I identified that explicitly passing the user in aioauth causes more problems than it solves. Moreover, explicit user validation was only implemented in one place:

aioauth/aioauth/response_type.py

Lines 93 to 96 in 1c8ce11

    
           if not request.user: 
        
               raise InvalidClientError[UserType]( 
        
                   request=request, description="User is not authorized", state=state 
        
               )

I believe that the process of checking a user authorization does not fall within the domain of the aioauth. So, it was decided to completely remove the "knowledge" of the user from aioauth and delegate this responsibility to the client of this library.

However, there are cases where passing the user from a request of a client framework (e.g., to storage) is still necessary. For such scenarios, the extra field of the Request class can be used. Kudos to @imgurbot12 for this feature!

You can find a complete example in the examples/fastapi_example.py file in this branch:

aioauth/examples/fastapi_example.py

Lines 84 to 103 in 7352502

    
               user = request.session.get("user", None) 
        
               response = await oauth.create_authorization_response(oauthreq) 
        
               # A demonstration example of request validation before checking the user's credentials. 
        
               # See a discussion here: https://github.com/aliev/aioauth/issues/101 
        
               if response.status_code >= 400: 
        
                   content = f""" 
        
               <html> 
        
                   <body> 
        
                       <h3>{response.content['error']}</h3> 
        
                       <p style="color: red">{response.content['description']}</p> 
        
                   </body> 
        
               </html> 
        
                   """ 
        
                   return HTMLResponse(content, status_code=response.status_code) 
        
               if user is None: 
        
                   request.session["oauth"] = oauthreq 
        
                   return RedirectResponse("/login")

This PR was created based on the following discussions: #101 #102

Copilot

Copilot reviewed 8 out of 23 changed files in this pull request and generated no comments.

Files not reviewed (15)

examples/shared/config.py: Evaluated as low risk
aioauth/oidc/core/requests.py: Evaluated as low risk
tests/factories.py: Evaluated as low risk
tests/classes.py: Evaluated as low risk
tests/test_db.py: Evaluated as low risk
aioauth/errors.py: Evaluated as low risk
aioauth/grant_type.py: Evaluated as low risk
aioauth/models.py: Evaluated as low risk
aioauth/oidc/core/grant_type.py: Evaluated as low risk
aioauth/response_type.py: Evaluated as low risk
tests/conftest.py: Evaluated as low risk
tests/test_endpoint.py: Evaluated as low risk
aioauth/storage.py: Evaluated as low risk
tests/oidc/core/test_flow.py: Evaluated as low risk
aioauth/server.py: Evaluated as low risk

imgurbot12 · 2025-01-26T22:14:04Z

It will take some time for me to review and test the changes to implementation since I'm already using 2.0.0 for a current project that will require some modification to run under these potential changes but my first inclination after glancing through and reading your explanation is that this is probably a good change.

Whilst the user check was useful, this definitely adds more flexibility to the design and leaves more room for freedom for the developer. Thanks for posting! Will hopefully return soon with better feedback.

imgurbot12 · 2025-01-26T23:21:23Z

Alright, I'm apparently a bad judge of time, I actually got through the required changes faster than I thought. Thank god for unit-tests. They made ensuring all my required cases are covered much easier lol.

So I do like the changes in theory. As I said I think this gives greater developer freedom by improving the potential to make things simpler on the developer. I have a few minor points I think should probably be discussed before moving forward with implementation.

First and foremost is that by making these changes you are essentially allowing for the decoupling of authorization-codes and access/refresh-tokens from being intrinsically tied to a user identity. At a higher level the storage does not have to associate their data with the actual resource owner directly. oauth2.net explains that "Access tokens do not convey user identity or any other information about the user to the OAuth client", but by default it does relax the potential for a more proactive security posture slightly so I figure its worth stating.

Second thing of note, is that there is one case I found where the user information is probably wanted in most circumstances even if its not required and developers will likely need to make regular use of Request.extra to support. IDTokenStorage.get_id_token is responsible for building a JWT with specific openid claims including the sub or "subject" claim which is supposed to contain the user identity. This claim is technically optional according to the original RFC but its likely most implementations will want to include it.

I am probably admittedly biased towards this last little point since my implementation was already using aioauth 2.0.0 before these changes so its design was based on the "old" model, but without further changes like splitting authorization validation and response generation, the simplest method I could use to accommodate the changes and still cover all the cases I discussed here, I was basically forced to just manually re-implemented the old system back again at the top AuthorizationCodeStorage.create_authorization_code lol.

async def create_authorization_code(self, request, ...):
  if 'user' not in request.extra:
    raise InvalidClientError(request,
      description='User is not authorized',
      state=request.query.state)

Assuming we would like to move forward with intentionally separating the validation and response/authorization-code generation its likely that last point won't matter as much since create_authorization_code wont be called until after confirming the request is first validated and a user has also logged in.

All in all, after tying all that out, the caveats are relatively minor but I think they are worth noting imo. Thanks again for the PR :)

imgurbot12 · 2025-01-26T23:30:49Z

If you would like, I can try to rebase the PR for splitting validate / response generaton using the changes in this PR to see if the changes here make an improvement to those proposed updates.

aliev · 2025-02-01T15:03:03Z

If you would like, I can try to rebase the PR for splitting validate / response generaton using the changes in this PR to see if the changes here make an improvement to those proposed updates.

Yes, of course! It would be interesting to see the result with your changes.

Another main issue I didn't consider in the current PR is that when I call create_authorization_response just for validation, it shouldn't use any storage methods to avoid creating objects in the database.

Once again, sorry for the delayed response.

imgurbot12 · 2025-02-01T23:05:08Z

Created a new PR with copied changes from the old PR into a new rebase of this PR. Hope it helps :)

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

aliev · 2025-02-03T21:59:03Z

@imgurbot12 could you check the last commit? what do you think? :)

now we have three methods:

create_authorization_response, _create_authorization_response, and validate_authorization_request. If a client needs a simpler approach, they can just use create_authorization_response after verifying if the user is authorized. For more complex cases (like in examples), they can use validate_authorization_request and _create_authorization_response separately, using state for the communication between them. This approach doesn't break the tests.

imgurbot12 · 2025-02-03T23:25:17Z

@aliev, that's a great idea and a decent compromise.

I like the idea of allowing the split or having the option of keeping it altogether. I'm slightly hesitant about requiring developers to call a "private/hidden" method with _create_authorization_response. Can we rename it to something similar so its "public"? something like build_authorization_response or finalize_authorization_response perhaps?

…on_response

aliev · 2025-02-08T09:20:20Z

@aliev, that's a great idea and a decent compromise.

I like the idea of allowing the split or having the option of keeping it altogether. I'm slightly hesitant about requiring developers to call a "private/hidden" method with _create_authorization_response. Can we rename it to something similar so its "public"? something like build_authorization_response or finalize_authorization_response perhaps?

done. finalize_authorization_response looks good :)

Do you have any objections to merging this branch into master? I think this would let us close #102 and #101

…uthorizationServer

imgurbot12 · 2025-02-09T00:34:39Z

Do you have any objections to merging this branch into master? I think this would let us close #102 and #101

Nope, looks great. Go ahead thanks :)

imgurbot12 and others added 3 commits January 20, 2025 20:56

chore: merge with master, refactor for 2.0.0 release

301036b

feat: remove user

e380fe2

fix: explicitly show the response error

7352502

aliev requested a review from Copilot January 26, 2025 13:32

aliev added the v2 label Jan 26, 2025

Copilot AI reviewed Jan 26, 2025

View reviewed changes

aliev marked this pull request as ready for review January 26, 2025 13:36

aliev mentioned this pull request Jan 26, 2025

feat: separate authcode validation and response generation #102

Merged

aliev self-assigned this Jan 26, 2025

fix: passing a user through the extra argument

c215ca4

feat: split authcode into verify/response generation

3639836

imgurbot12 mentioned this pull request Feb 1, 2025

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

Merged

imgurbot12 and others added 5 commits February 2, 2025 13:46

fix: support dynamic type assignmnt from wrapped function

bfddde3

fix: allow error handling on resposne-gen up to developer

f921eed

Merge pull request #120 from imgurbot12/feat/split-authcode

69f080f

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

Merge branch 'master' into feature/remove-user-dependency

9a37286

feat: splitting create_authorization_response into two methods

d9377ac

chore: renamed _create_authorization_response to finalize_authorizati…

e7a8bd9

…on_response

refactor: enhance type annotations for grants and response types in A…

f42764d

…uthorizationServer

aliev merged commit 2a3bcbf into master Feb 9, 2025
7 checks passed

aliev deleted the feature/remove-user-dependency branch February 9, 2025 08:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: remove user#118

feat: remove user#118
aliev merged 12 commits intomasterfrom
feature/remove-user-dependency

aliev commented Jan 26, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

imgurbot12 commented Jan 26, 2025 •

edited

Loading

Uh oh!

imgurbot12 commented Jan 26, 2025 •

edited

Loading

Uh oh!

imgurbot12 commented Jan 26, 2025

Uh oh!

aliev commented Feb 1, 2025

Uh oh!

imgurbot12 commented Feb 1, 2025

Uh oh!

aliev commented Feb 3, 2025

Uh oh!

imgurbot12 commented Feb 3, 2025 •

edited

Loading

Uh oh!

aliev commented Feb 8, 2025 •

edited

Loading

Uh oh!

imgurbot12 commented Feb 9, 2025 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	if not request.user:
	raise InvalidClientError[UserType](
	request=request, description="User is not authorized", state=state
	)

	user = request.session.get("user", None)

	response = await oauth.create_authorization_response(oauthreq)

	# A demonstration example of request validation before checking the user's credentials.
	# See a discussion here: https://github.com/aliev/aioauth/issues/101
	if response.status_code >= 400:
	content = f"""
	<html>
	<body>
	<h3>{response.content['error']}</h3>
	<p style="color: red">{response.content['description']}</p>
	</body>
	</html>
	"""
	return HTMLResponse(content, status_code=response.status_code)

	if user is None:
	request.session["oauth"] = oauthreq
	return RedirectResponse("/login")

Conversation

aliev commented Jan 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

imgurbot12 commented Jan 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

imgurbot12 commented Jan 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

imgurbot12 commented Jan 26, 2025

Uh oh!

aliev commented Feb 1, 2025

Uh oh!

imgurbot12 commented Feb 1, 2025

Uh oh!

aliev commented Feb 3, 2025

Uh oh!

imgurbot12 commented Feb 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aliev commented Feb 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

imgurbot12 commented Feb 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aliev commented Jan 26, 2025 •

edited

Loading

imgurbot12 commented Jan 26, 2025 •

edited

Loading

imgurbot12 commented Jan 26, 2025 •

edited

Loading

imgurbot12 commented Feb 3, 2025 •

edited

Loading

aliev commented Feb 8, 2025 •

edited

Loading

imgurbot12 commented Feb 9, 2025 •

edited

Loading