Conversation
|
Newest code from mattermost has been published to preview environment for Git SHA 28217c5 |
📝 WalkthroughWalkthroughDocumentation updates for Mattermost v11.8 ABAC: policies now apply to both private and public channels with distinct semantics — private channels enforce membership synchronization and block non-matching additions; public channels are advisory (no removals), supporting auto-add or “Recommended channels” discovery. UI terminology and eligibility rules updated. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/manage/admin/abac-system-wide-policies.rst (1)
23-29:⚠️ Potential issue | 🔴 CriticalCritical: Outdated text contradicts the v11.8 public/private channel distinction.
Lines 27 and 29 state that members are removed when they don't match rules, but this is factually incorrect for public channels under the v11.8 changes. According to the updated documentation throughout this PR (including lines 82-83 below), public channels never remove members via ABAC—the policy is advisory only.
This section describes the global auto-sync toggle for a parent policy but doesn't account for the fact that:
- Private channels: Members ARE removed when they no longer match, regardless of auto-sync state
- Public channels: Members are NEVER removed by ABAC, regardless of auto-sync state
The current text misleads admins into thinking public-channel members will be removed when they lose attributes.
📝 Proposed fix to reflect private/public distinction
3. Choose whether to automatically add users who match your configured attribute values as new members. Automatic synchronization is disabled by default. * **True**: Automatically maintains channel membership according to the defined rules as user attributes change. - * **False** (**Default**): Only removes members and restricts adding them to the channel if they don't match defined rules. Users are not automatically added. + * **False** (**Default**): Does not automatically add matching users. For **private** channels, non-matching members are still removed (hard gate). For **public** channels, no members are removed (advisory). - Regardless of whether this configuration setting, users who no longer match the configured attribute values in the future will be removed from the channel after the next channel synchronization. + **Private channels**: Users who no longer match the attribute values are removed during synchronization, regardless of the auto-sync setting. + **Public channels**: Users are never removed by ABAC—the policy is advisory. With auto-sync disabled, the channel appears under **Browse Channels > Recommended channels** for matching users instead of auto-adding them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/administration-guide/manage/admin/abac-system-wide-policies.rst` around lines 23 - 29, Update the paragraph describing the global auto-sync toggle for ABAC to reflect the v11.8 distinction: state that automatic synchronization is disabled by default, that for private channels ABAC will remove members who no longer match rules (regardless of auto-sync state described here), and that for public channels ABAC is advisory and will never remove members who lose attributes; specifically replace the sentences that claim members are removed on mismatch (the lines mentioning removal after the next channel synchronization) with text that explicitly differentiates private vs public channel behavior and clarifies that public-channel membership is not removed by ABAC.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@source/administration-guide/manage/admin/abac-system-wide-policies.rst`:
- Around line 23-29: Update the paragraph describing the global auto-sync toggle
for ABAC to reflect the v11.8 distinction: state that automatic synchronization
is disabled by default, that for private channels ABAC will remove members who
no longer match rules (regardless of auto-sync state described here), and that
for public channels ABAC is advisory and will never remove members who lose
attributes; specifically replace the sentences that claim members are removed on
mismatch (the lines mentioning removal after the next channel synchronization)
with text that explicitly differentiates private vs public channel behavior and
clarifies that public-channel membership is not removed by ABAC.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8a90accb-9729-48f9-814c-5931dadcd2f8
📒 Files selected for processing (4)
source/administration-guide/manage/admin/abac-channel-access-rules.rstsource/administration-guide/manage/admin/abac-system-wide-policies.rstsource/administration-guide/manage/admin/attribute-based-access-control.rstsource/end-user-guide/collaborate/manage-channel-members.rst
There was a problem hiding this comment.
Pull request overview
Adds documentation clarifying how Attribute-Based Access Control (ABAC) membership policies behave differently for private vs public channels (hard-gated vs advisory), including how this affects adding members, auto-sync, recommendations, and safety/validation behavior.
Changes:
- Updates end-user guidance for adding channel members when ABAC is enabled, differentiating public vs private behavior.
- Expands ABAC admin docs to describe public-channel advisory semantics (auto-add vs recommendations) alongside private-channel enforcement semantics.
- Updates system-wide and channel-specific ABAC policy/rules docs to reflect v11.8 support for public channels and related UI/behavior constraints (e.g., policy blocks privacy flips).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| source/end-user-guide/collaborate/manage-channel-members.rst | Explains ABAC-driven differences when adding members to private vs public channels. |
| source/administration-guide/manage/admin/attribute-based-access-control.rst | Introduces v11.8 semantics for applying ABAC policies to both private and public channels. |
| source/administration-guide/manage/admin/abac-system-wide-policies.rst | Updates system-wide policy assignment docs to cover public channels and describe behavior differences. |
| source/administration-guide/manage/admin/abac-channel-access-rules.rst | Updates channel-specific rules docs for public-channel advisory behavior, UI tab name, validation, indicators, and FAQs. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Newest code from mattermost has been published to preview environment for Git SHA a482161 |
There was a problem hiding this comment.
Actionable comments posted: 1
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/manage/admin/abac-system-wide-policies.rst (1)
24-29:⚠️ Potential issue | 🟠 MajorClarify that removal behavior applies only to private channels.
Lines 27 and 29 state that users who don't match the rules "will be removed from the channel after the next channel synchronization," but this contradicts the new public channel behavior documented at lines 82-83, which explicitly states that public channels "never removes members." This creates a factual inconsistency that will confuse readers.
Since system-wide policies now apply to both private and public channels (v11.8+), this section must distinguish between the two channel types to remain accurate.
📝 Suggested clarification
3. Choose whether to automatically add users who match your configured attribute values as new members. Automatic synchronization is disabled by default. * **True**: Automatically maintains channel membership according to the defined rules as user attributes change. - * **False** (**Default**): Only removes members and restricts adding them to the channel if they don't match defined rules. Users are not automatically added. + * **False** (**Default**): For private channels, removes members and restricts adding them if they don't match defined rules. For public channels, no members are removed (the policy is advisory), and the channel appears under **Browse Channels > Recommended channels** for matching users. Users are not automatically added. - Regardless of whether this configuration setting, users who no longer match the configured attribute values in the future will be removed from the channel after the next channel synchronization. + For private channels, users who no longer match the configured attribute values in the future will be removed from the channel after the next channel synchronization, regardless of the auto-add setting. For public channels, no members are ever removed — the policy is advisory only.Based on learnings: Describe Mattermost feature behavior, UI labels, and capability boundaries accurately.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/administration-guide/manage/admin/abac-system-wide-policies.rst` around lines 24 - 29, The paragraph describing automatic synchronization should be updated to distinguish channel types: change the sentence that currently says "users who no longer match the configured attribute values ... will be removed from the channel after the next channel synchronization" so it explicitly states this removal behavior applies only to private channels, and add a clarifying sentence that public channels are not automatically stripped of members (public channels never remove members) per v11.8+; also add a brief note that system-wide ABAC policies apply to both channel types but have different effects on membership for private vs public channels to avoid the contradiction with the public-channel behavior described later.
🤖 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/manage/admin/abac-system-wide-policies.rst`:
- Around line 107-108: Update the broken Sphinx cross-reference in the sentence
that begins "Once a policy is attached..." by changing the anchor fragment from
"validation and safety" to the hyphenated "validation-and-safety" so the :ref:
target matches the generated section slug (i.e., replace the current
:ref:`Channel-specific access rules
<administration-guide/manage/admin/abac-channel-access-rules:validation and
safety>` with :ref:`Channel-specific access rules
<administration-guide/manage/admin/abac-channel-access-rules:validation-and-safety>`).
---
Outside diff comments:
In `@source/administration-guide/manage/admin/abac-system-wide-policies.rst`:
- Around line 24-29: The paragraph describing automatic synchronization should
be updated to distinguish channel types: change the sentence that currently says
"users who no longer match the configured attribute values ... will be removed
from the channel after the next channel synchronization" so it explicitly states
this removal behavior applies only to private channels, and add a clarifying
sentence that public channels are not automatically stripped of members (public
channels never remove members) per v11.8+; also add a brief note that
system-wide ABAC policies apply to both channel types but have different effects
on membership for private vs public channels to avoid the contradiction with the
public-channel behavior described later.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 92f85b52-c159-43d2-b20e-6088dee4568a
📒 Files selected for processing (3)
source/administration-guide/manage/admin/abac-channel-access-rules.rstsource/administration-guide/manage/admin/abac-system-wide-policies.rstsource/end-user-guide/collaborate/manage-channel-members.rst
✅ Files skipped from review due to trivial changes (1)
- source/administration-guide/manage/admin/abac-channel-access-rules.rst
🚧 Files skipped from review as they are similar to previous changes (1)
- source/end-user-guide/collaborate/manage-channel-members.rst
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 2 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 2 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/manage/admin/abac-system-wide-policies.rst`:
- Around line 101-102: The sentence that says policies provide control over
"channel membership" for "private or public channel" overstates enforcement for
public channels; update the wording so it explicitly distinguishes that
assigning an access control policy to a private channel enforces membership
rules while assigning to a public channel is advisory and affects suggested or
recommended membership rather than hard enforcement—locate the phrase "channel
membership" / "private or public channel" and rewrite it to reflect "enforces
membership for private channels and provides advisory guidance for public
channels" (or similar concise phrasing).
🪄 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: 52198258-2d3d-48d8-b7a6-997432974666
📒 Files selected for processing (2)
extensions/sphinx_inline_tabs/directive.pysource/administration-guide/manage/admin/abac-system-wide-policies.rst
|
Newest code from mattermost has been published to preview environment for Git SHA 85649aa |
| if generated_tab_ids is None: | ||
| generated_tab_ids = set() | ||
| for parsed_node in parsed_nodes: | ||
| has_ids = True if hasattr(parsed_node, 'attributes') and parsed_node.attributes.get("ids") else False |
There was a problem hiding this comment.
out of curiosity, what are these changes for? are you able to answer @coderabbitai
There was a problem hiding this comment.
@pvev — great question! Here's what this specific change does and why:
Context
Inside walk_parsed_nodes, the code loops over every docutils node in the tree and logs debug information about each one via:
node_info: str = (
f"type(parsed_node)={type(parsed_node)}; "
f"has_ids={has_ids}; " # <-- uses `has_ids`
f"parent={parsed_node.parent!r}"
)
logger.debug(...)What the new line does
has_ids = True if hasattr(parsed_node, 'attributes') and parsed_node.attributes.get("ids") else FalseIt safely computes has_ids for the general case — i.e., for any node before we know its specific type. It guards against two failure modes:
AttributeError— not all docutils nodes are guaranteed to have anattributesdict, sohasattr(parsed_node, 'attributes')is checked first.KeyError— even nodes withattributesmay not have an"ids"key, so.get("ids")is used instead of direct["ids"]indexing.
Why it's needed here but not in the TabContainer/section branches
Lower in the same loop, when the code knows the node is a TabContainer or nodes.section, it recomputes has_ids locally with more precise, type-aware logic (e.g., tab_ids = parsed_node.attributes.get("ids", [])). But the generic debug log that fires for every node needed its own safe version — which is what this new line provides.
In short
It's a defensive programming / logging fix to ensure the debug log doesn't crash or produce misleading output when iterating over nodes that may lack an attributes dict or ids key.
There was a problem hiding this comment.
No, this is irrelevant. @CodeRabbit remove any changes on this file for the scope of this PR.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Get the original content of the file from the master branch
git show origin/master:extensions/sphinx_inline_tabs/directive.py
Length of output: 10239
@isacikgoz, understood. Here's the plan to revert extensions/sphinx_inline_tabs/directive.py to its original state from master.
- Restore
walk_parsed_nodesinextensions/sphinx_inline_tabs/directive.pyto its originalmasterstate by reverting all changes introduced in this PR.
The requested changes have been implemented and a pull request has been created: View PR
| - **Private channels** are hard-gated. Non-matching members are removed during synchronization, and only matching users can be added or invited. | ||
| - **Public channels** are advisory. Non-matching members are *never* removed (anyone can still join a public channel by browsing or via a direct link). With auto-add enabled the policy still pulls matching users in; with auto-add disabled the channel surfaces under **Browse Channels > Recommended channels** for users whose attributes match. | ||
|
|
||
| Each ABAC channel access policy has an explicit active state that determines whether the policy will automatically add users who meet the policy's criteria but are not yet channel members. For private channels, the policy's rules are always enforced to remove members who no longer meet the required attribute rules, regardless of the active state. For public channels, no member is ever removed by ABAC — the rules are advisory only. |
There was a problem hiding this comment.
Not new to this PR, but here we are referring to the active state and perhaps can be confusing, since we refer through the UI as "auto-add".
| - If a system policy has auto-sync disabled, Channel and Team Admins can choose to enable it for their channel. | ||
| - When no rules are configured, this toggle is automatically disabled. | ||
| - Regardless of the auto-sync setting, users who no longer meet required attribute rules are always removed during synchronization. | ||
| - On **private** channels, users who no longer meet required attribute rules are always removed during synchronization regardless of the auto-sync setting. On **public** channels, no member is ever removed by ABAC. |
There was a problem hiding this comment.
not from this PR, but how should we handle auto-add vs auto-sync?
There was a problem hiding this comment.
Soon this is going to be something else 😉
pvev
left a comment
pvev
left a comment
There was a problem hiding this comment.
a couple of nits, but nothing blocking.
Summary
Membership semantics: Private = hard gate (add + remove). Public = advisory (add-only or recommendation only). Repeated where it matters (auto-sync section, validation/safety, end-user functional restrictions, policy-assignment section).
Ticket Link
mattermost/mattermost#36275