Support claims containing dots in OAuth2 additional_scopes_key#16985
Support claims containing dots in OAuth2 additional_scopes_key#16985sunfinite wants to merge 2 commits into
Conversation
|
Tick the box to add this pull request to the merge queue (same as
|
lukebakken
left a comment
There was a problem hiding this comment.
Reviewed the change to support dot-containing claims in additional_scopes_key. The design is sound: the [[binary()]] shape correctly encodes an outer list of space-separated paths, each pre-tokenized into segment binaries, which is necessary once \. escaping means dots can no longer be re-split at runtime. Backward compatibility is preserved via the untouched is_binary clause, and there are no consumers of the field outside the plugin.
I ran the three relevant suites locally: rabbit_oauth2_schema_SUITE (22 ok), unit_SUITE (45 ok), and config_schema_SUITE (1 ok), all passing with the new tokenizer and escaped-dot coverage exercised.
I also pushed a single follow-up commit, 370e08d, adding a comment that documents the expected [[binary()]] shape of the tokenized value on the list clause of extract_scopes_from_additional_scopes_key/2.
There was a problem hiding this comment.
Thank you for the PR!
Could we preserve backward compatibility for the original binary format of extra_scopes_source?
Since some legacy or direct Erlang configurations might still define this as a flat binary (e.g., {extra_scopes_source, <<"roles">>}), it would be ideal if the validation code and the test suite supported both the original binary format and the new list-of-lists/array-of-arrays format.
I noticed {extra_scopes_source, <<"roles">>} had to be changed to {extra_scopes_source, [[<<"roles">>]]} for the existing tests to pass. Allowing the translation layer and the test cases to accept both the legacy binary and the nested list structure would be highly appreciated.
Implements changes from rabbitmq#16947
The list clause of `extract_scopes_from_additional_scopes_key/2` accepts any list, but only the `[[binary()]]` shape produced by the schema tokenizer is correct: an outer list of paths, each a list of segment binaries. A hand-written single-level list of binaries is silently treated as several top-level paths rather than one nested path. Add a comment recording this so future readers of advanced.config values understand the convention.
370e08d to
057fd12
Compare
Proposed Changes
Implements changes for #16947
OAuth2
additional_scopes_keycurrently supports nested claims separated by dots. This prevents it from accessing claims whose names contain dots such as URIs.This PR uses
cuttlefish_variable:tokenize/1for the value, which treats\. as a literal dot. The path is now tokenized once at config parse time and not per-token. Keys set directly viaadvanced.configas binaries keep the current behavior.Types of Changes
What types of changes does your code introduce to this project?
Checklist
CONTRIBUTING.mddocumentFurther Comments
Breaking case: Claim names that end with a
\followed by nesting work today but will be treated as a single token after this change. This does seem unlikely to affect anyone in practice, and any affected users can get the old behavior by setting the key inadvanced.configdirectly.