From 4bc7a9275edfa4bcd192bca232c8d5118593502f Mon Sep 17 00:00:00 2001
From: Ashoat Tevosyan <ashoat@comm.app>
Date: Tue, 5 Nov 2024 14:40:17 -0500
Subject: [PATCH] [services] Remove requirement to specify port for identity
 service config

Summary:
Currently, the identity service requires a port to be specified for all hosts that aren't `web.comm.app`.

However, when a port is specified that happens to be the default port for a given scheme (eg. 443 for `https` or 80 for `http`), the `url` crate ignores the port, and `url.port().is_none()` will return `true`.

As a result, there's no valid way to specify a host with a default port. This led to [ENG-9865](https://linear.app/comm/issue/ENG-9865/fix-issue-in-identity-service-logic-when-using-default-port).

My assessment here is that requiring a port isn't super necessary... if it's skipped, then it's reasonable for us to just use the default port for the given scheme.

Test Plan:
This diff is primarily removing a check, so there's not much to test beyond confirming that the things still work without the check.

1. I ran `cargo test` to confirm the unit tests still pass, especially `test_valid_origin_missing_port` now that we're not hardcoding an exception for `https://web.comm.app`
2. I'll make sure to deploy the changes to staging before deploying to prod

Reviewers: varun, will, bartek

Reviewed By: varun

Subscribers: tomek

Differential Revision: https://phab.comm.dev/D13883
---
 services/identity/src/config.rs    | 24 +++---------------------
 services/identity/src/constants.rs |  1 -
 2 files changed, 3 insertions(+), 22 deletions(-)

diff --git a/services/identity/src/config.rs b/services/identity/src/config.rs
index d9e8602c0e..bd95a08b00 100644
--- a/services/identity/src/config.rs
+++ b/services/identity/src/config.rs
@@ -9,8 +9,8 @@ use tracing::{error, info};
 use url::Url;
 
 use crate::constants::{
-  cors::ALLOW_ORIGIN_LIST, cors::PROD_ORIGIN_HOST_STR, BACKUP_SERVICE_URL,
-  BLOB_SERVICE_URL, DEFAULT_BACKUP_SERVICE_URL, DEFAULT_BLOB_SERVICE_URL,
+  cors::ALLOW_ORIGIN_LIST, BACKUP_SERVICE_URL, BLOB_SERVICE_URL,
+  DEFAULT_BACKUP_SERVICE_URL, DEFAULT_BLOB_SERVICE_URL,
   DEFAULT_OPENSEARCH_ENDPOINT, DEFAULT_TUNNELBROKER_ENDPOINT,
   KEYSERVER_PUBLIC_KEY, LOCALSTACK_ENDPOINT, OPAQUE_SERVER_SETUP,
   OPENSEARCH_ENDPOINT, REDACT_SENSITIVE_DATA, SECRETS_DIRECTORY,
@@ -180,7 +180,6 @@ pub enum Error {
 pub enum InvalidOriginError {
   InvalidScheme,
   MissingHost,
-  MissingPort,
   ParseError,
 }
 
@@ -231,15 +230,9 @@ fn validate_origin(origin_str: &str) -> Result<(), Error> {
   if !matches!(url.scheme(), "http" | "https") {
     return Err(Error::InvalidOrigin(InvalidOriginError::InvalidScheme));
   };
-  let Some(host_str) = url.host_str() else {
+  if url.host_str().is_none() {
     return Err(Error::InvalidOrigin(InvalidOriginError::MissingHost));
   };
-  if host_str == PROD_ORIGIN_HOST_STR {
-    return Ok(());
-  }
-  if url.port().is_none() {
-    return Err(Error::InvalidOrigin(InvalidOriginError::MissingPort));
-  };
   Ok(())
 }
 
@@ -276,7 +269,6 @@ mod tests {
 
   #[test]
   fn test_valid_origin_missing_port() {
-    // If the host is web.comm.app, we do not require a port
     let valid_origin = "https://web.comm.app";
     assert!(
       validate_origin(valid_origin).is_ok(),
@@ -284,16 +276,6 @@ mod tests {
     );
   }
 
-  #[test]
-  fn test_invalid_origin_missing_port() {
-    // If the host is not web.comm.app, we require a port
-    let invalid_origin = "http://localhost";
-    assert!(
-      validate_origin(invalid_origin).is_err(),
-      "Expected an invalid origin (missing port), but got a valid one"
-    );
-  }
-
   #[test]
   fn test_invalid_origin_invalid_scheme() {
     // We only allow http and https origins
diff --git a/services/identity/src/constants.rs b/services/identity/src/constants.rs
index 248d28b2f7..ed3d038fc4 100644
--- a/services/identity/src/constants.rs
+++ b/services/identity/src/constants.rs
@@ -337,7 +337,6 @@ pub mod cors {
     super::request_metadata::ACCESS_TOKEN,
   ];
   pub const ALLOW_ORIGIN_LIST: &str = "ALLOW_ORIGIN_LIST";
-  pub const PROD_ORIGIN_HOST_STR: &str = "web.comm.app";
 }
 
 // Tracing