Add DCR redirect URI allowlist setting to integrations config docs#8807
Add DCR redirect URI allowlist setting to integrations config docs#8807
Conversation
Documents the new DCRRedirectURIAllowlist config.json setting and System Console field added in Mattermost server v11.5 for OAuth Dynamic Client Registration (DCR) security. Co-authored-by: Combs7th <Combs7th@users.noreply.github.com>
Combs7th
added
1: Dev Review
|
@nickmisasi - Tagging you for dev review. Do the above changes look accurate? |
|
Newest code from mattermost has been published to preview environment for Git SHA bca8f38 |
| DCR redirect URI allowlist | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| A list of permitted redirect URIs for OAuth Dynamic Client Registration (DCR). Enter one redirect URI per line. When configured, only OAuth clients that register via the DCR endpoint (``POST /api/v4/oauth/apps/register``) with a redirect URI matching an entry in this list will be accepted. Leave blank to allow any redirect URI. |
There was a problem hiding this comment.
The redirect URI's are comma separated not line separated
|
There's also a bunch of slack import stuff in here I'm not sure if is intentional. I am not the best person to review those docs |
|
@claude, please revise the docs update to address the following review feedback:
|
|
Claude finished @Combs7th's task in 5m 36s —— View job Changes completed
Here's a summary of what was done: Fix: Redirect URI description corrected In Unrelated changes removed The two other modified files were changed by unrelated commits that were in the branch history but not part of this PR's scope:
This PR now only contains the DCR redirect URI allowlist documentation changes, as intended. |
- Update DCR redirect URI allowlist description to correctly state that URIs are comma-separated (not line-separated), addressing review feedback - Revert migrate-from-slack.rst to its pre-PR state (changes from PR #8741 are unrelated to this DCR feature and should not be in this PR) - Revert deprecated-features.rst to its pre-PR state (changes from PR #8804 are unrelated to this DCR feature and should not be in this PR) Co-authored-by: Combs7th <Combs7th@users.noreply.github.com>
|
Newest code from mattermost has been published to preview environment for Git SHA 0fdaccf |
|
@nickmisasi - I think the 2nd round of corrections that Claude helped with above are likely accurate now. Is there any dev you'd recommend I add here for technical review before we move to Eric for the merging? |
|
Newest code from mattermost has been published to preview environment for Git SHA 794bb41 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughDocumentation additions for a new OAuth Dynamic Client Registration configuration setting called "DCR Redirect URI Allowlist" have been added to the integrations configuration settings guide. The changes describe the setting's purpose, JSON path, environment variable, and allowlist control behaviour. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/administration-guide/onboard/migrate-from-slack.rst (1)
10-468:⚠️ Potential issue | 🟠 MajorThis file’s changes are out of scope for the DCR allowlist PR.
These migration-guide edits should be moved to a separate PR. Keeping them here makes technical review harder and conflicts with the stated objective to focus on the new integrations config setting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/administration-guide/onboard/migrate-from-slack.rst` around lines 10 - 468, The changes to the Slack migration guide are out of scope for the DCR allowlist PR; revert or remove the edits in the migrate-from-slack.rst document from this branch and put them into a separate PR focused on documentation updates (keep the current PR limited to the new integrations config setting). Locate the edits by the top-level headings/anchors such as "Migrations Steps", ".. _preparations:", and the "Export Slack data"/"Transform the export for Mattermost" sections, revert those changes here, and create a new branch/PR containing only the migration-guide modifications so reviewers can evaluate them independently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@source/administration-guide/configure/integrations-configuration-settings.rst`:
- Around line 159-162: Remove the duplicate informational note in the
Integration Management section by keeping a single instance of the note text
"Cloud admins can't modify this configuration setting." and deleting the other
occurrence(s) (the duplicate ".. note:: Cloud admins can't modify this
configuration setting." entries) so the section contains only one note with that
exact wording.
- Around line 168-179: The doc text is inconsistent about the
DCRRedirectURIAllowlist format (says "comma-separated list" but shows a JSON
array); update the DCR redirect URI allowlist section and the example so they
match the server's actual config format for DCRRedirectURIAllowlist (either a
single comma-separated string or a JSON array of strings). Locate references to
DCRRedirectURIAllowlist and the paragraph starting "A comma-separated list..."
and either: (a) change the prose to describe the JSON array format and show the
example as "DCRRedirectURIAllowlist": ["https://...","https://..."], or (b)
change the prose to describe a comma-separated string and show the example as
"DCRRedirectURIAllowlist": "https://...,https://..."; also ensure you mention
that leaving it blank allows any redirect URI and that this setting only applies
when enable dynamic client registration is enabled.
In `@source/administration-guide/onboard/migrate-from-slack.rst`:
- Around line 26-27: Replace inconsistent tool-name variants by using the actual
canonical executable/package name everywhere—change occurrences of
"slack_advanced_exporter" and "slack-advanced-exporter" to a single standardized
form (the real package/executable name) in the document; update both the
instance on the download instruction (currently showing
``slack_advanced_exporter``) and the other occurrences referenced (lines
~161-169) so commands and prose use the exact same token.
- Around line 75-76: Fix the spelling and grammar regressions in the migration
guide by correcting the misspelled words shown in the diff and comment: change
"necesary" to "necessary", "enought" to "enough", "additioanl" to "additional",
"authenticatation" to "authentication", "conderations" to "considerations",
"associate export reflect" to the intended phrasing (e.g., "associated export
reflects"), "looks for" to the correct tense if needed, "Stared" to "Started",
and "Converations" to "Conversations"; apply the same corrections in the other
affected passages referenced (lines noted in the review: 85-86, 134-135,
365-366, 375-379, 426-427, 466-469) and proofread surrounding sentences for
minor grammar fixes so the sentences read naturally and consistently throughout
the document.
In `@source/product-overview/deprecated-features.rst`:
- Around line 6-7: Revert the unintended heading change in
deprecated-features.rst by undoing the edit that replaced or altered the
"Removed features in upcoming versions" heading; restore the file to its prior
state (remove the new/unrelated heading modification) so the PR only contains
DCR allowlist documentation changes and no out-of-scope deprecations-page edits.
---
Outside diff comments:
In `@source/administration-guide/onboard/migrate-from-slack.rst`:
- Around line 10-468: The changes to the Slack migration guide are out of scope
for the DCR allowlist PR; revert or remove the edits in the
migrate-from-slack.rst document from this branch and put them into a separate PR
focused on documentation updates (keep the current PR limited to the new
integrations config setting). Locate the edits by the top-level headings/anchors
such as "Migrations Steps", ".. _preparations:", and the "Export Slack
data"/"Transform the export for Mattermost" sections, revert those changes here,
and create a new branch/PR containing only the migration-guide modifications so
reviewers can evaluate them independently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 44ddd433-003d-44fe-8387-ceb400d884a0
📒 Files selected for processing (3)
source/administration-guide/configure/integrations-configuration-settings.rstsource/administration-guide/onboard/migrate-from-slack.rstsource/product-overview/deprecated-features.rst
| .. note:: | ||
|
|
||
| Cloud admins can't modify this configuration setting. | ||
|
|
There was a problem hiding this comment.
Duplicate Cloud-admin note appears twice for the same area.
Keep a single .. note:: Cloud admins can't modify this configuration setting. to avoid repetition in the Integration Management section.
Also applies to: 181-183
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@source/administration-guide/configure/integrations-configuration-settings.rst`
around lines 159 - 162, Remove the duplicate informational note in the
Integration Management section by keeping a single instance of the note text
"Cloud admins can't modify this configuration setting." and deleting the other
occurrence(s) (the duplicate ".. note:: Cloud admins can't modify this
configuration setting." entries) so the section contains only one note with that
exact wording.
source/administration-guide/configure/integrations-configuration-settings.rst
Outdated
Show resolved
Hide resolved
| Removed features in upcoming versions | ||
| ------------------------------------- |
There was a problem hiding this comment.
Please revert this out-of-scope deprecations-page edit.
This heading change is unrelated to the DCR allowlist documentation objective and should be excluded from this PR to keep scope clean for review and release notes hygiene.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/product-overview/deprecated-features.rst` around lines 6 - 7, Revert
the unintended heading change in deprecated-features.rst by undoing the edit
that replaced or altered the "Removed features in upcoming versions" heading;
restore the file to its prior state (remove the new/unrelated heading
modification) so the PR only contains DCR allowlist documentation changes and no
out-of-scope deprecations-page edits.
|
@claude - Please review and revise this PR so it fully addresses all remaining CodeRabbit flags and leaves the branch in a clean, mergeable state. Context: The intended scope of this PR is:
Please do all of the following:
Please then provide:
|
|
Claude finished @Combs7th's task in 8m 40s —— View job Tasks
What was changed
These files are now identical to master. The root cause of the recurring "out-of-scope" flag was subtle: the previous Claude fix (0fdaccf) had incorrectly reverted both files to versions older than master. Both PR #8741 (slack migration updates) and PR #8804 (deprecated features heading) had already merged into master before this PR branch was created, so those changes belong in the branch. Restoring from master clears the diff for both files completely.
Fixed the format contradiction between "comma-separated" and
Regarding the "duplicate Cloud admin note": After investigation, the two notes are not duplicates — the note at line 159 is for the new Final diff scopeThe PR now modifies only Remaining ambiguityOne item that a dev reviewer should confirm: whether |
… master - Update DCR redirect URI allowlist body text to clarify the two formats: System Console accepts comma-separated URIs; config.json and environment variable use a JSON string array. Adds an example of the array format. - Restore migrate-from-slack.rst to master state (previous Claude revert incorrectly removed PR #8741 changes that are already in master) - Restore deprecated-features.rst to master state (previous Claude revert incorrectly removed PR #8804 changes that are already in master) Co-authored-by: Combs7th <Combs7th@users.noreply.github.com>
|
Newest code from mattermost has been published to preview environment for Git SHA 8fb6656 |
3 participants
Documents the new DCR Redirect URI Allowlist config.json setting and System Console field added in Mattermost server v11.5 for OAuth Dynamic Client Registration (DCR) security (MM-67605).
Closes #8806
Generated with Claude Code
Summary by CodeRabbit