Enhance SAML configuration instructions for Mattermost#8914
Conversation
|
Newest code from mattermost has been published to preview environment for Git SHA 8010315 |
|
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)
📝 WalkthroughWalkthroughThe Entra ID SAML guide revises enterprise-app setup, adds detailed claims-mapping rules, updates certificate and encryption instructions, and aligns Mattermost SAML configuration references with the revised steps. ChangesEntra SAML documentation
Estimated code review effort: 2 (Simple) | ~15 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
Updates the Mattermost documentation for configuring SAML with Microsoft Entra ID, with expanded guidance on setting up Attributes & Claims and identifying the correct Entra IDs needed for Mattermost SAML settings.
Changes:
- Switches the Entra enterprise app setup steps to the Entra admin center “Non-gallery” flow.
- Adds a detailed mapping explanation and a sample claim mapping table for Mattermost SAML attributes.
- Adds screenshots for the Entra Attributes & Claims page and where to find the Tenant ID.
Reviewed changes
Copilot reviewed 1 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| source/administration-guide/onboard/sso-saml-entraid.rst | Rewrites Entra setup steps; adds claim mapping guidance, notes, and new screenshots. |
| source/images/entra-tenant-id.png | New screenshot illustrating where to find Tenant ID in Entra. |
| source/images/entra-attributes-and-claims.png | New screenshot illustrating recommended simplified claim names in Entra. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Newest code from mattermost has been published to preview environment for Git SHA d2b8e2b |
|
Newest code from mattermost has been published to preview environment for Git SHA 7f0d3f8 |
|
Newest code from mattermost has been published to preview environment for Git SHA 99c7757 |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
source/administration-guide/onboard/sso-saml-entraid.rst (1)
132-140: Add a fallback note for Entra UI navigation drift.Microsoft Entra UI labels and navigation paths change frequently. Add a short fallback instruction (for example, "If labels differ from the menu shown, use the search bar to locate Enterprise apps") to reduce friction for novice admins who may encounter outdated or inconsistent UI.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/administration-guide/onboard/sso-saml-entraid.rst` around lines 132 - 140, Add a short fallback note after step 2 (the "In the left navigation menu, select **Entra ID > Enterprise applications**" instruction) to help when Microsoft Entra UI labels or navigation differ; for example, instruct editors to add a sentence like "If the menu labels differ, use the Entra search bar to find 'Enterprise applications' or 'Enterprise apps'." Ensure the note is also referenced where other menu paths are used (e.g., the "Manage > Users and groups" and "Manage > Single sign-on" steps) so readers are reminded to search the UI if they cannot find those exact labels.
🤖 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/onboard/sso-saml-entraid.rst`:
- Line 117: Update the sentence that claims the script outputs
"mattermost-x509.key" and "mattermost-x509.crt" to clarify these are the default
names: note that CRT_FILENAME (and related variables) can be configured so
actual output filenames may differ; reference the gencert.sh script and mention
"by default" and/or the variable name CRT_FILENAME so readers know to check or
override the filename if they changed configuration.
- Around line 188-191: Replace the current note admonition with a warning
admonition so the SAML username-claim guidance is shown as a risk (change the
RST block that starts with ".. note::" and contains the lines referencing
"user.mailnickname", "user.userprincipalname" and "mailnickname" to use "..
warning::" instead); ensure the block content remains identical besides the
admonition type so the explanation about using mailnickname (and the fallback
option for custom Entra attribute or UPN transformation) is preserved under the
warning.
- Around line 113-115: The chmod line currently sets restrictive permissions on
$CERT (the public certificate) instead of the private key; change the permission
target from $CERT to $KEY and ensure the private key file ($KEY) is set to a
restrictive mode (e.g., chmod 600 $KEY or chmod 400 $KEY) so the private key is
properly protected while leaving the certificate permissions unchanged.
- Line 105: Replace the CA-style basicConstraints in the OpenSSL extfile
invocation so the SAML Service Provider certificate is an end-entity, not a CA:
locate the extfile invocation that builds subjectAltName (the line containing
extfile <(echo -e
"...basicConstraints=critical,CA:true,pathlen:0\nsubjectAltName=${CRT_SAN:-..."}"))
and change the basicConstraints to basicConstraints=critical,CA:false and remove
the pathlen entry; keep the subjectAltName/CRT_SAN handling unchanged.
---
Nitpick comments:
In `@source/administration-guide/onboard/sso-saml-entraid.rst`:
- Around line 132-140: Add a short fallback note after step 2 (the "In the left
navigation menu, select **Entra ID > Enterprise applications**" instruction) to
help when Microsoft Entra UI labels or navigation differ; for example, instruct
editors to add a sentence like "If the menu labels differ, use the Entra search
bar to find 'Enterprise applications' or 'Enterprise apps'." Ensure the note is
also referenced where other menu paths are used (e.g., the "Manage > Users and
groups" and "Manage > Single sign-on" steps) so readers are reminded to search
the UI if they cannot find those exact labels.
🪄 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: ed1752f0-a712-4a34-a8d9-da4475b54733
⛔ Files ignored due to path filters (2)
source/images/entra-attributes-and-claims.pngis excluded by!**/*.pngsource/images/entra-tenant-id.pngis excluded by!**/*.png
📒 Files selected for processing (1)
source/administration-guide/onboard/sso-saml-entraid.rst
|
@Combs7th can you please give this a first pass review? |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
source/administration-guide/onboard/sso-saml-entraid.rst (8)
117-119: Cross-reference the permission fix after instructing to keep the key secure.Line 117 instructs users to "Keep the
.keyfile secure," but this advice is undermined by the incorrectchmodat line 113 (already flagged in past review comments). Consider adding a brief note here that proper file permissions are essential, and cross-reference the script's permission-setting step once that's corrected.As per coding guidelines: "Ensure commands include enough context for a technically literate admin to run them safely."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/administration-guide/onboard/sso-saml-entraid.rst` around lines 117 - 119, Add a brief note after the sentence about keeping the ``.key`` file secure that explicitly reminds admins to set restrictive file permissions and cross-reference the script's permission-setting step in the gencert.sh documentation (the permission-setting/chmod step in the generate-certificates/gencert reference). Mention the specific artifacts (``mattermost-x509.key`` and ``mattermost-x509.crt``) and point readers to the corrected permission step so they run the secure chmod when following the script.
157-157: Clarify the relationship between Name ID and Mattermost's Id Attribute setting.Line 157 mentions that "Mattermost account binding is controlled by the Id Attribute (SAML) setting if you configure it, or by email otherwise." This is technically accurate, but the sentence is dense and mixes three concepts (Name ID format, Id Attribute setting, email fallback) in a way that may confuse Novice Nate. Consider breaking this into two sentences: one explaining Name ID's role in SAML, and another explaining how Mattermost determines account binding.
Suggested restructuring
-Set the **Name identifier format** and **Source attribute** values as required for your environment. The Name ID is part of the SAML assertion, but Mattermost account binding is controlled by the **Id Attribute (SAML)** setting if you configure it, or by email otherwise. If you want immutable user binding in Mattermost, add a separate ``Id`` claim under **Additional claims** and set its **Value** (source attribute) to an immutable Entra attribute such as ``user.objectid``. +Set the **Name identifier format** and **Source attribute** values as required for your environment. The Name ID is part of every SAML assertion. Mattermost determines account binding using the **Id Attribute (SAML)** setting if configured, or by email address otherwise. If you want immutable user binding in Mattermost, add a separate ``Id`` claim under **Additional claims** and set its **Value** (source attribute) to an immutable Entra attribute such as ``user.objectid``.As per coding guidelines: "Ensure clear transitions between steps and topics; avoid gaps that jump between subjects without connection."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/administration-guide/onboard/sso-saml-entraid.rst` at line 157, Rewrite the dense sentence into two clear sentences: first state the role of the Name ID in the SAML assertion (e.g., "The Name ID is a SAML assertion attribute used to convey a user identifier and its format/source are set via Name identifier format and Source attribute"), and then explain Mattermost's account-binding order separately (e.g., "Mattermost binds accounts using the Id Attribute (SAML) if configured, and falls back to email otherwise; to ensure immutable binding add an additional 'Id' claim with its Value set to an immutable Entra attribute such as user.objectid—user.userprincipalname is a common human-readable Name ID but may break if renamed"). Reference the existing terms Name ID, Id Attribute (SAML), Additional claims, Id claim, user.objectid, and user.userprincipalname when making the split so readers can locate and update those phrases.
46-50: Clarify that CN and SAN flexibility applies specifically to SAML encryption certificates.Lines 46-50 state that the CN and SAN "don't need to match your Mattermost hostname" for "this SAML encryption certificate." While this is technically correct (Entra ID uses the certificate solely for encrypting SAML assertions, not for TLS hostname validation), novice administrators may find this confusing if they're used to TLS certificate requirements. Consider adding a brief clarification that this flexibility exists because the certificate is used for encryption key exchange in SAML, not for HTTPS/TLS.
Optional clarification
Add a sentence after line 47:
Common Name value. You can use any descriptive value for this SAML encryption certificate; it doesn't need to match your Mattermost hostname, though you can use the hostname if you prefer. +This certificate is used for SAML assertion encryption, not TLS hostname validation, so matching the hostname is optional.As per coding guidelines: "When reviewing documentation, evaluate it through the lens of Novice Nate—a novice IT Administrator with 1-2 years of experience ... can run CLI commands but wants to understand them first."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/administration-guide/onboard/sso-saml-entraid.rst` around lines 46 - 50, Clarify that the CN and SAN flexibility applies specifically to the SAML encryption certificate by adding a brief sentence after the second bullet (after the line mentioning "this SAML encryption certificate") explaining that Entra ID uses the certificate only to encrypt SAML assertions (key exchange), not for HTTPS/TLS hostname validation, so the CN and SAN do not need to match the Mattermost hostname; reference the terms CN, SAN, SAML encryption certificate, Entra ID, and Mattermost hostname when inserting the sentence.
203-203: Clarify the .cer vs .crt file extension compatibility upfront.Line 203 mentions in passing that "The Import dialog says to upload a certificate with a file extension
.cer, but.crtfiles are also accepted." This is helpful information, but it's buried mid-instruction. Consider moving this note immediately after the filename is first mentioned, or using a.. note::admonition to make it more visible, since Novice Nate might hesitate or restart when seeing the .cer requirement.Suggested restructuring
-14. In the **Mattermost** enterprise application settings, select **Security > Token encryption**. Select **Import Certificate** to import the Service Provider certificate. If you used the Bash script referenced in the **Before you begin** section, this is the ``mattermost-x509.crt`` file. The Import dialog says to upload a certificate with a file extension ``.cer``, but ``.crt`` files are also accepted. Upload the file then select **Add**. +14. In the **Mattermost** enterprise application settings, select **Security > Token encryption**. Select **Import Certificate** to import the Service Provider certificate. If you used the Bash script referenced in the **Before you begin** section, this is the ``mattermost-x509.crt`` file. + + .. note:: + The Import dialog requests a ``.cer`` file, but ``.crt`` files are also accepted. Both extensions represent X.509 certificates. + + Upload the file then select **Add**.As per coding guidelines: "Use
noteadmonition for clarifications, exceptions, non-blocking caveats, or extra context that helps the reader."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/administration-guide/onboard/sso-saml-entraid.rst` at line 203, Move the clarification about .cer vs .crt up so it immediately follows the first mention of the Service Provider certificate filename (mattermost-x509.crt) in source/administration-guide/onboard/sso-saml-entraid.rst; replace the inline parenthetical with a visible admonition using the reST note directive (.. note::) that explicitly states the Import dialog requests .cer but .crt files are accepted, so readers see it before attempting the upload. Ensure the note is placed directly after the sentence that names mattermost-x509.crt and retains the example of the Bash script reference in the "Before you begin" section.
78-83: Enhance error handling for private key generation failure.Lines 78-83 check for key generation failure but use
exitwithout a non-zero exit code. This makes it harder to detect failures in automated workflows. Additionally, for Novice Nate's benefit, the error message could mention what to check (e.g., OpenSSL installation, permissions).Suggested improvement
if [ $? -ne 0 ]; then - echo "Error generating key" - exit + echo "Error generating key. Ensure OpenSSL is installed and you have write permissions in this directory." + exit 1 fiApply similar pattern to other error checks at lines 92-95 and 107-110.
As per coding guidelines: "Include expected output or success checks after key steps to help readers verify progress" and "Define technical terms briefly inline on first use rather than assuming reader knowledge."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/administration-guide/onboard/sso-saml-entraid.rst` around lines 78 - 83, The private key generation uses "openssl genrsa -out $KEY 4096" and currently calls plain "exit" on failure; change the error handling to call "exit 1" (non-zero) and expand the echo to include actionable checks (e.g., verify OpenSSL is installed, check file permissions and available disk space), and mirror this pattern for the other key-check blocks that use plain "exit" (the subsequent openssl checks). Also add a brief expected-success message after key creation (e.g., "Private key written to $KEY") and define "private key" or "OpenSSL" inline on first use for the novice reader.
207-209: Enhance the GUID disambiguation note with examples.Lines 207-209 warn about confusing three different GUIDs but don't show what they look like or where exactly each appears in the UI. For Novice Nate, adding a brief example format (e.g., "Tenant ID: xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx, found on the Overview page") would make this more concrete and easier to verify.
As per coding guidelines: "Include expected output or success checks after key steps to help readers verify progress."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/administration-guide/onboard/sso-saml-entraid.rst` around lines 207 - 209, Update the existing disambiguation note about GUIDs (the Tenant ID, Application ID, and Object ID) to include concrete example GUID formats and brief UI locations for each so readers can verify: show example formats like "Tenant ID: 11111111-1111-1111-1111-111111111111 (found on the Azure Entra Overview page)", "Application ID: 22222222-2222-2222-2222-222222222222 (found on the App registrations or enterprise app Overview)", and "Object ID: 33333333-3333-3333-3333-333333333333 (seen on the enterprise application's Properties page)"; also add a short verification check sentence telling the user which GUIDs Mattermost uses (Tenant ID and Application ID) and a quick success check such as "Verify: the Tenant ID and Application ID match the values entered into Mattermost's SAML settings."
143-145: Use consistent placeholder formatting for Mattermost URLs.Lines 143-145 use
<your-mattermost-url>as a placeholder in literal URL values. While this works, consider whether this should be formatted as inline code or whether the placeholder should use a different convention (e.g.,{your-mattermost-url}orYOUR_MATTERMOST_URL) to make it more obvious that it's a template. Check if nearby documentation uses a consistent pattern for URL placeholders.As per coding guidelines: "Match nearby pages and local repo patterns before applying generic Markdown or English style rules."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/administration-guide/onboard/sso-saml-entraid.rst` around lines 143 - 145, The three literal URL placeholders in the SSO SAML checklist ("Identifier (Entity ID)", "Reply URL (Assertion Consumer Service URL)", and "Sign on URL") use ``<your-mattermost-url>`` inconsistently with surrounding docs; update these three placeholders to match the project’s established pattern (for example replace ``<your-mattermost-url>`` with a consistent token such as ``{your-mattermost-url}`` or ``YOUR_MATTERMOST_URL``) and render them as inline code in each of the three bullet points so they read consistently (Identifier (Entity ID), Reply URL (Assertion Consumer Service URL), Sign on URL); before committing, verify the chosen placeholder format matches nearby pages in the repo and adjust to that canonical style.
233-236: Group encryption-related settings together for clarity.Lines 233-236 introduce the encryption settings (Enable Encryption, Service Provider Private Key, Service Provider Public Certificate) but split them across steps 11-13. For Novice Nate, it would be clearer to introduce these as a group with a brief explanation that all three settings work together. Consider adding a brief note before step 11 explaining that the next three steps configure SAML assertion encryption.
As per coding guidelines: "Ensure clear transitions between steps and topics; avoid gaps that jump between subjects without connection."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/administration-guide/onboard/sso-saml-entraid.rst` around lines 233 - 236, Add a brief transitional note before step 11 explaining that the next three settings (Enable Encryption, Service Provider Private Key, Service Provider Public Certificate) configure SAML assertion encryption and must be configured together; then group steps 11–13 under that note (e.g., "Configure SAML assertion encryption:") and keep step 14 separate for signing options (Sign Request / Signature Algorithm) while referencing the recommended algorithm RSAwithSHA256 in that step.
🤖 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/onboard/sso-saml-entraid.rst`:
- Line 70: Change the umask from 007 to 077 so private key files are never
group-readable at creation (replace the line setting "umask 007" with "umask
077"), and ensure the subsequent chmod targets the generated private key
variable ($KEY) (i.e., use "chmod 600 $KEY"). Also add a warning admonition near
the section to call out the security risk if umask is not 077 so readers know
this is required for private key safety.
- Line 240: Update the paragraph that begins "The **Test single sign-on with
Mattermost SAML** tool..." to explicitly state that Entra ID's HTTP-Redirect
(GET) binding places the encoded SAMLRequest and any signature parameters
(Signature and SigAlg) into the URL query string, which can exceed Entra ID's
~4096-byte limit; explain that enabling "Sign Request" appends Signature and
SigAlg to the query string and therefore increases the risk of hitting
AADSTS90015, and add that using HTTP-POST binding (which embeds the SAML payload
and signature in the request body) avoids this query-string size constraint.
- Line 199: Update the UI wording in the sentence that lists configurable SAML
attributes: replace the phrase "Preferred Language" with the exact System
Console label "Preferred language attribute" so the sentence reads "... Guest,
Admin, Nickname, and Preferred language attribute ..." to match Mattermost's
official SAML 2.0 authentication configuration terminology.
---
Nitpick comments:
In `@source/administration-guide/onboard/sso-saml-entraid.rst`:
- Around line 117-119: Add a brief note after the sentence about keeping the
``.key`` file secure that explicitly reminds admins to set restrictive file
permissions and cross-reference the script's permission-setting step in the
gencert.sh documentation (the permission-setting/chmod step in the
generate-certificates/gencert reference). Mention the specific artifacts
(``mattermost-x509.key`` and ``mattermost-x509.crt``) and point readers to the
corrected permission step so they run the secure chmod when following the
script.
- Line 157: Rewrite the dense sentence into two clear sentences: first state the
role of the Name ID in the SAML assertion (e.g., "The Name ID is a SAML
assertion attribute used to convey a user identifier and its format/source are
set via Name identifier format and Source attribute"), and then explain
Mattermost's account-binding order separately (e.g., "Mattermost binds accounts
using the Id Attribute (SAML) if configured, and falls back to email otherwise;
to ensure immutable binding add an additional 'Id' claim with its Value set to
an immutable Entra attribute such as user.objectid—user.userprincipalname is a
common human-readable Name ID but may break if renamed"). Reference the existing
terms Name ID, Id Attribute (SAML), Additional claims, Id claim, user.objectid,
and user.userprincipalname when making the split so readers can locate and
update those phrases.
- Around line 46-50: Clarify that the CN and SAN flexibility applies
specifically to the SAML encryption certificate by adding a brief sentence after
the second bullet (after the line mentioning "this SAML encryption certificate")
explaining that Entra ID uses the certificate only to encrypt SAML assertions
(key exchange), not for HTTPS/TLS hostname validation, so the CN and SAN do not
need to match the Mattermost hostname; reference the terms CN, SAN, SAML
encryption certificate, Entra ID, and Mattermost hostname when inserting the
sentence.
- Line 203: Move the clarification about .cer vs .crt up so it immediately
follows the first mention of the Service Provider certificate filename
(mattermost-x509.crt) in
source/administration-guide/onboard/sso-saml-entraid.rst; replace the inline
parenthetical with a visible admonition using the reST note directive (..
note::) that explicitly states the Import dialog requests .cer but .crt files
are accepted, so readers see it before attempting the upload. Ensure the note is
placed directly after the sentence that names mattermost-x509.crt and retains
the example of the Bash script reference in the "Before you begin" section.
- Around line 78-83: The private key generation uses "openssl genrsa -out $KEY
4096" and currently calls plain "exit" on failure; change the error handling to
call "exit 1" (non-zero) and expand the echo to include actionable checks (e.g.,
verify OpenSSL is installed, check file permissions and available disk space),
and mirror this pattern for the other key-check blocks that use plain "exit"
(the subsequent openssl checks). Also add a brief expected-success message after
key creation (e.g., "Private key written to $KEY") and define "private key" or
"OpenSSL" inline on first use for the novice reader.
- Around line 207-209: Update the existing disambiguation note about GUIDs (the
Tenant ID, Application ID, and Object ID) to include concrete example GUID
formats and brief UI locations for each so readers can verify: show example
formats like "Tenant ID: 11111111-1111-1111-1111-111111111111 (found on the
Azure Entra Overview page)", "Application ID:
22222222-2222-2222-2222-222222222222 (found on the App registrations or
enterprise app Overview)", and "Object ID: 33333333-3333-3333-3333-333333333333
(seen on the enterprise application's Properties page)"; also add a short
verification check sentence telling the user which GUIDs Mattermost uses (Tenant
ID and Application ID) and a quick success check such as "Verify: the Tenant ID
and Application ID match the values entered into Mattermost's SAML settings."
- Around line 143-145: The three literal URL placeholders in the SSO SAML
checklist ("Identifier (Entity ID)", "Reply URL (Assertion Consumer Service
URL)", and "Sign on URL") use ``<your-mattermost-url>`` inconsistently with
surrounding docs; update these three placeholders to match the project’s
established pattern (for example replace ``<your-mattermost-url>`` with a
consistent token such as ``{your-mattermost-url}`` or ``YOUR_MATTERMOST_URL``)
and render them as inline code in each of the three bullet points so they read
consistently (Identifier (Entity ID), Reply URL (Assertion Consumer Service
URL), Sign on URL); before committing, verify the chosen placeholder format
matches nearby pages in the repo and adjust to that canonical style.
- Around line 233-236: Add a brief transitional note before step 11 explaining
that the next three settings (Enable Encryption, Service Provider Private Key,
Service Provider Public Certificate) configure SAML assertion encryption and
must be configured together; then group steps 11–13 under that note (e.g.,
"Configure SAML assertion encryption:") and keep step 14 separate for signing
options (Sign Request / Signature Algorithm) while referencing the recommended
algorithm RSAwithSHA256 in that step.
🪄 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: 649ea5b9-b005-4d10-b846-cb44fdcbbc78
📒 Files selected for processing (1)
source/administration-guide/onboard/sso-saml-entraid.rst
Combs7th
left a comment
Combs7th
left a comment
There was a problem hiding this comment.
Thanks, Eric! I finally spent some time giving this a first-pass review.
@svelle, the Entra updates look useful overall. I agree the full certificate script should be removed in favor of linking to the existing gencert.sh docs.
When you have time, please also address the remaining security comments, confirm the UI labels, and very the technical guidance and screenshots. Then re-request review and I can take another look.
Updated instructions for configuring SAML attributes and claims in Entra for Mattermost integration, including detailed explanations of required and additional claims.
svelle
added
the
preview-environment
|
Newest code from mattermost has been published to preview environment for Git SHA 6d71613 |
1 similar comment
|
Newest code from mattermost has been published to preview environment for Git SHA 6d71613 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@source/administration-guide/onboard/sso-saml-entraid.rst`:
- Line 35: Update the onboarding step to present assigning approved users and/or
groups as the default path. Move the “Assignment required? = No” alternative
into a .. warning:: directive stating that it grants Mattermost access to all
users in the tenant, while preserving the existing navigation and save
instructions.
- Line 23: Update the role prerequisites in the SAML Entra onboarding guidance
to list Cloud Application Administrator alongside Application Administrator as
supported roles, including the corresponding role reference at the other noted
location. Preserve the existing formatting and account requirement wording.
- Line 145: Clarify the “Sign Request” guidance in the SAML setup instructions
to distinguish Mattermost signing AuthnRequests from Entra ID signing responses
or assertions. If signed requests are enabled, add the Entra-side step to upload
Mattermost’s public certificate for request verification and specify that Entra
expects RSA-SHA256 for signed requests.
🪄 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: 5d573658-d59d-48f2-b1bd-9cd53ecce0a1
⛔ Files ignored due to path filters (2)
source/images/entra-attributes-and-claims.pngis excluded by!**/*.pngsource/images/entra-tenant-id.pngis excluded by!**/*.png
📒 Files selected for processing (1)
source/administration-guide/onboard/sso-saml-entraid.rst
Make assigning users/groups the default path in step 8; move the "Assignment required? = No" option into a warning directive that notes it grants access to all tenant users and prompts the reader to evaluate fit for their environment. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Clarify Sign Request step to distinguish Mattermost signing outbound AuthnRequests from Entra signing responses/assertions; specify that Entra requires RSA-SHA256 for signed requests; add missing Entra-side step to upload the SP public certificate under Verification certificates. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Newest code from mattermost has been published to preview environment for Git SHA 04d3664 |
|
You rock, Sven! It's looking much better after that round of updates. From a Novice Nate perspective, my only lingering concern is whether the cert generation section assumes a bit too much prior OpenSSL/Linux knowledge, but that's also coming from someone who's not super familiar with generating certs myself. Otherwise, it looks good to me. I'll approve from my side unless @esethna still has any concerns. |
|
Newest code from mattermost has been published to preview environment for Git SHA 7fc4d4f |
|
Thanks @Combs7th |
|
Newest code from mattermost has been published to preview environment for Git SHA 169239c |
|
Newest code from mattermost has been published to preview environment for Git SHA 88f60c8 |
* Update mattermost-v11-changelog.md (#9098) * Update generate_changelog.py * Update generate_changelog.py * Enhance SAML configuration instructions for Mattermost (#8914) * Enhance SAML configuration instructions for Mattermost Updated instructions for configuring SAML attributes and claims in Entra for Mattermost integration, including detailed explanations of required and additional claims. * Update sso-saml-entraid.rst * Update sso-saml-entraid.rst * Update sso-saml-entraid.rst * Add image for Tenant ID location in Entra Added an image reference for the Tenant ID location in Entra. * Add files via upload * Fix formatting of image attributes in SSO SAML guide * Update source/administration-guide/onboard/sso-saml-entraid.rst Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update source/administration-guide/onboard/sso-saml-entraid.rst Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update source/administration-guide/onboard/sso-saml-entraid.rst Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update source/administration-guide/onboard/sso-saml-entraid.rst Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update source/administration-guide/onboard/sso-saml-entraid.rst Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Add files via upload * Update source/administration-guide/onboard/sso-saml-entraid.rst Make assigning users/groups the default path in step 8; move the "Assignment required? = No" option into a warning directive that notes it grants access to all tenant users and prompts the reader to evaluate fit for their environment. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Update source/administration-guide/onboard/sso-saml-entraid.rst Clarify Sign Request step to distinguish Mattermost signing outbound AuthnRequests from Entra signing responses/assertions; specify that Entra requires RSA-SHA256 for signed requests; add missing Entra-side step to upload the SP public certificate under Verification certificates. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Combs7th <147677911+Combs7th@users.noreply.github.com> --------- Co-authored-by: Katie Wiersgalla <39744472+wiersgallak@users.noreply.github.com> Co-authored-by: Sven Hüster <sven@mattermost.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Combs7th <147677911+Combs7th@users.noreply.github.com>
* Bump v10.11 download link to v10.11.22 * Add v10.11.22 dot release changelog entry * Add v10.11.22 to version archive * Bump v11.7 download link to v11.7.7 * Add v11.7.7 dot release changelog entry * Add v11.7.7 to version archive * Bump current ESR to v11.7.7 * Bump current ESR to v11.7.7 * Add v11.8.4 placeholder changelog entry * Update deploy-rhel.rst * Update deploy-tar.rst * Update mattermost-server-releases.md * Update version-archive.rst * Update mattermost-v11-changelog.md * Update mattermost-v11-changelog.md * Update source/product-overview/version-archive.rst Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Update mattermost-v11-changelog.md * Update mattermost-v11-changelog.md * Update mattermost-v11-changelog.md * Update mattermost-v10-changelog.md * Update mattermost-v11-changelog.md * Update mattermost-v11-changelog.md * Update mattermost-mobile-releases.md * Update generate_changelog.py (#9099) * Update mattermost-v11-changelog.md (#9098) * Update generate_changelog.py * Update generate_changelog.py * Enhance SAML configuration instructions for Mattermost (#8914) * Enhance SAML configuration instructions for Mattermost Updated instructions for configuring SAML attributes and claims in Entra for Mattermost integration, including detailed explanations of required and additional claims. * Update sso-saml-entraid.rst * Update sso-saml-entraid.rst * Update sso-saml-entraid.rst * Add image for Tenant ID location in Entra Added an image reference for the Tenant ID location in Entra. * Add files via upload * Fix formatting of image attributes in SSO SAML guide * Update source/administration-guide/onboard/sso-saml-entraid.rst Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update source/administration-guide/onboard/sso-saml-entraid.rst Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update source/administration-guide/onboard/sso-saml-entraid.rst Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update source/administration-guide/onboard/sso-saml-entraid.rst Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update source/administration-guide/onboard/sso-saml-entraid.rst Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Add files via upload * Update source/administration-guide/onboard/sso-saml-entraid.rst Make assigning users/groups the default path in step 8; move the "Assignment required? = No" option into a warning directive that notes it grants access to all tenant users and prompts the reader to evaluate fit for their environment. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Update source/administration-guide/onboard/sso-saml-entraid.rst Clarify Sign Request step to distinguish Mattermost signing outbound AuthnRequests from Entra signing responses/assertions; specify that Entra requires RSA-SHA256 for signed requests; add missing Entra-side step to upload the SP public certificate under Verification certificates. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Combs7th <147677911+Combs7th@users.noreply.github.com> --------- Co-authored-by: Katie Wiersgalla <39744472+wiersgallak@users.noreply.github.com> Co-authored-by: Sven Hüster <sven@mattermost.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Combs7th <147677911+Combs7th@users.noreply.github.com> * Update mobile-app-changelog.md * Update mattermost-v10-changelog.md * Update mattermost-v11-changelog.md * Update version-archive.rst --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Katie Wiersgalla <39744472+wiersgallak@users.noreply.github.com> Co-authored-by: Sven Hüster <sven@mattermost.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Combs7th <147677911+Combs7th@users.noreply.github.com>
Updated instructions for configuring SAML attributes and claims in Entra for Mattermost integration, including detailed explanations of required and additional claims.