Replace guest access with programmatic public access config#1233
Merged
Conversation
Move unauthenticated access control from moq-token Key fields to moq-relay configuration. Guest fields (guest, guest_sub, guest_pub) are removed from the JWK Key struct as they coupled access policy to cryptographic material and were non-standard JWK extensions. The relay's `public` config now supports three TOML formats: - Simple: `public = "anon"` or `public = ["anon", "demo"]` - Detailed: `[auth.public]` with separate `subscribe` and `publish` - URL-based: any http/https value fetches permissions dynamically, appending the connection namespace to the URL CLI flags: --auth-public, --auth-public-subscribe, --auth-public-publish Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ests The separate --auth-public-subscribe and --auth-public-publish flags overlap with PublicConfig::Detailed which handles separate directions via the TOML table format. Remove them to simplify the config surface. Add comprehensive unit tests for TOML deserialization (string, array, table, URL, subscribe-only, publish-only) and clap FromStr parsing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughGuest-related CLI flags and generation options were removed and the 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Replace manual Visitor impls and StringOrVec helper with toml::Value dispatch + serde_with OneOrMany for table fields. Also remove the redundant --auth-public-subscribe/--auth-public-publish CLI flags since PublicConfig::Detailed covers separate directions via TOML. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace Vec<PublicAccess> per-direction with a single PublicAccess struct containing subscribe/publish prefixes and an optional API URL. The API field lives alongside static prefixes so common paths like /anon avoid API calls entirely. Extract PublicDetailed struct from PublicConfig::Detailed so into_detailed() can normalize Simple → Detailed cleanly. Remove redundant --auth-public-subscribe/--auth-public-publish CLI flags. Add comprehensive TOML/clap parsing tests including the new `api` field. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace manual Serialize impl and Raw deserialization helper with derived impls using serde_as OneOrMany on the PublicDetailed struct. The only remaining custom code is the top-level TOML dispatch (string vs array vs table) which can't be expressed with derives. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
rs/moq-relay/src/auth.rs (1)
478-495: Add a test that actually exercises theapibranch.The new tests only prove that
[public] api = ...parses; nothing in this file reaches Line 519’s fetch/merge path. A small async test aroundAuth::new(... api ...)+verify()with a mocked JSON response would lock down URL joining, permission merging, and failure handling.Also applies to: 1478-1490
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-relay/src/auth.rs` around lines 478 - 495, Add an async test that constructs an Auth instance using the "[public] api = ..." configuration and then calls Auth::verify() so the code path that calls fetch_public_response() / merges permissions is exercised; set up a mocked HTTP client or middleware to intercept the GET to the joined URL and return a controlled JSON body (matching PublicResponse) or error to validate URL joining, permission merging, and error handling, and assert expected merged permissions and error variants accordingly (use Auth::new, verify(), PublicResponse and the client mock used by ClientWithMiddleware to locate hooks to inject the response).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rs/moq-relay/src/auth.rs`:
- Around line 519-523: The public API URL is built from params.path with only
the leading slash removed (path_trimmed), causing /demo and /demo/ to produce
different URLs/cache keys; normalize the namespace the same way it’s
canonicalized later by removing any trailing slash (or otherwise canonicalizing
params.path) before calling base.join and before using it as a cache key —
update the code around path_trimmed and the usage that calls base.join(...) /
Self::fetch_public_response(...) so the namespace is normalized (no trailing
slash, except for root) to match Path::new(¶ms.path) canonicalization.
- Around line 156-166: PublicConfig::into_detailed currently treats
PublicConfig::Simple(...) as static subscribe/publish prefixes, which prevents
URL shorthands from ever reaching fetch_public_response; change into_detailed so
that when the Simple variant contains a single entry that parses as an
HTTP/HTTPS URL (use Url::parse or a robust starts_with check), return
PublicDetailed with api = Some(parsed_url_string) and subscribe/publish set to
empty (or appropriate defaults) instead of duplicating the URL into prefixes;
this ensures URL-valued inputs (including those created by CLI handling that
constructs PublicConfig::Simple) are normalized into the api path and will
trigger fetch_public_response for dynamic resolution.
---
Nitpick comments:
In `@rs/moq-relay/src/auth.rs`:
- Around line 478-495: Add an async test that constructs an Auth instance using
the "[public] api = ..." configuration and then calls Auth::verify() so the code
path that calls fetch_public_response() / merges permissions is exercised; set
up a mocked HTTP client or middleware to intercept the GET to the joined URL and
return a controlled JSON body (matching PublicResponse) or error to validate URL
joining, permission merging, and error handling, and assert expected merged
permissions and error variants accordingly (use Auth::new, verify(),
PublicResponse and the client mock used by ClientWithMiddleware to locate hooks
to inject the response).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 22c6a5e9-e096-4c49-9f51-17f5bd1b6ba4
📒 Files selected for processing (1)
rs/moq-relay/src/auth.rs
- Fix cargo fmt diff (single-line try_into chain) - Normalize API namespace using Path::new for consistent URL construction - Bump moq-token 0.5.13 → 0.5.14 for semver (breaking: removed guest fields) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
rs/moq-relay/src/auth.rs (1)
156-164:⚠️ Potential issue | 🟠 MajorNormalize URL shorthands into
apiinstead of literal prefixes.
PublicConfig::Simplealways setsapi: None, sopublic = "https://..."and--auth-public https://...are still treated as path prefixes and never reachfetch_public_response. Please flip the URL-string tests with the fix too; they currently lock in the broken path.Suggested normalization
impl PublicConfig { /// Normalize into the detailed form. pub fn into_detailed(self) -> PublicDetailed { match self { + PublicConfig::Simple(mut prefixes) if prefixes.len() == 1 => { + let value = prefixes.pop().unwrap(); + if parse_url(&value).is_some() { + PublicDetailed { + subscribe: Vec::new(), + publish: Vec::new(), + api: Some(value), + } + } else { + PublicDetailed { + subscribe: vec![value.clone()], + publish: vec![value], + api: None, + } + } + } PublicConfig::Simple(prefixes) => PublicDetailed { subscribe: prefixes.clone(), publish: prefixes, api: None, }, PublicConfig::Detailed(d) => d, } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-relay/src/auth.rs` around lines 156 - 164, The PublicConfig::into_detailed implementation should treat a Simple variant that contains a URL string as an API endpoint instead of a path prefix: in into_detailed, detect when the Simple(prefixes) is a single entry matching an HTTP(S) URL (e.g., starts_with "http://" or "https://"), set PublicDetailed.api = Some(url) and set subscribe/publish to empty vectors (or otherwise not treat the URL as path prefixes), otherwise keep the existing behavior; update tests that assert URL strings become prefixes so they instead assert api is populated and that fetch_public_response will be reached. Use the symbols PublicConfig::into_detailed, PublicConfig::Simple, PublicDetailed, and fetch_public_response to locate and verify the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rs/moq-relay/src/auth.rs`:
- Around line 113-120: Add two new fields to the AuthConfig struct to expose
split CLI flags: public_subscribe and public_publish (both Option<PublicConfig>)
with corresponding #[arg(long = "auth-public-subscribe", env =
"MOQ_AUTH_PUBLIC_SUBSCRIBE")] and #[arg(long = "auth-public-publish", env =
"MOQ_AUTH_PUBLIC_PUBLISH")] attributes, include the same doc comments as the
existing public field and use #[serde(default, deserialize_with =
"PublicConfig::deserialize_option")] so TOML deserialization matches; keep the
existing public: Option<PublicConfig> field and ensure the new fields use the
same PublicConfig type and deserializer so asymmetric CLI config behaves like
the TOML variant.
---
Duplicate comments:
In `@rs/moq-relay/src/auth.rs`:
- Around line 156-164: The PublicConfig::into_detailed implementation should
treat a Simple variant that contains a URL string as an API endpoint instead of
a path prefix: in into_detailed, detect when the Simple(prefixes) is a single
entry matching an HTTP(S) URL (e.g., starts_with "http://" or "https://"), set
PublicDetailed.api = Some(url) and set subscribe/publish to empty vectors (or
otherwise not treat the URL as path prefixes), otherwise keep the existing
behavior; update tests that assert URL strings become prefixes so they instead
assert api is populated and that fetch_public_response will be reached. Use the
symbols PublicConfig::into_detailed, PublicConfig::Simple, PublicDetailed, and
fetch_public_response to locate and verify the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 628873cb-3c23-4837-aba9-b5ad455e1be3
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
rs/moq-relay/Cargo.tomlrs/moq-relay/src/auth.rsrs/moq-token/Cargo.toml
✅ Files skipped from review due to trivial changes (2)
- rs/moq-relay/Cargo.toml
- rs/moq-token/Cargo.toml
…vers path - Replace Option<PublicAccess> with PublicAccess::default() for cleaner code - Use .context() instead of .ok_or_else(|| anyhow!()) for URL parsing - Skip API call when static subscribe+publish prefixes already cover the connection path (e.g. /anon avoids the API if public.subscribe = "anon") Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
rs/moq-relay/src/auth.rs (2)
156-164:⚠️ Potential issue | 🟠 MajorNormalize URL shorthands into
api, not static prefixes.Line 160 copies
PublicConfig::Simpleintosubscribe/publish, while Line 458 only parsesd.apias a URL. As a result,public = "https://..."and--auth-public https://...are treated as literal path prefixes, sofetch_public_response()is never reached.🔧 Minimal normalization fix
impl PublicConfig { /// Normalize into the detailed form. pub fn into_detailed(self) -> PublicDetailed { match self { - PublicConfig::Simple(prefixes) => PublicDetailed { - subscribe: prefixes.clone(), - publish: prefixes, - api: None, - }, + PublicConfig::Simple(prefixes) => { + if prefixes.len() == 1 && parse_url(&prefixes[0]).is_some() { + PublicDetailed { + subscribe: vec![], + publish: vec![], + api: Some(prefixes.into_iter().next().unwrap()), + } + } else { + PublicDetailed { + subscribe: prefixes.clone(), + publish: prefixes, + api: None, + } + } + } PublicConfig::Detailed(d) => d, } } }Also applies to: 458-466
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-relay/src/auth.rs` around lines 156 - 164, PublicConfig::into_detailed currently copies Simple(prefixes) into subscribe/publish, treating URL shorthands as path prefixes; change it to detect URL shorthands in PublicConfig::Simple (e.g., single entry that starts_with "http://" or "https://", or contains "://") and set PublicDetailed.api = Some(url_string) instead of placing that entry into subscribe/publish so fetch_public_response (which reads d.api) is reached; update PublicConfig::into_detailed to branch on that pattern and preserve non-URL entries as subscribe/publish.
113-120:⚠️ Potential issue | 🟠 MajorExpose the split public CLI flags.
Line 118 still wires only
--auth-public, so asymmetric public access is still TOML-only. That leaves the new CLI surface incomplete.➕ Minimal clap surface sketch
+ #[arg(long = "auth-public-subscribe", env = "MOQ_AUTH_PUBLIC_SUBSCRIBE")] + #[serde(default, deserialize_with = "PublicConfig::deserialize_option")] + pub public_subscribe: Option<PublicConfig>, + + #[arg(long = "auth-public-publish", env = "MOQ_AUTH_PUBLIC_PUBLISH")] + #[serde(default, deserialize_with = "PublicConfig::deserialize_option")] + pub public_publish: Option<PublicConfig>,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-relay/src/auth.rs` around lines 113 - 120, The CLI currently only exposes a single --auth-public flag (attr on the field public: Option<PublicConfig> using #[arg(long = "auth-public", env = "MOQ_AUTH_PUBLIC")]) while split subscribe/publish options are only supported via TOML; add two new CLI flags --auth-public-subscribe and --auth-public-publish (with envs MOQ_AUTH_PUBLIC_SUBSCRIBE and MOQ_AUTH_PUBLIC_PUBLISH) and wire them into the same public: Option<PublicConfig> parsing path (ensure the struct deserialization via PublicConfig::deserialize_option still accepts values from these flags or map the two new clap args into PublicConfig before constructing the field), keeping backward compatibility with the existing --auth-public flag and its serde deserializer.
🧹 Nitpick comments (1)
rs/moq-relay/src/auth.rs (1)
141-153: Reject unknown keys inside[auth.public].This new nested auth table currently accepts typos silently. A misspelling like
publsih = "anon"will change the access policy instead of failing config load, which bypasses the stricter unknown-field handling at the top-level config.🛡️ Hardening tweak
#[serde_as] #[derive(Clone, Debug, Default, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] pub struct PublicDetailed {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-relay/src/auth.rs` around lines 141 - 153, Add strict unknown-field rejection to the nested auth struct by annotating PublicDetailed with serde's deny_unknown_fields. Specifically, add #[serde(deny_unknown_fields)] to the PublicDetailed declaration (alongside the existing #[serde_as] and derives) so typos like "publsih" will cause config deserialization to fail instead of silently being ignored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@rs/moq-relay/src/auth.rs`:
- Around line 156-164: PublicConfig::into_detailed currently copies
Simple(prefixes) into subscribe/publish, treating URL shorthands as path
prefixes; change it to detect URL shorthands in PublicConfig::Simple (e.g.,
single entry that starts_with "http://" or "https://", or contains "://") and
set PublicDetailed.api = Some(url_string) instead of placing that entry into
subscribe/publish so fetch_public_response (which reads d.api) is reached;
update PublicConfig::into_detailed to branch on that pattern and preserve
non-URL entries as subscribe/publish.
- Around line 113-120: The CLI currently only exposes a single --auth-public
flag (attr on the field public: Option<PublicConfig> using #[arg(long =
"auth-public", env = "MOQ_AUTH_PUBLIC")]) while split subscribe/publish options
are only supported via TOML; add two new CLI flags --auth-public-subscribe and
--auth-public-publish (with envs MOQ_AUTH_PUBLIC_SUBSCRIBE and
MOQ_AUTH_PUBLIC_PUBLISH) and wire them into the same public:
Option<PublicConfig> parsing path (ensure the struct deserialization via
PublicConfig::deserialize_option still accepts values from these flags or map
the two new clap args into PublicConfig before constructing the field), keeping
backward compatibility with the existing --auth-public flag and its serde
deserializer.
---
Nitpick comments:
In `@rs/moq-relay/src/auth.rs`:
- Around line 141-153: Add strict unknown-field rejection to the nested auth
struct by annotating PublicDetailed with serde's deny_unknown_fields.
Specifically, add #[serde(deny_unknown_fields)] to the PublicDetailed
declaration (alongside the existing #[serde_as] and derives) so typos like
"publsih" will cause config deserialization to fail instead of silently being
ignored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5b545d7b-f860-42fd-b3f0-e30e0c13cc24
📒 Files selected for processing (1)
rs/moq-relay/src/auth.rs
Allows asymmetric public access configuration via CLI flags (--auth-public-subscribe, --auth-public-publish) matching the TOML table variant. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
guest,guest_sub,guest_pubare removed from the JWKKeystruct (Rust + JS). These were non-standard JWK extensions that coupled access policy to cryptographic keys.[auth]config now supports flexible public access viapublicfield (string, array, or table with separatesubscribe/publish).http://orhttps://value triggers a fetch to{url}/{namespace}, returning{ "subscribe": [...], "publish": [...] }with HTTP cache support.TOML config examples
CLI flags
--auth-public <prefix>(both directions, existing)--auth-public-subscribe <prefix-or-url>(new)--auth-public-publish <prefix-or-url>(new)Breaking changes
moq-token:Key.guest,Key.guest_sub,Key.guest_pubfields removed. Old keys with these fields still deserialize (serde ignores unknown fields).moq-token-cli:--guest,--guest-subscribe,--guest-publishoptions removed fromgeneratecommand.@moq/token: Guest fields removed from key schema and generation.Test plan
just fix && just checkpasses🤖 Generated with Claude Code