[identity] pull out CORS logic
ClosedPublic
Actions

Authored by varun on Nov 1 2023, 4:46 PM.

Details

Reviewers

bartek
michal
• jon
ashoat

Commits

rCOMM62a378159314: [identity] pull out CORS logic

Summary

tonic_web::enable() is convenient but doesn't let us set a custom CORS configuration. as a result, the server currently rejects request headers "code_version" and "device_type" from web. therefore, i'm modifying our Server builder to take a cors layer and a GrcpWebLayer. it's slightly more verbose, but we need this to send request metadata over grpc-web.

Test Plan

confirmed the metadata could be parsed when i sent a GenerateNonce request to my local identity service from web

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

varun created this revision.Nov 1 2023, 4:46 PM

Herald added a reviewer: • jon. · View Herald TranscriptNov 1 2023, 4:46 PM

Herald added subscribers: will, tomek, ashoat. · View Herald Transcript

varun added a child revision: D9671: [web] add flow types for grpc-web unary interceptor.Nov 1 2023, 4:58 PM

Harbormaster completed remote builds in B23649: Diff 32600.Nov 1 2023, 5:04 PM

varun requested review of this revision.Nov 1 2023, 5:04 PM

add more allowed headers for auth interceptor

Harbormaster completed remote builds in B23653: Diff 32604.Nov 1 2023, 6:44 PM

added a couple new crates, so requesting review from @ashoat

services/identity/src/constants.rs
162–168 ↗	(On Diff #32604)	these are the defaults from tonic-web

New dependencies look good. Rust code looks simple, but will leave it for somebody else to review

@michal would you mind taking a look since bartek is out the rest of the week?

Nice to see the magic string constants removed!

services/identity/src/constants.rs
153–157 ↗	(On Diff #32604)	I find it a bit cleaner to use hierarchy for cases like this but it's fine either way.
services/identity/src/cors.rs
9 ↗	(On Diff #32604)	Is `allow_methods` not needed?
10 ↗	(On Diff #32604)	In other web services (`services/comm-services-lib/src/http.rs::cors_config`) we currently set: all origins in sandbox `web.comm.app` and `localhost` (for local testing) for production We might want to do that here too (at least the production setup).
16 ↗	(On Diff #32604)	Why are these `cloned()` needed?

address @michal's feedback

services/identity/src/cors.rs
9 ↗	(On Diff #32604)	i think simple methods are allowed by default. that's why requests succeeded in testing. also i took the CorsLayer from here
16 ↗	(On Diff #32604)	The `cloned()` method takes each `&&'static str` yielded by the original iterator (the one created with `iter()`) and clones the `&'static str`, effectively turning the iterator into one that yields `&'static str`.