Page MenuHomePhabricator

[services] Remove requirement to specify port for identity service config
ClosedPublic

Authored by ashoat on Tue, Nov 5, 11:45 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 15, 6:18 AM
Unknown Object (File)
Wed, Nov 13, 5:00 AM
Unknown Object (File)
Tue, Nov 12, 10:51 AM
Unknown Object (File)
Tue, Nov 12, 10:07 AM
Unknown Object (File)
Sun, Nov 10, 10:40 PM
Unknown Object (File)
Fri, Nov 8, 1:56 PM
Unknown Object (File)
Thu, Nov 7, 6:49 AM
Unknown Object (File)
Thu, Nov 7, 5:16 AM
Subscribers

Details

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.

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

Diff Detail

Repository
rCOMM Comm
Branch
ashoat/identity
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Baby's first Rust diff! Admittedly it's rather simple, and I got some help from ChatGPT

ashoat published this revision for review.Tue, Nov 5, 11:46 AM

Will wait on identity CI to pass before landing

This comment was removed by varun.
This revision is now accepted and ready to land.Tue, Nov 5, 11:50 AM

Removed my suggestion, your solution is good

This revision was landed with ongoing or failed builds.Tue, Nov 5, 12:02 PM
This revision was automatically updated to reflect the committed changes.