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
F3165774: D13883.id45628.diff
Wed, Nov 6, 10:49 PM
F3165773: D13883.id45627.diff
Wed, Nov 6, 10:49 PM
F3165737: D13883.diff
Wed, Nov 6, 10:48 PM
F3165531: D13883.id.diff
Wed, Nov 6, 10:04 PM
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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

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.