Page MenuHomePhabricator
Differential D12719

Fix CORS issues with identity HTTP
ClosedPublic
Actions

Authored by marcin on Wed, Jul 10, 6:28 AM.
Tags
None
Referenced Files
F2369881: D12719.id42278.diff
Wed, Jul 31, 6:26 PM
Unknown Object (File)
Sun, Jul 28, 1:12 AM
Unknown Object (File)
Sat, Jul 27, 1:17 PM
Unknown Object (File)
Sat, Jul 27, 10:13 AM
Unknown Object (File)
Sat, Jul 27, 9:29 AM
Unknown Object (File)
Sat, Jul 27, 9:29 AM
Unknown Object (File)
Sat, Jul 27, 9:29 AM
Unknown Object (File)
Sat, Jul 27, 9:29 AM
View All 36 Files
Subscribers
ashoat
tomek

Details

Reviewers
varun
will
bartek
Commits
rCOMM52aa37541e68: Fix CORS issues with identity HTTP
Summary

The context is here: https://linear.app/comm/issue/ENG-8806/unable-to-call-identity-http-endpoint-from-service-workerjs-due-to. There were two CORS issues with identity HTTP. This differential applies two fixes from Varun and Will combined

Test Plan

Execute test plan for https://phab.comm.dev/D12703 (D12703). Without changes in this diff it is not possible to succesfully fetch inbound keys from service worker.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

marcin created this revision.Wed, Jul 10, 6:28 AM
Herald added subscribers: tomek, ashoat. · View Herald TranscriptWed, Jul 10, 6:28 AM
Harbormaster completed remote builds in B30265: Diff 42206.Wed, Jul 10, 7:32 AM
marcin requested review of this revision.Wed, Jul 10, 7:32 AM
will added inline comments.Wed, Jul 10, 7:41 AM
services/identity/Cargo.toml
45 ↗(On Diff #42206)

@varun Should we normalize adding cargo crates to the cargo workspace Cargo.toml?

services/identity/src/constants.rs
313 ↗(On Diff #42206)

So besides for authorization, accept is also required?

services/identity/src/websockets/mod.rs
116 ↗(On Diff #42206)

We can probably remove this comment as the code is self-explanatory

varun accepted this revision.Wed, Jul 10, 11:55 PM
varun added inline comments.
services/identity/Cargo.toml
45 ↗(On Diff #42206)

yeah good call-out. we should add this in the root Cargo.toml instead

This revision is now accepted and ready to land.Wed, Jul 10, 11:55 PM
marcin added inline comments.Fri, Jul 12, 2:42 AM
services/identity/src/constants.rs
313 ↗(On Diff #42206)

It is possible that it is not necessary but I will need a test to confirm.

marcin added inline comments.Fri, Jul 12, 3:08 AM
services/identity/src/constants.rs
313 ↗(On Diff #42206)

Confirming that accept header is not necessary

marcin updated this revision to Diff 42256.Fri, Jul 12, 3:13 AM

Address Varun's and Will's feedbacks

Harbormaster completed remote builds in B30301: Diff 42256.Fri, Jul 12, 3:36 AM
marcin updated this revision to Diff 42257.Fri, Jul 12, 5:15 AM

rebase before landing

Harbormaster completed remote builds in B30302: Diff 42257.Fri, Jul 12, 5:32 AM
will accepted this revision.Fri, Jul 12, 8:01 AM

Looks good!

marcin updated this revision to Diff 42276.Mon, Jul 15, 2:43 AM

Rebase before landing

This revision was landed with ongoing or failed builds.Mon, Jul 15, 3:04 AM
Closed by commit rCOMM52aa37541e68: Fix CORS issues with identity HTTP (authored by marcin). · Explain Why
This revision was automatically updated to reflect the committed changes.
marcin added a commit: rCOMM52aa37541e68: Fix CORS issues with identity HTTP.
Harbormaster failed remote builds in B30316: Diff 42276!Mon, Jul 15, 3:13 AM

Revision Contents
Changeset List

PathSize
1 line
services/
identity/
1 line
src/
3 lines
websockets/
9 lines

Diff 42277

View Options

Cargo.lock

Loading...
View Options

Cargo.toml

Loading...
View Options

services/identity/Cargo.toml

Loading...
View Options

services/identity/src/constants.rs

Loading...
View Options

services/identity/src/websockets/mod.rs

Loading...