Page MenuHomePhabricator

[identity] add allow origin list to config struct
ClosedPublic

Authored by varun on Feb 28 2024, 10:35 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Jun 18, 2:43 AM
Unknown Object (File)
Tue, Jun 18, 2:43 AM
Unknown Object (File)
Tue, Jun 18, 2:43 AM
Unknown Object (File)
Tue, Jun 18, 2:43 AM
Unknown Object (File)
Tue, Jun 18, 2:05 AM
Unknown Object (File)
Fri, Jun 14, 8:33 AM
Unknown Object (File)
May 12 2024, 11:31 PM
Unknown Object (File)
Apr 29 2024, 8:20 PM
Subscribers

Details

Summary

use env var to set AllowOrigin in config.

Depends on D11193

Test Plan

tested on staging and confirmed that all only the origins listed in terraform are allowed.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tomek requested changes to this revision.Feb 29 2024, 12:49 AM
tomek added inline comments.
services/identity/src/config.rs
99 ↗(On Diff #37683)

This approach doesn't seem to follow our practices of handling different environments. E.g. when handling webapp_url in keyserver instead of having an env variable staging or prod, we allow flexible configuration on any platform using a dedicated env variable.

Another issue: what should be the value of REMOTE_ENVIRONMENT when running the Identity service locally?

It seems like we should have an env variable dedicated to just the allowed origins for CORS and set it differently for different environments. (E.g. COMM_SERVICES_IDENTITY_ALLOWED_ORIGINS). What do you think?

services/identity/src/constants.rs
220–222 ↗(On Diff #37683)

I think it wouldn't hurt to add a couple more

This revision now requires changes to proceed.Feb 29 2024, 12:49 AM
services/identity/src/config.rs
99 ↗(On Diff #37683)

This approach doesn't seem to follow our practices of handling different environments. E.g. when handling webapp_url in keyserver instead of having an env variable staging or prod, we allow flexible configuration on any platform using a dedicated env variable.

fair enough. i've added a dedicated env var in the latest revision and now we actually enumerate the allowed origins in the previous diff in the stack (in terraform)

Another issue: what should be the value of REMOTE_ENVIRONMENT when running the Identity service locally?

.ok() converts any error to None. i.e., when running locally, REMOTE_ENVIRONMENT doesn't need to be provided.

It seems like we should have an env variable dedicated to just the allowed origins for CORS and set it differently for different environments. (E.g. COMM_SERVICES_IDENTITY_ALLOWED_ORIGINS). What do you think?

I'm good with this. didn't prefix with COMM_SERVICES_IDENTITY, though, since we don't do this for the other env vars

services/identity/src/constants.rs
220–222 ↗(On Diff #37683)

added 10 in the previous diff in stack

tomek added inline comments.
services/identity/src/config.rs
81 ↗(On Diff #37732)

How about storing a parsed list here? It would be easier to use and to debug: catching invalid configuration should happen in from_cli instead of the place where it is used.

This revision is now accepted and ready to land.Mar 4 2024, 12:48 AM
services/identity/src/config.rs
81 ↗(On Diff #37732)

your point about catching invalid config in from_cli makes sense. i think we should just call slice_to_allow_origin directly from from_cli, then. this will let us catch issues with HeaderValue::from_str, too.

varun retitled this revision from [identity] separate allow origin lists for staging and prod to [identity] add allow origin list to config struct.
varun edited the summary of this revision. (Show Details)
varun edited the test plan for this revision. (Show Details)

parse allow origin list when creating config

services/identity/src/config.rs
1–15 ↗(On Diff #37859)

this rearrangement is intentional. we should be separating imports into 3 groups: std, external, and local

services/identity/src/config.rs
94 ↗(On Diff #37859)

server_setup is more closely associated with path_buf than with keyserver_public_key so moved the blank lines around a little

This revision was landed with ongoing or failed builds.Mar 5 2024, 12:46 PM
This revision was automatically updated to reflect the committed changes.