Conversation
c34581f to
0082eef
Compare
2chanhaeng
left a comment
2chanhaeng
left a comment
There was a problem hiding this comment.
First, tests for #81 are needed.
Also, this PR seems to contain more changes related to reducing multiple origins to a single origin than changes related to #81. Wouldn’t it be better to keep the PR and create a separate issue for this? It would also be helpful to include in the issue the reasons for changing to a single origin.
It also seems that the documentation needs to be updated. Searching the codebase for DRFED_LOGIN_ORIGINS, loginOrigins, and similar terms still turns up a lot of outdated documentation.
0a58caf to
812c0c1
Compare
dahlia
left a comment
dahlia
left a comment
There was a problem hiding this comment.
Following up on @2chanhaeng's review: please add a regression test that configures https://app.example. and verifies that a login link on https://app.example is accepted. Cover the CLI configuration path and direct createYogaServer() usage, and keep IP literals allowed. The current test changes only adapt existing tests to the new interface.
I agree that the single-origin restriction belongs in a separate discussion. The reply on #81 explains why multiple origins can be useful.
| // the project's own, and the login mail would be rejected or junked. | ||
| emailFrom: opt.emailFrom ?? `noreply@${canonicalHostname(opt.rootOrigin)}`, | ||
| loginOrigins: opt.loginOrigins, | ||
| loginOrigin: opt.loginOrigin, |
There was a problem hiding this comment.
The CLI normalizes the trailing dot, but direct createYogaServer() callers bypass that parser. I reproduced this through loginByEmail: loginOrigin: new URL("https://app.example.") rejects a verify URL on https://app.example. Please normalize the configured origin at the GraphQL API boundary too.
| }), | ||
| ); | ||
|
|
||
| const loginOriginParser = option( |
There was a problem hiding this comment.
This makes --login-origin mandatory and removes support for DRFED_LOGIN_ORIGINS. DRFED_LOGIN_ORIGIN is forwarded only by scripts/dev.mts; setting it for the installed CLI still fails with Missing option --login-origin. Please keep the existing configuration working for the #81 fix, or handle the CLI migration separately and update the documentation.
| @@ -1,2 +1,2 @@ | |||
| DRFED_LOGIN_ORIGINS=https://drfed.example.com,http://localhost:3000 | |||
| DRFED_LOGIN_ORIGIN=https://drfed.example.com | |||
There was a problem hiding this comment.
The documentation still describes the old interface: packages/drfed/README.md requires DRFED_LOGIN_ORIGINS and shows a command that now fails, while packages/graphql/README.md passes loginOrigins: new Set(...). Please update these examples to match the final configuration and API. The comments in scripts/dev.mts and packages/drfed/src/parser.test.ts also still refer to the removed behavior.
| // DNS or /etc/hosts setup, which is what makes per-instance subdomains usable | ||
| // in development. | ||
| const defaultRootOrigin = "http://drfed.localhost:8888"; | ||
| const defaultLoginOrigin = "http://drfed.localhost:3000"; |
There was a problem hiding this comment.
When DRFED_LOGIN_ORIGIN is unset, this allows only http://drfed.localhost:3000, but the frontend dev server advertises http://localhost:3000/. Sign-in builds its verify URL from location.origin, so opening the advertised URL causes login to fail with an unauthorized origin. Please default to http://localhost:3000, or configure the frontend to serve and advertise drfed.localhost:3000 too.
3 participants
Closes: #81
Change:
--login-originoptions for running SeverURLof--login-originand its origin..envto set value, like--root-origin.