Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implicit discovery session authorization using Request context variable #621

Merged
merged 6 commits into from
Oct 17, 2024

Conversation

byewokko
Copy link
Collaborator

@byewokko byewokko commented Oct 14, 2024

Summary

  • asab.web.WebContainer stores every incoming request in asab.contextvars.Request.
  • DiscoveryService.session now uses the Authorization header from the Request context by default, so it is no longer necessary to specify auth=request.

Usage

# In web handler
async def assign_ticket_to_user(self, request):
	ticket_id = request.match_info["ticket_id"]
	user_id = request.match_info["user_id"]
	await self.TicketService.assign_ticket(ticket_id, user_id)

# In TicketService
async def assign_ticket(self, ticket_id, user_id):
	await self.Storage.put(ticket_id, user_id)

	# Notify user about ticket using emailing microservice
	# NOTE that request does not have to be passed as argument to the session method;
	#   the authorization is passed on automatically.
	async with self.DiscoveryService.session(base_url="http://email-microservice.service_id.asab") as session:
		async with session.put("/send", json={"message": "You have been assigned a ticket.", ...}) as resp:
			...

@byewokko byewokko added the enhancement New feature or request label Oct 14, 2024
@byewokko byewokko self-assigned this Oct 14, 2024
@byewokko byewokko changed the title Authorize discovery session using Request context variable Implicit discovery session authorization using Request context variable Oct 14, 2024
Comment on lines +140 to +151
@aiohttp.web.middleware
async def set_request_context(request: aiohttp.web.Request, handler):
"""
Make sure that the incoming aiohttp.web.Request is available via Request context variable
"""
request_ctx = Request.set(request)
try:
return await handler(request)
finally:
Request.reset(request_ctx)

self.WebApp.middlewares.append(set_request_context)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm not sure if asab.web.container is the right place for this piece of code

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is that needed?

Copy link
Collaborator Author

@byewokko byewokko Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at the moment mostly because of DiscoveryService.session(). most discovery session calls are (or soon will need to be) authorized which means you have to pass the authorization from the incoming request to the outgoing request.

instead of requiring devs to always pass on the request object (like DiscoveryService.session(auth=request)), it can be taken automatically from the context variable and used as the default auth value.

also, HTTP request objects should not be passed from the handler layer to the service layer. it makes sense to treat the request like a context variable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am already using this :)

Copy link
Contributor

@PremyslCerny PremyslCerny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems ok, but I really now cannot image a use case.

@byewokko byewokko merged commit 6da2080 into master Oct 17, 2024
8 checks passed
@byewokko byewokko deleted the feature/discovery-request-context branch October 17, 2024 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants