Implement OIDC #1205
Implement OIDC #1205
Conversation
pizkaz
force-pushed
the
300-implement-oidc
branch
from
116b484 to
cfa39ee
Compare
pizkaz
force-pushed
the
300-implement-oidc
branch
from
cfa39ee to
9d8a961
Compare
pizkaz
force-pushed
the
300-implement-oidc
branch
from
9d8a961 to
499eea4
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1205 +/- ##
=============================================
+ Coverage 96.73% 97.34% +0.60%
=============================================
Files 405 177 -228
Lines 11642 6216 -5426
Branches 2062 2066 +4
=============================================
- Hits 11262 6051 -5211
+ Misses 380 165 -215 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Should be build from scratch using https://github.com/jumbojett/OpenID-Connect-PHP, as this implementation is missing most security checks |
pizkaz
force-pushed
the
300-implement-oidc
branch
11 times, most recently
from
863a94c to
4c06ec6
Compare
pizkaz
force-pushed
the
300-implement-oidc
branch
from
4c06ec6 to
c2256b4
Compare
pizkaz
force-pushed
the
300-implement-oidc
branch
from
c2256b4 to
b3b9b92
Compare
|
Hmm how can I convice GitHub that the requested changes were indeed addressed? 🧐 |
pizkaz
force-pushed
the
300-implement-oidc
branch
from
b3b9b92 to
3bde8c3
Compare
pizkaz
force-pushed
the
300-implement-oidc
branch
3 times, most recently
from
2c15c17 to
65e0749
Compare
pizkaz
force-pushed
the
300-implement-oidc
branch
from
65e0749 to
96e8409
Compare
![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
🧹 Nitpick comments (14)
app/Auth/OIDC/OIDCUser.php (2)
9-9: Remove unused propertyThe
$raw_attributesproperty is declared but never used in the class.- protected $raw_attributes;
15-27: Optimize attribute mapping performanceThe current implementation uses nested loops which could be inefficient for users with many attributes. Consider refactoring to use a more efficient approach.
A more efficient implementation would be:
- foreach ($attributeMap as $attribute => $oidc_attribute) { - foreach ($oidc_user as $attribute_name => $value) { - if (strcasecmp($oidc_attribute, $attribute_name) == 0) { - if (is_array($value)) { - foreach ($value as $sub_value) { - $this->addAttributeValue($attribute, $sub_value); - } - } else { - $this->addAttributeValue($attribute, $value); - } - } - } - } + // Create case-insensitive lookup array for OIDC attributes + $normalizedOidcAttributes = array_change_key_case($oidc_user, CASE_LOWER); + + foreach ($attributeMap as $attribute => $oidc_attribute) { + $oidc_attribute_lower = strtolower($oidc_attribute); + + if (isset($normalizedOidcAttributes[$oidc_attribute_lower])) { + $value = $normalizedOidcAttributes[$oidc_attribute_lower]; + if (is_array($value)) { + foreach ($value as $sub_value) { + $this->addAttributeValue($attribute, $sub_value); + } + } else { + $this->addAttributeValue($attribute, $value); + } + } + }app/Http/Controllers/api/v1/auth/LoginController.php (2)
68-69: Good introduction of a logout message placeholder.
This allows the controller to return context about incomplete or special logout states. Consider documenting the possible values of$messagefor clarity and easier debugging.
75-80: Ensure consistent handling of OIDC logout states.
When$redirectis empty and$messageis'oidc_incomplete', the frontend might need to display a clear warning or guide the user. Also, consider adding logs for this scenario (e.g., incomplete OIDC logout).Also applies to: 89-89
app/Auth/OIDC/OpenIDConnectClient.php (1)
54-62: Clean redirect approach.
OverridingredirectwithApp::abortis a practical way to mitigate “headers already sent” issues. Just be mindful thatabort(...)might bypass certain global exception handlers or logging flows.docs/docs/administration/08-advanced/01-external-authentication.md (7)
14-14: Add a comma after “Therefore.”
A minor grammar fix for clarity as flagged by static analysis.- Therefore PILOS can be connected + Therefore, PILOS can be connected🧰 Tools
🪛 LanguageTool
[uncategorized] ~14-~14: A comma may be missing after the conjunctive/linking adverb ‘Therefore’.
Context: ...practical to manage all users in PILOS. Therefore PILOS can be connected to external auth...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
133-133: Add missing determiner "the."- If you need more scopes to get all required attributes, add them as a comma separated list. + If you need more scopes to get all the required attributes, add them as a comma separated list.🧰 Tools
🪛 LanguageTool
[uncategorized] ~133-~133: You might be missing the article “the” here.
Context: ...set. If you need more scopes to get all required attributes, add them as a comma separat...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
144-144: Add a comma after “IDP.”- In your IDP you should configure the following: + In your IDP, you should configure the following:🧰 Tools
🪛 LanguageTool
[uncategorized] ~144-~144: Possible missing comma found.
Context: ...IDC_SCOPES="profile,email" ``` In your IDP you should configure the following: - ...(AI_HYDRA_LEO_MISSING_COMMA)
152-152: Add a comma after “For each external authenticator.”- For each external authenticator the attribute and role mapping needs to be configured. + For each external authenticator, the attribute and role mapping needs to be configured.🧰 Tools
🪛 LanguageTool
[typographical] ~152-~152: It appears that a comma is missing.
Context: ...## Configure mapping For each external authenticator the attribute and role mapping needs to...(DURING_THAT_TIME_COMMA)
230-230: Add a comma after “In this example.”- In this example the LDAP schema uses the common name (CN)... + In this example, the LDAP schema uses the common name (CN)...🧰 Tools
🪛 LanguageTool
[typographical] ~230-~230: It appears that a comma is missing.
Context: ...s #### LDAP ##### Attributes In this example the LDAP schema uses the common name (C...(DURING_THAT_TIME_COMMA)
281-281: Use “sent” instead of “send.”- The attribute names are the header names in which the attribute values are send... + The attribute names are the header names in which the attribute values are sent...🧰 Tools
🪛 LanguageTool
[uncategorized] ~281-~281: This verb may not be in the correct form. Consider using a different form for this context.
Context: ...names in which the attribute values are send by the apache mod_shib to the applicati...(AI_EN_LECTOR_REPLACEMENT_VERB_FORM)
332-332: Add a comma after “In this example.”- In this example the OpenID Connect provider returns... + In this example, the OpenID Connect provider returns...🧰 Tools
🪛 LanguageTool
[typographical] ~332-~332: It appears that a comma is missing.
Context: ...enID Connect ##### Attributes In this example the OpenID Connect provider returns the...(DURING_THAT_TIME_COMMA)
app/Auth/OIDC/OIDCController.php (2)
17-23: Consider validating config parameters.
Ifconfig('services.oidc.issuer'),client_id, orclient_secretare missing or invalid, initialization may fail. Proactively handle such cases to avoid runtime exceptions.
110-113: Use Laravel's request helper for consistency.
Accessing$request['id_token']and$request['logout_url']directly bypasses some safety checks. Considerrequest()->input('id_token')andrequest()->input('logout_url')for clarity and uniformity with the rest of the application.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
.env.example(1 hunks)app/Auth/OIDC/OIDCController.php(1 hunks)app/Auth/OIDC/OIDCUser.php(1 hunks)app/Auth/OIDC/OpenIDConnectClient.php(1 hunks)app/Http/Controllers/api/v1/auth/LoginController.php(3 hunks)app/Http/Middleware/VerifyCsrfToken.php(1 hunks)app/Http/Resources/Config.php(1 hunks)composer.json(1 hunks)config/services.php(2 hunks)docs/docs/administration/08-advanced/01-external-authentication.md(5 hunks)lang/de/admin.php(1 hunks)lang/de/auth.php(1 hunks)lang/en/admin.php(1 hunks)lang/en/auth.php(1 hunks)lang/fr/admin.php(1 hunks)lang/fr/auth.php(1 hunks)resources/js/components/MainNav.vue(1 hunks)resources/js/views/Login.vue(3 hunks)resources/js/views/Logout.vue(1 hunks)routes/web.php(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
app/Http/Resources/Config.php (1)
app/Http/Controllers/api/v1/ApplicationController.php (1)
config(18-21)
routes/web.php (1)
app/Auth/OIDC/OIDCController.php (1)
OIDCController(13-128)
🪛 LanguageTool
docs/docs/administration/08-advanced/01-external-authentication.md
[uncategorized] ~14-~14: A comma may be missing after the conjunctive/linking adverb ‘Therefore’.
Context: ...practical to manage all users in PILOS. Therefore PILOS can be connected to external auth...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[uncategorized] ~133-~133: You might be missing the article “the” here.
Context: ...set. If you need more scopes to get all required attributes, add them as a comma separat...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~144-~144: Possible missing comma found.
Context: ...IDC_SCOPES="profile,email" ``` In your IDP you should configure the following: - ...
(AI_HYDRA_LEO_MISSING_COMMA)
[typographical] ~152-~152: It appears that a comma is missing.
Context: ...## Configure mapping For each external authenticator the attribute and role mapping needs to...
(DURING_THAT_TIME_COMMA)
[typographical] ~230-~230: It appears that a comma is missing.
Context: ...s #### LDAP ##### Attributes In this example the LDAP schema uses the common name (C...
(DURING_THAT_TIME_COMMA)
[uncategorized] ~281-~281: This verb may not be in the correct form. Consider using a different form for this context.
Context: ...names in which the attribute values are send by the apache mod_shib to the applicati...
(AI_EN_LECTOR_REPLACEMENT_VERB_FORM)
[typographical] ~332-~332: It appears that a comma is missing.
Context: ...enID Connect ##### Attributes In this example the OpenID Connect provider returns the...
(DURING_THAT_TIME_COMMA)
🪛 markdownlint-cli2 (0.17.2)
docs/docs/administration/08-advanced/01-external-authentication.md
135-135: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
146-146: Bare URL used
null
(MD034, no-bare-urls)
147-147: Bare URL used
null
(MD034, no-bare-urls)
148-148: Bare URL used
null
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Frontend Tests (5)
- GitHub Check: Frontend Tests (4)
- GitHub Check: Frontend Tests (3)
- GitHub Check: Frontend Tests (2)
- GitHub Check: Frontend Tests (1)
🔇 Additional comments (20)
composer.json (1)
18-18:❓ Verification inconclusive
Security concern with the OIDC library.
Adding the OpenID Connect PHP library is necessary for the OIDC implementation, but according to the PR comments, SamuelWei pointed out that this library "lacks many critical security checks" and didn't pass official certification tests. This raises significant security concerns for an authentication feature.
🌐 Web query:
What are the known security issues with jumbojett/openid-connect-php library?💡 Result:
The jumbojett/openid-connect-php library has been identified with several security issues and limitations over time. Below is a summary of the known issues:
Security Issues
Unauthenticated Cross-Site Scripting (XSS):
- The library was found vulnerable to an unauthenticated XSS attack. Specifically, it accepts an
error_descriptionparameter from the$_REQUESTobject, which is used in exception messages during authentication. If an attacker controls this parameter, they can inject malicious code that could be executed when the error message is displayed. This issue was noted in the implementation of the ILIAS eLearning platform using the library[3].Basic Authentication Header Encoding Issue:
- One of the active issues on the GitHub repository highlights incorrect encoding of the Basic Authentication header in the
OpenIDConnectClient, which can cause authentication failures with some OpenID Connect providers[1].Incomplete Token Endpoint Authentication Methods:
- Older versions of the library only supported the
client_secret_basicauthentication method for token endpoints, which is less secure than other available methods likeprivate_key_jwtorclient_secret_jwt. Though some of these methods have been added in newer versions, they must be explicitly enabled, leaving room for misconfiguration or lack of awareness[2].Insufficient Testing and Updates:
- The library has been criticized for a lack of proper testing and updates to support newer PHP versions, leaving potential vulnerabilities unpatched in older versions[9].
Potential Compatibility Issues with Strict OpenID Providers:
- Certain requests from the library may be rejected by strict OpenID Connect providers (e.g., Okta). This has required manual patches in the past, such as unsetting unused parameters in token requests[8].
Development Concerns
SSL Verification Disabled in Development:
- For development purposes, SSL verification can be disabled using
setVerifyHost(false)andsetVerifyPeer(false). While this is useful for testing, it poses a significant security risk if carried over to production inadvertently[2].Missing Features in Dynamic Registration:
- The library lacks support for registration authentication tokens and endpoints in dynamic client registration, which limits its functionality in certain use cases[2].
Compatibility with Latest OpenID Standards:
- The library's adherence to the latest OpenID standards has been questioned, as reflected by the creation of forks like
jakub-onderka/openid-connect-phpto address missing features and compliance issues[9].Recommendations
- Regularly update the library to ensure the latest security fixes and features are incorporated.
- Avoid disabling SSL verification (
setVerifyHostandsetVerifyPeer) in production environments.- Consider alternatives or forks (e.g.,
jakub-onderka/openid-connect-php) if advanced features like improved token security or better PHP support are required.- Validate all inputs, especially those used in error handling, to mitigate XSS risks.
Citations:
- 1: https://github.com/jumbojett/OpenID-Connect-PHP/issues
- 2: https://github.com/jumbojett/OpenID-Connect-PHP
- 3: https://sec-consult.com/vulnerability-lab/advisory/multiple-high-risk-vulnerabilities-in-ilias-elearning-platform/
- 4: https://community.f5.com/discussions/technicalforum/oauth-2-0-openid-connect-and-claims/64328
- 5: https://dev.mydex.org/idp-api/openid-connect.html
- 6: https://openid.net/notice-of-a-security-vulnerability/
- 7: https://github.com/dptsi/php-openid-connect-client
- 8: https://www.mediawiki.org/wiki/Extension:OpenID_Connect/en
- 9: https://packagist.org/packages/jakub-onderka/openid-connect-php
- 10: https://packagist.org/packages/creativecrafts/laravel-openid-connect
Security Concern: Re-Evaluate the jumbojett/openid-connect-php Dependency
The dependency declared in composer.json is:
"jumbojett/openid-connect-php": "^1.0",Updated Findings & Recommendations:
- Unauthenticated XSS Vulnerability: The library accepts an
error_descriptionparameter (sourced from$_REQUEST) without proper sanitization—enabling potential XSS attacks.- Basic Authentication Header Issue: There’s an identified encoding problem in the Basic Authentication header within the
OpenIDConnectClient, risking failed authentications with some providers.- Limited Token Endpoint Authentication: Older versions support only
client_secret_basicby default. Although newer methods exist (e.g.,private_key_jwt,client_secret_jwt), they must be manually enabled, increasing the risk of misconfiguration.- Insufficient Testing & Compatibility: The project has been noted for inadequate testing/updates, which may leave some vulnerabilities unpatched and affect compatibility with stricter OpenID Connect providers.
- SSL Verification Concerns: While it’s possible to disable SSL verification (via
setVerifyHost(false)andsetVerifyPeer(false)) for development, this poses a major risk if accidentally carried into production.Given these issues, it's critical that the team:
- Re-assesses the usage of this library in security-sensitive authentication flows.
- Considers alternative or forked libraries (for example,
jakub-onderka/openid-connect-php) that may offer improved security and better compliance with the latest standards.- Ensures rigorous input validation and production-ready SSL settings to mitigate these risks.
.env.example (1)
80-85: LGTM - Environment variables match requirements for OIDC implementation.The environment variables follow the established pattern of other authentication methods in the file and include all necessary OIDC configuration parameters.
app/Http/Resources/Config.php (1)
104-104: LGTM - Properly exposes OIDC configuration to the frontend.The change correctly adds the OIDC enabled state to the auth configuration, consistent with how other authentication methods are exposed.
app/Http/Middleware/VerifyCsrfToken.php (1)
16-16:✅ Verification successful
CSRF exclusion for OIDC logout is appropriate.
Adding the OIDC logout endpoint to the CSRF exclusion list is consistent with the similar exclusion for Shibboleth. This exemption is typically necessary for authentication providers that handle redirects and callbacks from external identity services.
Just ensure that the corresponding controller properly validates the OIDC logout request through other means to prevent abuse.
🏁 Script executed:
#!/bin/bash # Check how the OIDC logout route is defined and protected rg -A 5 "Route.*auth/oidc/logout"Length of output: 454
CSRF Exclusion for OIDC Logout Verified
The OIDC logout endpoint is correctly excluded from CSRF protection in
app/Http/Middleware/VerifyCsrfToken.php, and the route definition inroutes/web.php(using both GET and POST) aligns with the external authentication flow patterns seen with Shibboleth. Please double-check thatOIDCController::logoutproperly validates logout requests through other mechanisms to prevent abuse.lang/en/admin.php (1)
413-413: Appropriate localization for OIDC authentication type.The OIDC authentication type has been correctly added to the English language file, maintaining consistency with other authentication methods.
lang/de/admin.php (1)
413-413: OIDC authentication type properly localized for German.The addition of the OIDC authentication type to the German language file is consistent with the other changes and provides proper localization support.
It's good to see comprehensive language support being maintained across the application.
lang/fr/admin.php (1)
413-413: LGTM: OIDC authentication type added correctly.The addition of the OIDC option in the French localization file is appropriate and consistent with the authentication types being implemented.
lang/en/auth.php (1)
40-45: LGTM: OIDC authentication strings look good.The English localization strings for OpenID Connect authentication are properly structured and provide complete coverage for the login flow and logout scenarios.
resources/js/components/MainNav.vue (1)
332-333: LGTM: Enhanced logout flow for OIDC integration.The changes to the logout function properly extract the message from the response and pass it to the logout route, enabling appropriate feedback during the OIDC logout process.
lang/de/auth.php (1)
40-44: LGTM: OIDC translations correctly implementedThe German translations for OpenID Connect authentication are properly structured and consistent with other authentication methods in the file (like LDAP and Shibboleth).
resources/js/views/Login.vue (4)
21-25: LGTM: OIDC tab implementation is correctThe OpenID Connect tab is correctly implemented with conditional rendering based on the auth.oidc setting, following the same pattern as other authentication methods.
59-69: LGTM: OIDC tab panel correctly implementedThe OIDC tab panel appropriately uses the LoginTabExternal component with the correct props, maintaining consistency with the Shibboleth implementation.
124-126: LGTM: OIDC active tab logic is correctThe active tab selection logic properly includes OIDC in its priority order, consistent with how other authentication methods are handled.
131-136: LGTM: OIDC redirect URL implementation is correctThe oidcRedirectUrl computed property correctly handles the redirect parameter, following the same pattern as the shibbolethRedirectUrl implementation.
routes/web.php (2)
3-3: LGTM: OIDCController import is correctly addedThe OIDCController class is properly imported at the top of the file.
34-39: LGTM: OIDC routes implementation is correctThe OIDC routes are properly implemented with:
- Correct middleware to conditionally enable routes
- Appropriate route methods and paths
- Proper controller method bindings
- Named routes for better referencing
The implementation follows the same pattern as the Shibboleth routes, which ensures consistency across the application.
app/Http/Controllers/api/v1/auth/LoginController.php (1)
5-5: No issues with the new import.
Bringing inOIDCControlleris a necessary step for supporting OIDC in the logout flow.app/Auth/OIDC/OpenIDConnectClient.php (1)
9-12: Session handling appears consistent.
Using Laravel'sSessionfacade integrates well for session management. Ensure session concurrency scenarios (e.g., multiple tabs) are handled as expected, though no immediate issues are evident.app/Auth/OIDC/OIDCController.php (2)
70-70: Potential open redirect risk.
Storingoidc_id_tokenin session is fine, but verify the final logout or redirect URL to ensure it can’t be exploited as an open redirect or lead to malicious sites. If necessary, restrict to known domains or patterns.
115-127: Great usage of named routes.
Returningroute('auth.oidc.signout', $params)matches Laravel best practices. This approach helps avoid hardcoding URLs. No issues found.
config/services.php
Outdated
| 'oidc' => [ | ||
| 'enabled' => $oidcEnabled, | ||
| 'issuer' => env('OIDC_ISSUER'), | ||
| 'client_id' => env('OIDC_CLIENT_ID'), | ||
| 'client_secret' => env('OIDC_CLIENT_SECRET'), | ||
| 'scopes' => explode(',', env('OIDC_SCOPES', 'profile,email')), | ||
| 'mapping' => $oidcEnabled ? json_decode(file_get_contents(app_path('Auth/config/oidc_mapping.json'))) : null, | ||
| ], |
There was a problem hiding this comment.
Add error handling for mapping file loading.
The implementation loads a mapping file but doesn't handle potential file access errors. If the oidc_mapping.json file is missing or unreadable, this will cause runtime exceptions.
Apply this change to add proper error handling:
- 'mapping' => $oidcEnabled ? json_decode(file_get_contents(app_path('Auth/config/oidc_mapping.json'))) : null,
+ 'mapping' => $oidcEnabled && file_exists(app_path('Auth/config/oidc_mapping.json')) ?
+ json_decode(file_get_contents(app_path('Auth/config/oidc_mapping.json'))) : null,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 'oidc' => [ | |
| 'enabled' => $oidcEnabled, | |
| 'issuer' => env('OIDC_ISSUER'), | |
| 'client_id' => env('OIDC_CLIENT_ID'), | |
| 'client_secret' => env('OIDC_CLIENT_SECRET'), | |
| 'scopes' => explode(',', env('OIDC_SCOPES', 'profile,email')), | |
| 'mapping' => $oidcEnabled ? json_decode(file_get_contents(app_path('Auth/config/oidc_mapping.json'))) : null, | |
| ], | |
| 'oidc' => [ | |
| 'enabled' => $oidcEnabled, | |
| 'issuer' => env('OIDC_ISSUER'), | |
| 'client_id' => env('OIDC_CLIENT_ID'), | |
| 'client_secret' => env('OIDC_CLIENT_SECRET'), | |
| 'scopes' => explode(',', env('OIDC_SCOPES', 'profile,email')), | |
| - 'mapping' => $oidcEnabled ? json_decode(file_get_contents(app_path('Auth/config/oidc_mapping.json'))) : null, | |
| + 'mapping' => $oidcEnabled && file_exists(app_path('Auth/config/oidc_mapping.json')) ? | |
| + json_decode(file_get_contents(app_path('Auth/config/oidc_mapping.json'))) : null, | |
| ], |
💡 Verification agent
🧩 Analysis chain
Verify OIDC mapping file exists in the codebase.
The code references an OIDC mapping file, but it's not clear if this file was added as part of the PR.
🏁 Script executed:
#!/bin/bash
# Check if OIDC mapping file exists in the codebase
if [ -f "app/Auth/config/oidc_mapping.json" ]; then
echo "OIDC mapping file exists"
cat "app/Auth/config/oidc_mapping.json"
else
echo "OIDC mapping file is missing"
fi
# Check if the OIDC controller and related implementations exist
echo -e "\nChecking for OIDC implementation files:"
for file in "app/Auth/OIDC/OIDCController.php" "app/Auth/OIDC/OIDCUser.php" "app/Auth/OIDC/OpenIDConnectClient.php"; do
if [ -f "$file" ]; then
echo "$file exists"
else
echo "$file is missing"
fi
doneLength of output: 964
OIDC Mapping File Missing – Action Required
The configuration in config/services.php references the OIDC mapping file (app/Auth/config/oidc_mapping.json) when OIDC is enabled. However, our verification shows that this file is missing in the repository, which may result in runtime errors if OIDC is activated.
- Action: Please add the missing
app/Auth/config/oidc_mapping.jsonfile, or update the code to gracefully handle its absence (e.g., by checking for existence before reading). - Note: The existence of the OIDC implementation files suggests that the mapping file should accompany them to ensure proper configuration.
| <Message | ||
| v-if="props.message === 'oidc_incomplete'" | ||
| severity="warn" | ||
| :closable="false" | ||
| >{{ $t("auth.oidc.logout_incomplete") }}</Message | ||
| > |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Good addition for OIDC logout state handling.
The new message block for oidc_incomplete state is well-structured and follows the pattern of existing message blocks.
However, there's a small inconsistency in the severity attribute value: the existing message above uses severity="warning" while this one uses severity="warn". Verify which is correct according to the PrimeVue Message component API.
🌐 Web query:
What are the valid severity values for PrimeVue Message component?
💡 Result:
The valid severity values for the PrimeVue Message component are:
- info
- success
- warn
- error
- secondary
- contrast
These values can be set via the severity prop on the <Message> component to indicate the type of message being displayed, such as informational, success, warning, error, secondary, or contrast messages[1][3].
Citations:
- 1: https://primevue.org/message/
- 2: https://v3.primevue.org/message/
- 3: https://tailwind.primevue.org/message/
- 4: https://primevue.org/toast/
- 5: https://v3.primevue.org/inlinemessage/
- 6: https://primevue.org/inputtext/
- 7: https://primevue.org/rating/
- 8: https://app.studyraid.com/en/read/12386/400031/message-handling
- 9: Size and Variants for Message component primefaces/primevue#6653
- 10: @primevue/forms:
Formcomponents submit event parameter missingvaluesprimefaces/primevue#7006
Action Required: Correct Inconsistent Severity Value Usage
The new OIDC logout handling addition looks excellent and follows the existing pattern. Note that the new block correctly uses severity="warn", which aligns with the PrimeVue Message component API. However, the prior message block uses severity="warning", and according to the API, it should also be warn for consistency. Please update the existing message block to use severity="warn".
| 'oidc' => [ | ||
| 'redirect' => 'Connexion', | ||
| 'tab_title' => 'OpenID Connect', | ||
| 'title' => 'Connexion avec OpenID Connect', | ||
| ], |
There was a problem hiding this comment.
Missing 'logout_incomplete' translation in the French locale.
The English version of this file includes a 'logout_incomplete' message for OIDC that's missing from the French translation. This would lead to inconsistent user experience when French users encounter an incomplete OIDC logout.
Add the missing translation:
'oidc' => [
'redirect' => 'Connexion',
'tab_title' => 'OpenID Connect',
'title' => 'Connexion avec OpenID Connect',
+ 'logout_incomplete' => 'Vous êtes toujours connecté au fournisseur OpenID Connect.',
],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 'oidc' => [ | |
| 'redirect' => 'Connexion', | |
| 'tab_title' => 'OpenID Connect', | |
| 'title' => 'Connexion avec OpenID Connect', | |
| ], | |
| 'oidc' => [ | |
| 'redirect' => 'Connexion', | |
| 'tab_title' => 'OpenID Connect', | |
| 'title' => 'Connexion avec OpenID Connect', | |
| 'logout_incomplete' => 'Vous êtes toujours connecté au fournisseur OpenID Connect.', | |
| ], |
| public function __construct(array $oidc_user) | ||
| { | ||
| $attributeMap = config('services.oidc.mapping')->attributes; | ||
|
|
||
| foreach ($attributeMap as $attribute => $oidc_attribute) { | ||
| foreach ($oidc_user as $attribute_name => $value) { | ||
| if (strcasecmp($oidc_attribute, $attribute_name) == 0) { | ||
| if (is_array($value)) { | ||
| foreach ($value as $sub_value) { | ||
| $this->addAttributeValue($attribute, $sub_value); | ||
| } | ||
| } else { | ||
| $this->addAttributeValue($attribute, $value); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add error handling for missing or invalid configuration
The constructor assumes that config('services.oidc.mapping')->attributes will always be a valid object with attributes. If the configuration is missing or invalid, this could cause runtime errors.
Consider adding validation for the mapping configuration:
public function __construct(array $oidc_user)
{
$attributeMap = config('services.oidc.mapping')->attributes;
+
+ if (!$attributeMap || !is_object($attributeMap)) {
+ \Log::error('OIDC attribute mapping configuration is missing or invalid');
+ throw new \RuntimeException('OIDC attribute mapping configuration is missing or invalid');
+ }
foreach ($attributeMap as $attribute => $oidc_attribute) {Also, verify the configuration is properly loaded:
🏁 Script executed:
#!/bin/bash
# Check if the OIDC mapping configuration is properly defined
grep -r "services.oidc.mapping" --include="*.php" app/ config/Length of output: 285
OIDC Attribute Mapping Configuration Validation Required
The constructor in app/Auth/OIDC/OIDCUser.php assumes that config('services.oidc.mapping')->attributes always returns a valid object. However, if the configuration is missing or malformed, this could lead to runtime errors.
Action Points:
- Validation Check: After retrieving the mapping configuration, validate that it is defined and is an object.
- Error Handling: If the validation fails, log an error and throw a
RuntimeExceptionto handle the error immediately. - Consistency Review: Consider if similar error handling should be applied in other parts of the code (e.g., in
OIDCController.phpwhere a related configuration (->roles) is used).
Suggested Diff:
public function __construct(array $oidc_user)
{
- $attributeMap = config('services.oidc.mapping')->attributes;
+ $attributeMap = config('services.oidc.mapping')->attributes;
+ if (!$attributeMap || !is_object($attributeMap)) {
+ \Log::error('OIDC attribute mapping configuration is missing or invalid');
+ throw new \RuntimeException('OIDC attribute mapping configuration is missing or invalid');
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function __construct(array $oidc_user) | |
| { | |
| $attributeMap = config('services.oidc.mapping')->attributes; | |
| foreach ($attributeMap as $attribute => $oidc_attribute) { | |
| foreach ($oidc_user as $attribute_name => $value) { | |
| if (strcasecmp($oidc_attribute, $attribute_name) == 0) { | |
| if (is_array($value)) { | |
| foreach ($value as $sub_value) { | |
| $this->addAttributeValue($attribute, $sub_value); | |
| } | |
| } else { | |
| $this->addAttributeValue($attribute, $value); | |
| } | |
| } | |
| } | |
| } | |
| } | |
| public function __construct(array $oidc_user) | |
| { | |
| $attributeMap = config('services.oidc.mapping')->attributes; | |
| if (!$attributeMap || !is_object($attributeMap)) { | |
| \Log::error('OIDC attribute mapping configuration is missing or invalid'); | |
| throw new \RuntimeException('OIDC attribute mapping configuration is missing or invalid'); | |
| } | |
| foreach ($attributeMap as $attribute => $oidc_attribute) { | |
| foreach ($oidc_user as $attribute_name => $value) { | |
| if (strcasecmp($oidc_attribute, $attribute_name) == 0) { | |
| if (is_array($value)) { | |
| foreach ($value as $sub_value) { | |
| $this->addAttributeValue($attribute, $sub_value); | |
| } | |
| } else { | |
| $this->addAttributeValue($attribute, $value); | |
| } | |
| } | |
| } | |
| } | |
| } |
pizkaz
force-pushed
the
300-implement-oidc
branch
from
20dc698 to
8ea767a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
docs/docs/administration/08-advanced/01-external-authentication.md (8)
14-14: Add a comma after 'Therefore' for clarity.The sentence "Therefore PILOS can be connected to external authentication systems." should have a comma after "Therefore" for correct punctuation.
-Therefore PILOS can be connected to external authentication systems. +Therefore, PILOS can be connected to external authentication systems.🧰 Tools
🪛 LanguageTool
[uncategorized] ~14-~14: Possible missing comma found.
Context: ...activated. External users In large environments it is impractical to manage all users i...(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~14-~14: A comma may be missing after the conjunctive/linking adverb ‘Therefore’.
Context: ...practical to manage all users in PILOS. Therefore PILOS can be connected to external auth...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
144-144: Add a comma after 'In your IDP' for clarity.The sentence "In your IDP you should configure the following:" should have a comma after "In your IDP".
-In your IDP you should configure the following: +In your IDP, you should configure the following:🧰 Tools
🪛 LanguageTool
[uncategorized] ~144-~144: Possible missing comma found.
Context: ...IDC_SCOPES="profile,email" ``` In your IDP you should configure the following: - ...(AI_HYDRA_LEO_MISSING_COMMA)
135-142: Specify the language for the OIDC configuration code block.For consistency and markdown best practices, specify the language as
bashfor the OIDC configuration code block.-``` +```bash🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
135-135: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
146-148: Consider formatting URLs as markdown links.Bare URLs are present for redirect URIs. For better readability and markdown linting compliance, consider formatting them as markdown links.
-- Redirect URI: https://your-domain.com/auth/oidc/callback -- Post Logout Redirect URI: https://your-domain.com/logout -- Backchannel Logout URI: https://your-domain.com/auth/oidc/logout +- Redirect URI: [https://your-domain.com/auth/oidc/callback](https://your-domain.com/auth/oidc/callback) +- Post Logout Redirect URI: [https://your-domain.com/logout](https://your-domain.com/logout) +- Backchannel Logout URI: [https://your-domain.com/auth/oidc/logout](https://your-domain.com/auth/oidc/logout)🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
146-146: Bare URL used
null(MD034, no-bare-urls)
147-147: Bare URL used
null(MD034, no-bare-urls)
148-148: Bare URL used
null(MD034, no-bare-urls)
152-152: Add a comma after 'For each external authenticator' for clarity.The sentence "For each external authenticator the attribute and role mapping needs to be configured." should have a comma after "For each external authenticator".
-For each external authenticator the attribute and role mapping needs to be configured. +For each external authenticator, the attribute and role mapping needs to be configured.🧰 Tools
🪛 LanguageTool
[typographical] ~152-~152: It appears that a comma is missing.
Context: ...## Configure mapping For each external authenticator the attribute and role mapping needs to...(DURING_THAT_TIME_COMMA)
230-230: Add a comma after 'In this example' for clarity.The sentence "In this example the LDAP schema uses the common name (CN) as username..." should have a comma after "In this example".
-In this example the LDAP schema uses the common name (CN) as username and has the group memberships in the memberof attribute. +In this example, the LDAP schema uses the common name (CN) as username and has the group memberships in the memberof attribute.🧰 Tools
🪛 LanguageTool
[typographical] ~230-~230: It appears that a comma is missing.
Context: ...s #### LDAP ##### Attributes In this example the LDAP schema uses the common name (C...(DURING_THAT_TIME_COMMA)
281-281: Use 'sent' instead of 'send' for correct verb form.The sentence "The attribute names are the header names in which the attribute values are send by the apache mod_shib to the application." should use "sent" instead of "send".
-The attribute names are the header names in which the attribute values are send by the apache mod_shib to the application. +The attribute names are the header names in which the attribute values are sent by the apache mod_shib to the application.🧰 Tools
🪛 LanguageTool
[uncategorized] ~281-~281: This verb may not be in the correct form. Consider using a different form for this context.
Context: ...names in which the attribute values are send by the apache mod_shib to the applicati...(AI_EN_LECTOR_REPLACEMENT_VERB_FORM)
332-332: Add a comma after 'In this example' for clarity.The sentence "In this example the OpenID Connect provider returns the claim..." should have a comma after "In this example".
-In this example the OpenID Connect provider returns the claim `preferred_username` which contains the username and an additional claim `roles` with an array of roles. +In this example, the OpenID Connect provider returns the claim `preferred_username` which contains the username and an additional claim `roles` with an array of roles.🧰 Tools
🪛 LanguageTool
[typographical] ~332-~332: It appears that a comma is missing.
Context: ...enID Connect ##### Attributes In this example the OpenID Connect provider returns the...(DURING_THAT_TIME_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
.env.example(1 hunks)app/Auth/OIDC/OIDCController.php(1 hunks)app/Auth/OIDC/OIDCUser.php(1 hunks)app/Auth/OIDC/OpenIDConnectClient.php(1 hunks)app/Http/Controllers/api/v1/auth/LoginController.php(3 hunks)app/Http/Middleware/VerifyCsrfToken.php(1 hunks)app/Http/Resources/Config.php(1 hunks)composer.json(1 hunks)config/services.php(2 hunks)docker/app/playback-player/build.sh(1 hunks)docs/docs/administration/08-advanced/01-external-authentication.md(5 hunks)lang/de/admin.php(1 hunks)lang/de/auth.php(1 hunks)lang/en/admin.php(1 hunks)lang/en/auth.php(1 hunks)lang/fr/admin.php(1 hunks)lang/fr/auth.php(1 hunks)resources/js/components/MainNav.vue(1 hunks)resources/js/views/Login.vue(3 hunks)resources/js/views/Logout.vue(1 hunks)routes/web.php(2 hunks)
✅ Files skipped from review due to trivial changes (4)
- app/Http/Resources/Config.php
- .env.example
- resources/js/components/MainNav.vue
- composer.json
🚧 Files skipped from review as they are similar to previous changes (15)
- lang/fr/admin.php
- lang/en/auth.php
- app/Http/Middleware/VerifyCsrfToken.php
- lang/de/auth.php
- lang/de/admin.php
- lang/fr/auth.php
- lang/en/admin.php
- resources/js/views/Login.vue
- app/Auth/OIDC/OIDCUser.php
- routes/web.php
- resources/js/views/Logout.vue
- config/services.php
- app/Http/Controllers/api/v1/auth/LoginController.php
- app/Auth/OIDC/OpenIDConnectClient.php
- app/Auth/OIDC/OIDCController.php
🧰 Additional context used
🪛 LanguageTool
docs/docs/administration/08-advanced/01-external-authentication.md
[uncategorized] ~14-~14: Possible missing comma found.
Context: ...activated. External users In large environments it is impractical to manage all users i...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~14-~14: A comma may be missing after the conjunctive/linking adverb ‘Therefore’.
Context: ...practical to manage all users in PILOS. Therefore PILOS can be connected to external auth...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[uncategorized] ~144-~144: Possible missing comma found.
Context: ...IDC_SCOPES="profile,email" ``` In your IDP you should configure the following: - ...
(AI_HYDRA_LEO_MISSING_COMMA)
[typographical] ~152-~152: It appears that a comma is missing.
Context: ...## Configure mapping For each external authenticator the attribute and role mapping needs to...
(DURING_THAT_TIME_COMMA)
[typographical] ~230-~230: It appears that a comma is missing.
Context: ...s #### LDAP ##### Attributes In this example the LDAP schema uses the common name (C...
(DURING_THAT_TIME_COMMA)
[uncategorized] ~281-~281: This verb may not be in the correct form. Consider using a different form for this context.
Context: ...names in which the attribute values are send by the apache mod_shib to the applicati...
(AI_EN_LECTOR_REPLACEMENT_VERB_FORM)
[typographical] ~332-~332: It appears that a comma is missing.
Context: ...enID Connect ##### Attributes In this example the OpenID Connect provider returns the...
(DURING_THAT_TIME_COMMA)
🪛 markdownlint-cli2 (0.17.2)
docs/docs/administration/08-advanced/01-external-authentication.md
135-135: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
146-146: Bare URL used
null
(MD034, no-bare-urls)
147-147: Bare URL used
null
(MD034, no-bare-urls)
148-148: Bare URL used
null
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Frontend Tests (5)
- GitHub Check: Frontend Tests (4)
- GitHub Check: Frontend Tests (3)
- GitHub Check: Frontend Tests (2)
- GitHub Check: Frontend Tests (1)
- GitHub Check: Visual Tests
- GitHub Check: Backend
🔇 Additional comments (2)
docker/app/playback-player/build.sh (1)
25-33: Improved error handling for extraction and directory change—LGTM!The refactor to check the success of
unzipandcddirectly within conditional blocks is robust and idiomatic. This prevents the script from proceeding on extraction or directory errors, making the build process safer and easier to debug.No issues found—this is a solid improvement.
docs/docs/administration/08-advanced/01-external-authentication.md (1)
1-361: Comprehensive and clear OIDC documentation.The documentation for OIDC integration is thorough, clear, and consistent with the rest of the file. It provides actionable steps for configuration, mapping, and role assignment, and aligns with the implementation described in the PR objectives and AI summary.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~14-~14: Possible missing comma found.
Context: ...activated. External users In large environments it is impractical to manage all users i...(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~14-~14: A comma may be missing after the conjunctive/linking adverb ‘Therefore’.
Context: ...practical to manage all users in PILOS. Therefore PILOS can be connected to external auth...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[misspelling] ~96-~96: Did you mean “to”?
Context: ...roxyPass /shibboleth-sp ! ``` You need the add the url of the font- and back-chann...(THE_TO)
[style] ~105-~105: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...e shibboleth To enable Shibboleth, you need to enable it in the.envfile. ```bash ...(REP_NEED_TO_VB)
[uncategorized] ~122-~122: Possible missing comma found.
Context: ...LOS. To disable the shibboleth session check you can set the following.envvariab...(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~144-~144: Possible missing comma found.
Context: ...IDC_SCOPES="profile,email" ``` In your IDP you should configure the following: - ...(AI_HYDRA_LEO_MISSING_COMMA)
[typographical] ~152-~152: It appears that a comma is missing.
Context: ...## Configure mapping For each external authenticator the attribute and role mapping needs to...(DURING_THAT_TIME_COMMA)
[typographical] ~200-~200: The conjunction “so that” does not require a comma.
Context: ...owever, if you want to combine the rules, so that every rule must be fulfilled, set the a...(SO_THAT_UNNECESSARY_COMMA)
[grammar] ~205-~205: “Expression” is a singular noun. It appears that the verb form is incorrect.
Context: ...r expression. If the regular expression find a match the rule is fulfilled. To crea...(PCT_SINGULAR_NOUN_PLURAL_VERB_AGREEMENT)
[uncategorized] ~205-~205: A comma might be missing here.
Context: ...ssion. If the regular expression find a match the rule is fulfilled. To create and t...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[typographical] ~230-~230: It appears that a comma is missing.
Context: ...s #### LDAP ##### Attributes In this example the LDAP schema uses the common name (C...(DURING_THAT_TIME_COMMA)
[uncategorized] ~281-~281: This verb may not be in the correct form. Consider using a different form for this context.
Context: ...names in which the attribute values are send by the apache mod_shib to the applicati...(AI_EN_LECTOR_REPLACEMENT_VERB_FORM)
[typographical] ~332-~332: It appears that a comma is missing.
Context: ...enID Connect ##### Attributes In this example the OpenID Connect provider returns the...(DURING_THAT_TIME_COMMA)
🪛 markdownlint-cli2 (0.17.2)
135-135: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
146-146: Bare URL used
null(MD034, no-bare-urls)
147-147: Bare URL used
null(MD034, no-bare-urls)
148-148: Bare URL used
null(MD034, no-bare-urls)
207-207: Bare URL used
null(MD034, no-bare-urls)
207-207: Bare URL used
null(MD034, no-bare-urls)
SamuelWei
force-pushed
the
300-implement-oidc
branch
from
8ea767a to
6509a08
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
app/Auth/OIDC/OIDCController.php (2)
54-62: Validate user data before creating the Eloquent model.The code creates or finds a user model and syncs attributes, but could benefit from additional validation to ensure data integrity.
// Get eloquent user (existing or new) $user = $oidc_user->createOrFindEloquentModel('oidc'); // Sync attributes try { + // Validate that essential user data is valid before syncing + if (empty($user->email) || !filter_var($user->email, FILTER_VALIDATE_EMAIL)) { + Log::warning('Invalid email format for OIDC user', ['sub' => $user_info['sub']]); + return redirect('/external_login?error=invalid_user_data'); + } $oidc_user->syncWithEloquentModel($user, config('services.oidc.mapping')->roles); } catch (MissingAttributeException $e) { return redirect('/external_login?error=missing_attributes'); }
82-105: Enhance backchannel logout security measures.The backchannel logout implementation could benefit from additional security measures like IP validation and rate limiting to prevent abuse.
/** * Backchannel logout */ public function logout(Request $request) { Log::debug('OIDC backchannel logout handler called'); + // Optional: Add rate limiting to prevent abuse + // if (RateLimiter::tooManyAttempts('oidc-logout:'.$request->ip(), 10)) { + // Log::warning('Rate limit exceeded for OIDC logout from IP: ' . $request->ip()); + // return response('Too many attempts', 429); + // } if (! $this->oidc->verifyLogoutToken()) { Log::warning('Logout token verification failed'); + // Add more context to the log for security monitoring + Log::warning('Logout token verification failed', [ + 'ip' => $request->ip(), + 'user_agent' => $request->userAgent() + ]); return; } $sub = $this->oidc->getSubjectFromBackChannel(); if (! isset($sub)) { Log::warning('Getting subject from backchannel failed'); return; } $lookupSessions = SessionData::where('key', 'oidc_sub')->where('value', $sub)->get(); foreach ($lookupSessions as $lookupSession) { $user = $lookupSession->session->user->getLogLabel(); Log::info('Deleting session of user {user}', ['user' => $user, 'type' => 'oidc']); $lookupSession->session()->delete(); } }docs/docs/administration/08-advanced/01-external-authentication.md (5)
14-14: Add a comma after "Therefore" for better readability.The sentence would read more smoothly with a comma after the conjunctive adverb.
-In large environments it is impractical to manage all users in PILOS. Therefore PILOS can be connected to external authentication systems. LDAP, Shibboleth and OpenID-Connect are available as interfaces. All three authentication providers can be operated in parallel, but none of them more than once. +In large environments it is impractical to manage all users in PILOS. Therefore, PILOS can be connected to external authentication systems. LDAP, Shibboleth and OpenID-Connect are available as interfaces. All three authentication providers can be operated in parallel, but none of them more than once.🧰 Tools
🪛 LanguageTool
[uncategorized] ~14-~14: A comma may be missing after the conjunctive/linking adverb ‘Therefore’.
Context: ...practical to manage all users in PILOS. Therefore PILOS can be connected to external auth...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
133-142: Specify a language for the code block.Adding a language identifier to the code block will enable proper syntax highlighting.
-``` +```bash # OpenID Connect config OIDC_ENABLED=true OIDC_ISSUER=http://idp.university.org OIDC_CLIENT_ID=my_client_id OIDC_CLIENT_SECRET=my_client_secret OIDC_SCOPES="profile,email"🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
135-135: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
144-149: Format URLs consistently with Markdown syntax.The URLs should be formatted consistently with Markdown syntax instead of using bare URLs.
-In your IDP you should configure the following: +In your IDP, you should configure the following: -- Redirect URI: https://your-domain.com/auth/oidc/callback -- Post Logout Redirect URI: https://your-domain.com/logout -- Backchannel Logout URI: https://your-domain.com/auth/oidc/logout +- Redirect URI: [https://your-domain.com/auth/oidc/callback](https://your-domain.com/auth/oidc/callback) +- Post Logout Redirect URI: [https://your-domain.com/logout](https://your-domain.com/logout) +- Backchannel Logout URI: [https://your-domain.com/auth/oidc/logout](https://your-domain.com/auth/oidc/logout)🧰 Tools
🪛 LanguageTool
[uncategorized] ~144-~144: Possible missing comma found.
Context: ...IDC_SCOPES="profile,email" ``` In your IDP you should configure the following: - ...(AI_HYDRA_LEO_MISSING_COMMA)
🪛 markdownlint-cli2 (0.17.2)
146-146: Bare URL used
null(MD034, no-bare-urls)
147-147: Bare URL used
null(MD034, no-bare-urls)
148-148: Bare URL used
null(MD034, no-bare-urls)
281-281: Correct verb form: "are send" → "are sent".The verb form should be changed from "send" to "sent" for correct grammar.
-The attribute names are the header names in which the attribute values are send by the apache mod_shib to the application. +The attribute names are the header names in which the attribute values are sent by the apache mod_shib to the application.🧰 Tools
🪛 LanguageTool
[uncategorized] ~281-~281: This verb may not be in the correct form. Consider using a different form for this context.
Context: ...names in which the attribute values are send by the apache mod_shib to the applicati...(AI_EN_LECTOR_REPLACEMENT_VERB_FORM)
330-360: Consider adding more details about the OIDC attribute mapping.The OIDC example could benefit from additional explanation about common OIDC claims and how they map to application attributes.
##### Attributes In this example the OpenID Connect provider returns the claim `preferred_username` which contains the username and an additional claim `roles` with an array of roles. +Common standard claims provided by OIDC providers include: +- `sub`: Subject identifier, a unique identifier for the user +- `preferred_username`: The user's preferred username +- `given_name`: First name +- `family_name`: Last name +- `email`: Email address +- `email_verified`: Boolean indicating if the email has been verified + +Custom claims like `roles` can be defined by your identity provider. ##### Roles - The "superuser" role is assigned to any user who has the "pilos-superuser" role.🧰 Tools
🪛 LanguageTool
[typographical] ~332-~332: It appears that a comma is missing.
Context: ...enID Connect ##### Attributes In this example the OpenID Connect provider returns the...(DURING_THAT_TIME_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
.env.example(1 hunks)app/Auth/OIDC/OIDCController.php(1 hunks)app/Auth/OIDC/OIDCUser.php(1 hunks)app/Auth/OIDC/OpenIDConnectClient.php(1 hunks)app/Http/Controllers/api/v1/auth/LoginController.php(3 hunks)app/Http/Middleware/VerifyCsrfToken.php(1 hunks)app/Http/Resources/Config.php(1 hunks)composer.json(1 hunks)config/services.php(2 hunks)docs/docs/administration/08-advanced/01-external-authentication.md(5 hunks)lang/de/admin.php(1 hunks)lang/de/auth.php(1 hunks)lang/en/admin.php(1 hunks)lang/en/auth.php(1 hunks)lang/fr/admin.php(1 hunks)lang/fr/auth.php(1 hunks)resources/js/components/MainNav.vue(1 hunks)resources/js/views/Login.vue(3 hunks)resources/js/views/Logout.vue(1 hunks)routes/web.php(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- .env.example
- composer.json
🚧 Files skipped from review as they are similar to previous changes (16)
- app/Http/Resources/Config.php
- lang/fr/admin.php
- app/Http/Middleware/VerifyCsrfToken.php
- resources/js/views/Logout.vue
- lang/en/auth.php
- resources/js/components/MainNav.vue
- app/Auth/OIDC/OIDCUser.php
- app/Http/Controllers/api/v1/auth/LoginController.php
- lang/fr/auth.php
- lang/de/admin.php
- config/services.php
- routes/web.php
- resources/js/views/Login.vue
- app/Auth/OIDC/OpenIDConnectClient.php
- lang/de/auth.php
- lang/en/admin.php
🧰 Additional context used
🪛 LanguageTool
docs/docs/administration/08-advanced/01-external-authentication.md
[uncategorized] ~14-~14: A comma may be missing after the conjunctive/linking adverb ‘Therefore’.
Context: ...practical to manage all users in PILOS. Therefore PILOS can be connected to external auth...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[uncategorized] ~144-~144: Possible missing comma found.
Context: ...IDC_SCOPES="profile,email" ``` In your IDP you should configure the following: - ...
(AI_HYDRA_LEO_MISSING_COMMA)
[typographical] ~152-~152: It appears that a comma is missing.
Context: ...## Configure mapping For each external authenticator the attribute and role mapping needs to...
(DURING_THAT_TIME_COMMA)
[typographical] ~230-~230: It appears that a comma is missing.
Context: ...s #### LDAP ##### Attributes In this example the LDAP schema uses the common name (C...
(DURING_THAT_TIME_COMMA)
[uncategorized] ~281-~281: This verb may not be in the correct form. Consider using a different form for this context.
Context: ...names in which the attribute values are send by the apache mod_shib to the applicati...
(AI_EN_LECTOR_REPLACEMENT_VERB_FORM)
[typographical] ~332-~332: It appears that a comma is missing.
Context: ...enID Connect ##### Attributes In this example the OpenID Connect provider returns the...
(DURING_THAT_TIME_COMMA)
🪛 markdownlint-cli2 (0.17.2)
docs/docs/administration/08-advanced/01-external-authentication.md
135-135: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
146-146: Bare URL used
null
(MD034, no-bare-urls)
147-147: Bare URL used
null
(MD034, no-bare-urls)
148-148: Bare URL used
null
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Docker Build
- GitHub Check: Backend
app/Auth/OIDC/OIDCController.php
Outdated
| public function __construct() | ||
| { | ||
| $this->middleware('guest'); | ||
| $this->oidc = new OpenIDConnectClient( | ||
| config('services.oidc.issuer'), | ||
| config('services.oidc.client_id'), | ||
| config('services.oidc.client_secret') | ||
| ); | ||
| $this->oidc->setRedirectURL(url('/auth/oidc/callback')); | ||
| $this->oidc->addScope(config('services.oidc.scopes')); | ||
|
|
||
| if (config('app.env') == 'local') { | ||
| $this->oidc->setHttpUpgradeInsecureRequests(false); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add configuration validation to prevent runtime errors.
The constructor initializes the OIDC client with configuration values but doesn't validate if they exist or are valid. If any required configuration is missing, this could lead to runtime errors.
public function __construct()
{
$this->middleware('guest');
+ // Validate that required configuration values exist
+ if (!config('services.oidc.issuer') || !config('services.oidc.client_id')) {
+ Log::error('Required OIDC configuration missing');
+ abort(500, 'OIDC configuration error');
+ }
$this->oidc = new OpenIDConnectClient(
config('services.oidc.issuer'),
config('services.oidc.client_id'),
config('services.oidc.client_secret')
);
$this->oidc->setRedirectURL(url('/auth/oidc/callback'));
$this->oidc->addScope(config('services.oidc.scopes'));
if (config('app.env') == 'local') {
$this->oidc->setHttpUpgradeInsecureRequests(false);
}
}
app/Auth/OIDC/OIDCController.php
Outdated
| public function signoutRedirectURL(string $logout_url) | ||
| { | ||
| if (! $this->oidc->hasEndSessionEndpoint()) { | ||
| return false; | ||
| } | ||
|
|
||
| $params = [ | ||
| 'id_token' => session('oidc_id_token'), | ||
| 'logout_url' => $logout_url, | ||
| ]; | ||
|
|
||
| return route('auth.oidc.signout', $params); | ||
| } |
There was a problem hiding this comment.
Validate the logout URL in signoutRedirectURL method.
The method should validate that the provided logout URL is safe before including it in the redirect.
public function signoutRedirectURL(string $logout_url)
{
if (! $this->oidc->hasEndSessionEndpoint()) {
return false;
}
+ // Validate that the logout URL is safe
+ $allowed_domains = ['your-app-domain.com']; // Add your allowed domains
+ $logout_url_host = parse_url($logout_url, PHP_URL_HOST);
+ if (!in_array($logout_url_host, $allowed_domains)) {
+ Log::warning('Invalid logout URL domain in signoutRedirectURL', ['url' => $logout_url]);
+ return false;
+ }
$params = [
'id_token' => session('oidc_id_token'),
'logout_url' => $logout_url,
];
return route('auth.oidc.signout', $params);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function signoutRedirectURL(string $logout_url) | |
| { | |
| if (! $this->oidc->hasEndSessionEndpoint()) { | |
| return false; | |
| } | |
| $params = [ | |
| 'id_token' => session('oidc_id_token'), | |
| 'logout_url' => $logout_url, | |
| ]; | |
| return route('auth.oidc.signout', $params); | |
| } | |
| public function signoutRedirectURL(string $logout_url) | |
| { | |
| if (! $this->oidc->hasEndSessionEndpoint()) { | |
| return false; | |
| } | |
| // Validate that the logout URL is safe | |
| $allowed_domains = ['your-app-domain.com']; // Add your allowed domains | |
| $logout_url_host = parse_url($logout_url, PHP_URL_HOST); | |
| if (! in_array($logout_url_host, $allowed_domains)) { | |
| Log::warning('Invalid logout URL domain in signoutRedirectURL', ['url' => $logout_url]); | |
| return false; | |
| } | |
| $params = [ | |
| 'id_token' => session('oidc_id_token'), | |
| 'logout_url' => $logout_url, | |
| ]; | |
| return route('auth.oidc.signout', $params); | |
| } |
app/Auth/OIDC/OIDCController.php
Outdated
| public function signout(Request $request) | ||
| { | ||
| $this->oidc->signOut($request['id_token'], $request['logout_url']); | ||
| } |
There was a problem hiding this comment.
Validate frontchannel logout parameters.
The signout method accepts parameters from the request without validation, which could lead to security issues. Add validation for the id_token and logout_url parameters.
public function signout(Request $request)
{
+ // Validate the request parameters
+ $validated = $request->validate([
+ 'id_token' => 'required|string',
+ 'logout_url' => 'required|url'
+ ]);
+
+ // Make sure the logout URL is allowed
+ $allowed_domains = ['your-app-domain.com']; // Add your allowed domains
+ $logout_url_host = parse_url($request['logout_url'], PHP_URL_HOST);
+ if (!in_array($logout_url_host, $allowed_domains)) {
+ Log::warning('Invalid logout URL domain', ['url' => $request['logout_url']]);
+ return redirect('/external_login?error=invalid_logout_url');
+ }
$this->oidc->signOut($request['id_token'], $request['logout_url']);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function signout(Request $request) | |
| { | |
| $this->oidc->signOut($request['id_token'], $request['logout_url']); | |
| } | |
| public function signout(Request $request) | |
| { | |
| // Validate the request parameters | |
| $validated = $request->validate([ | |
| 'id_token' => 'required|string', | |
| 'logout_url' => 'required|url', | |
| ]); | |
| // Make sure the logout URL is allowed | |
| $allowed_domains = ['your-app-domain.com']; // Add your allowed domains | |
| $logout_url_host = parse_url($request['logout_url'], PHP_URL_HOST); | |
| if (! in_array($logout_url_host, $allowed_domains)) { | |
| Log::warning('Invalid logout URL domain', ['url' => $request['logout_url']]); | |
| return redirect('/external_login?error=invalid_logout_url'); | |
| } | |
| $this->oidc->signOut($request['id_token'], $request['logout_url']); | |
| } |
app/Auth/OIDC/OIDCController.php
Outdated
|
|
||
| $url = '/external_login'; | ||
|
|
||
| return redirect($request->session()->has('redirect_url') ? ($url.'?redirect='.urlencode($request->session()->get('redirect_url'))) : $url); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Sanitize the redirect URL to prevent open redirect vulnerabilities.
The code uses a redirect URL from the session without validation, which could potentially lead to an open redirect vulnerability.
🏁 Script executed:
#!/bin/bash
# Check if there's any sanitization of redirect URLs in the codebase
# Search for redirect URL validation
rg -A 5 -B 5 "redirect.*\?.*=" app/Http/
rg -A 5 -B 5 "redirect_url" app/Http/Length of output: 80
Let’s locate all uses of redirect_url (where it’s set and if it’s ever validated) and inspect the context in OIDCController.php:
🏁 Script executed:
#!/bin/bash
# 1. Find where redirect_url is stored in session
rg -n "session()->put.*redirect_url"
# 2. Show surrounding code in OIDCController for any redirect_url usage
rg -n "redirect_url" -A5 -B5 app/Auth/OIDC/OIDCController.php
# 3. Check for any URL validation functions applied to redirect_url
rg -n -e "filter_var.*redirect_url" -e "parse_url.*redirect_url" -e "Validator::make.*redirect_url"Length of output: 602
Prevent open-redirect by validating the session’s redirect_url
There is no evidence that the stored redirect_url is ever checked or sanitized before being appended to your redirect response. To eliminate an open-redirect risk:
• Location: app/Auth/OIDC/OIDCController.php, line 76
• Action:
- Identify where you push
redirect_urlinto the session and ensure it can only be set to an allowed internal path or hostname. - Before redirecting, pull and validate it—e.g. with Laravel’s validator or a simple whitelist of internal paths:
- return redirect(
- $request->session()->has('redirect_url')
- ? ($url.'?redirect='.urlencode($request->session()->get('redirect_url')))
- : $url
- );
+ $redirect = $request->session()->pull('redirect_url');
+ // Example validation: allow only paths under your own domain
+ if ($redirect
+ && filter_var($redirect, FILTER_VALIDATE_URL)
+ && Str::of(parse_url($redirect, PHP_URL_PATH))->startsWith('/')
+ && parse_url($redirect, PHP_URL_HOST) === request()->getHost()
+ ) {
+ $url .= '?redirect='.urlencode($redirect);
+ }
+ return redirect($url);• Alternatively, use Laravel’s Validator:
$redirect = $request->session()->pull('redirect_url');
Validator::make(
['redirect' => $redirect],
['redirect' => ['nullable','url', new App\Rules\InternalUrl]]
)->validate();
$url .= $redirect ? '?redirect='.urlencode($redirect) : '';
return redirect($url);Ensure that every user-supplied redirect target is strictly constrained to prevent malicious open-redirects.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (6)
app/Auth/OIDC/OIDCController.php (6)
147-148: Use Laravel's route helper correctly.The code manually constructs a route URL with parameters, but Laravel provides a more robust way to generate URLs for named routes.
- return route('auth.oidc.signout', $params); + return route('auth.oidc.signout', $params);This issue has already been identified in a previous review comment, so the code appears to have been corrected to use the proper Laravel route generation.
14-28:⚠️ Potential issueAdd configuration validation to prevent runtime errors.
The constructor initializes the OIDC client with configuration values but doesn't validate if they exist or are valid. If any required configuration is missing, this could lead to runtime errors.
Apply this diff to validate configuration:
public function __construct() { $this->middleware('guest'); + // Validate that required configuration values exist + if (!config('services.oidc.issuer') || !config('services.oidc.client_id')) { + Log::error('Required OIDC configuration missing'); + abort(500, 'OIDC configuration error'); + } $this->oidc = new OpenIDConnectClient( config('services.oidc.issuer'), config('services.oidc.client_id'), config('services.oidc.client_secret') ); $this->oidc->setRedirectURL(url('/auth/oidc/callback')); $this->oidc->addScope(config('services.oidc.scopes')); if (config('app.env') == 'local') { $this->oidc->setHttpUpgradeInsecureRequests(false); } }
30-33:⚠️ Potential issueAdd error handling for the authentication redirect.
The redirect method initiates the OIDC flow but doesn't handle potential exceptions. This could result in unhandled errors if the OIDC provider is unreachable or misconfigured.
Apply this diff:
public function redirect(Request $request) { + try { $this->oidc->authenticate(); + } catch (\Exception $e) { + Log::error('OIDC authentication redirect failed: ' . $e->getMessage()); + return redirect('/external_login?error=openid_connect_exception'); + } }
97-97:⚠️ Potential issuePrevent open-redirect by validating the session's
redirect_url.There is no validation for the redirect URL stored in the session, which could lead to an open redirect vulnerability.
- return redirect($request->session()->has('redirect_url') ? ($url.'?redirect='.urlencode($request->session()->get('redirect_url'))) : $url); + $redirect = $request->session()->pull('redirect_url'); + // Example validation: allow only paths under your own domain + if ($redirect + && filter_var($redirect, FILTER_VALIDATE_URL) + && Str::of(parse_url($redirect, PHP_URL_PATH))->startsWith('/') + && parse_url($redirect, PHP_URL_HOST) === request()->getHost() + ) { + $url .= '?redirect='.urlencode($redirect); + } + return redirect($url);
131-134:⚠️ Potential issueValidate frontchannel logout parameters.
The signout method accepts parameters from the request without validation, which could lead to security issues.
public function signout(Request $request) { + // Validate the request parameters + $validated = $request->validate([ + 'id_token' => 'required|string', + 'logout_url' => 'required|url' + ]); + + // Make sure the logout URL is allowed + $allowed_domains = [parse_url(config('app.url'), PHP_URL_HOST)]; // Use app URL as allowed domain + $logout_url_host = parse_url($request['logout_url'], PHP_URL_HOST); + if (!in_array($logout_url_host, $allowed_domains)) { + Log::warning('Invalid logout URL domain', ['url' => $request['logout_url']]); + return redirect('/external_login?error=invalid_logout_url'); + } $this->oidc->signOut($request['id_token'], $request['logout_url']); }
136-148:⚠️ Potential issueValidate the logout URL in signoutRedirectURL method.
The method should validate that the provided logout URL is safe before including it in the redirect.
public function signoutRedirectURL(string $logout_url) { if (! $this->oidc->hasEndSessionEndpoint()) { return false; } + // Validate that the logout URL is safe + $allowed_domains = [parse_url(config('app.url'), PHP_URL_HOST)]; // Use app URL as allowed domain + $logout_url_host = parse_url($logout_url, PHP_URL_HOST); + if (!in_array($logout_url_host, $allowed_domains)) { + Log::warning('Invalid logout URL domain in signoutRedirectURL', ['url' => $logout_url]); + return false; + } $params = [ 'id_token' => session('oidc_id_token'), 'logout_url' => $logout_url, ]; - return route('auth.oidc.signout', $params); + return route('auth.oidc.signout', $params); }
🧹 Nitpick comments (2)
app/Auth/OIDC/OIDCController.php (2)
42-47: Consider using a custom exception for IAT validation failure.The temporary check for the "issued at" (iat) claim uses a generic Exception. Using a specific exception class would improve error handling and make debugging easier.
- if (! isset($claims->iat) || ! is_int($claims->iat) || ($claims->iat >= time() + $this->oidc->getLeeway())) { - // Invalid Issued At - throw new \Exception('Invalid Issued At'); - } + if (! isset($claims->iat) || ! is_int($claims->iat) || ($claims->iat >= time() + $this->oidc->getLeeway())) { + // Invalid Issued At + throw new \OpenIDConnectClientException('Invalid Issued At: IAT claim validation failed'); + }
43-44: Add timeframe for removing temporary code.The TODO comments mention PRs that need to be merged, but there's no timeframe or tracking issue linked to ensure this temporary code is eventually removed.
- // @todo: Remove when https://github.com/jumbojett/OpenID-Connect-PHP/pull/476 is merged + // @todo: Remove when https://github.com/jumbojett/OpenID-Connect-PHP/pull/476 is merged + // Issue #XXX: Track the removal of this temporary code (target: Q3 2023)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/Auth/OIDC/OIDCController.php(1 hunks)resources/js/views/ExternalLogin.vue(1 hunks)
🔇 Additional comments (1)
resources/js/views/ExternalLogin.vue (1)
24-29: Added OIDC error handling correctly.The new error message component for OpenID Connect exceptions follows the same pattern as the existing error messages, providing consistent user feedback for OIDC authentication failures.
app/Auth/OIDC/OIDCController.php
Outdated
| Log::error($e); | ||
|
|
||
| return redirect('/external_login?error=openid_connect_exception'); | ||
| } | ||
|
|
||
| // Create new open-id connect user | ||
| try { | ||
| $user_info = get_object_vars($this->oidc->requestUserInfo()); | ||
| $oidc_user = new OIDCUser($user_info); | ||
|
|
||
| // Check subject | ||
| // @todo: Remove when https://github.com/jumbojett/OpenID-Connect-PHP/pull/478 is merged | ||
| if (! isset($user_info['sub']) || $user_info['sub'] !== $this->oidc->getIdTokenPayload()->sub) { | ||
| // Invalid subject in user info response | ||
| throw new \Exception('Invalid Subject in user info response'); | ||
| } | ||
|
|
||
| } catch (MissingAttributeException $e) { | ||
| return redirect('/external_login?error=missing_attributes'); | ||
| } catch (\Exception $e) { | ||
| Log::error($e); | ||
|
|
||
| return redirect('/external_login?error=openid_connect_exception'); | ||
| } | ||
|
|
||
| // Get eloquent user (existing or new) | ||
| $user = $oidc_user->createOrFindEloquentModel('oidc'); | ||
|
|
||
| // Sync attributes | ||
| try { | ||
| $oidc_user->syncWithEloquentModel($user, config('services.oidc.mapping')->roles); | ||
| } catch (MissingAttributeException $e) { | ||
| return redirect('/external_login?error=missing_attributes'); | ||
| } | ||
|
|
||
| Auth::login($user); | ||
|
|
||
| session(['session_data' => [ | ||
| ['key' => 'oidc_sub', 'value' => $user_info['sub']], | ||
| ]]); | ||
|
|
||
| session()->put('oidc_id_token', $this->oidc->getIdToken()); | ||
|
|
||
| \Log::info('External user {user} has been successfully authenticated.', ['user' => $user->getLogLabel(), 'type' => 'oidc']); | ||
|
|
||
| $url = '/external_login'; | ||
|
|
||
| return redirect($request->session()->has('redirect_url') ? ($url.'?redirect='.urlencode($request->session()->get('redirect_url'))) : $url); | ||
| } | ||
|
|
||
| /** | ||
| * Backchannel logout | ||
| */ | ||
| public function logout(Request $request) | ||
| { | ||
| Log::debug('OIDC backchannel logout handler called'); | ||
|
|
||
| if (! $this->oidc->verifyLogoutToken()) { | ||
| Log::warning('Logout token verification failed'); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| $sub = $this->oidc->getSubjectFromBackChannel(); | ||
| if (! isset($sub)) { | ||
| Log::warning('Getting subject from backchannel failed'); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| $lookupSessions = SessionData::where('key', 'oidc_sub')->where('value', $sub)->get(); | ||
| foreach ($lookupSessions as $lookupSession) { | ||
| $user = $lookupSession->session->user->getLogLabel(); | ||
| Log::info('Deleting session of user {user}', ['user' => $user, 'type' => 'oidc']); | ||
| $lookupSession->session()->delete(); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Frontchannel logout | ||
| */ | ||
| public function signout(Request $request) | ||
| { | ||
| $this->oidc->signOut($request['id_token'], $request['logout_url']); | ||
| } | ||
|
|
||
| public function signoutRedirectURL(string $logout_url) | ||
| { | ||
| if (! $this->oidc->hasEndSessionEndpoint()) { | ||
| return false; | ||
| } | ||
|
|
||
| $params = [ | ||
| 'id_token' => session('oidc_id_token'), | ||
| 'logout_url' => $logout_url, | ||
| ]; | ||
|
|
||
| return route('auth.oidc.signout', $params); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement CSRF protection for OIDC endpoints.
The controller doesn't include CSRF protection which is essential for preventing cross-site request forgery attacks, especially for authentication flows.
The OIDC auth flow should validate a state parameter to prevent CSRF attacks. Consider implementing this in the redirect method:
public function redirect(Request $request)
{
+ // Generate and store a random state parameter
+ $state = bin2hex(random_bytes(16));
+ session()->put('oidc_state', $state);
+ $this->oidc->setState($state);
try {
$this->oidc->authenticate();
} catch (\Exception $e) {
Log::error('OIDC authentication redirect failed: ' . $e->getMessage());
return redirect('/external_login?error=openid_connect_exception');
}
}
public function callback(Request $request)
{
try {
+ // Verify the state parameter to prevent CSRF
+ $storedState = session()->pull('oidc_state');
+ $returnedState = $request->input('state');
+
+ if (empty($storedState) || $storedState !== $returnedState) {
+ Log::warning('OIDC state validation failed', [
+ 'stored' => $storedState,
+ 'returned' => $returnedState
+ ]);
+ throw new \Exception('Invalid state parameter');
+ }
+
$this->oidc->authenticate();
// ...
}
}Committable suggestion skipped: line range outside the PR's diff.
app/Auth/OIDC/OIDCController.php
Outdated
| session(['session_data' => [ | ||
| ['key' => 'oidc_sub', 'value' => $user_info['sub']], | ||
| ]]); | ||
|
|
||
| session()->put('oidc_id_token', $this->oidc->getIdToken()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement session security best practices.
The code stores sensitive OIDC information in the session but doesn't set appropriate session flags or expiration for these tokens.
- session(['session_data' => [
- ['key' => 'oidc_sub', 'value' => $user_info['sub']],
- ]]);
-
- session()->put('oidc_id_token', $this->oidc->getIdToken());
+ // Store OIDC subject in session data table for backchannel logout
+ session(['session_data' => [
+ ['key' => 'oidc_sub', 'value' => $user_info['sub']],
+ ]]);
+
+ // Store ID token with appropriate expiration based on token's exp claim
+ $idToken = $this->oidc->getIdToken();
+ $tokenPayload = $this->oidc->getIdTokenPayload();
+ $tokenExpiration = isset($tokenPayload->exp) ? $tokenPayload->exp - time() : 3600;
+
+ // Ensure token expiration doesn't exceed session lifetime
+ $sessionLifetime = config('session.lifetime') * 60;
+ $expiration = min($tokenExpiration, $sessionLifetime);
+
+ session()->put('oidc_id_token', $idToken);
+ // Optionally mark this session value as sensitive to prevent exposure in debug modes
+ session()->increment('_token_sensitivity');Committable suggestion skipped: line range outside the PR's diff.
app/Auth/OIDC/OIDCController.php
Outdated
| $sub = $this->oidc->getSubjectFromBackChannel(); | ||
| if (! isset($sub)) { | ||
| Log::warning('Getting subject from backchannel failed'); | ||
|
|
||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify and handle backchannel logout token validation more robustly.
The code doesn't handle malformed subjects or provide detailed logging for debugging purposes.
- $sub = $this->oidc->getSubjectFromBackChannel();
- if (! isset($sub)) {
- Log::warning('Getting subject from backchannel failed');
-
- return;
- }
+ try {
+ $sub = $this->oidc->getSubjectFromBackChannel();
+ if (! isset($sub) || empty($sub)) {
+ Log::warning('Invalid or empty subject in backchannel logout token');
+ return response('Bad Request: Invalid subject', 400);
+ }
+
+ Log::debug('Processing backchannel logout for subject', ['sub' => $sub]);
+ } catch (\Exception $e) {
+ Log::error('Exception in backchannel logout subject extraction', [
+ 'error' => $e->getMessage(),
+ 'trace' => $e->getTraceAsString()
+ ]);
+ return response('Internal Server Error', 500);
+ }
app/Auth/OIDC/OIDCController.php
Outdated
| try { | ||
| $user_info = get_object_vars($this->oidc->requestUserInfo()); | ||
| $oidc_user = new OIDCUser($user_info); | ||
|
|
||
| // Check subject | ||
| // @todo: Remove when https://github.com/jumbojett/OpenID-Connect-PHP/pull/478 is merged | ||
| if (! isset($user_info['sub']) || $user_info['sub'] !== $this->oidc->getIdTokenPayload()->sub) { | ||
| // Invalid subject in user info response | ||
| throw new \Exception('Invalid Subject in user info response'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling for user info request.
The code doesn't specifically handle network errors or malformed responses when requesting user info, which could lead to confusing error messages.
try {
- $user_info = get_object_vars($this->oidc->requestUserInfo());
- $oidc_user = new OIDCUser($user_info);
-
- // Check subject
- // @todo: Remove when https://github.com/jumbojett/OpenID-Connect-PHP/pull/478 is merged
- if (! isset($user_info['sub']) || $user_info['sub'] !== $this->oidc->getIdTokenPayload()->sub) {
- // Invalid subject in user info response
- throw new \Exception('Invalid Subject in user info response');
- }
+ try {
+ $user_info_response = $this->oidc->requestUserInfo();
+ if (!$user_info_response) {
+ throw new \Exception('Empty user info response');
+ }
+ $user_info = get_object_vars($user_info_response);
+ $oidc_user = new OIDCUser($user_info);
+
+ // Check subject
+ // @todo: Remove when https://github.com/jumbojett/OpenID-Connect-PHP/pull/478 is merged
+ if (! isset($user_info['sub']) || $user_info['sub'] !== $this->oidc->getIdTokenPayload()->sub) {
+ // Invalid subject in user info response
+ throw new \Exception('Invalid Subject in user info response');
+ }
+ } catch (\GuzzleHttp\Exception\RequestException $e) {
+ Log::error('OIDC userinfo request failed', ['error' => $e->getMessage()]);
+ throw new \Exception('Failed to retrieve user information from OIDC provider');
+ }
pizkaz
force-pushed
the
300-implement-oidc
branch
from
203ca14 to
8bc5394
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (8)
app/Auth/OIDC/OIDCController.php (8)
14-28: 🛠️ Refactor suggestionAdd configuration validation to prevent runtime errors.
The constructor initializes the OIDC client with configuration values without validating if they exist or are valid. If any required configuration is missing, this could lead to runtime errors.
public function __construct() { $this->middleware('guest'); + // Validate that required configuration values exist + if (!config('services.oidc.issuer') || !config('services.oidc.client_id')) { + Log::error('Required OIDC configuration missing'); + abort(500, 'OIDC configuration error'); + } $this->oidc = new OpenIDConnectClient( config('services.oidc.issuer'), config('services.oidc.client_id'), config('services.oidc.client_secret') ); $this->oidc->setRedirectURL(url('/auth/oidc/callback')); $this->oidc->addScope(config('services.oidc.scopes')); if (config('app.env') == 'local') { $this->oidc->setHttpUpgradeInsecureRequests(false); } }
30-33: 🛠️ Refactor suggestionAdd error handling and CSRF protection to the authentication redirect.
The redirect method initiates the OIDC flow but lacks error handling and CSRF protection using the state parameter.
public function redirect(Request $request) { + // Generate and store a random state parameter + $state = bin2hex(random_bytes(16)); + session()->put('oidc_state', $state); + $this->oidc->setState($state); + + try { $this->oidc->authenticate(); + } catch (\Exception $e) { + Log::error('OIDC authentication redirect failed: ' . $e->getMessage()); + return redirect('/external_login?error=authentication_failed'); + } }
56-65: 🛠️ Refactor suggestionImprove error handling for user info request.
The code doesn't specifically handle network errors or malformed responses when requesting user info, which could lead to confusing error messages.
try { - $user_info = get_object_vars($this->oidc->requestUserInfo()); - $oidc_user = new OIDCUser($user_info); - - // Check subject - // @todo: Remove when https://github.com/jumbojett/OpenID-Connect-PHP/pull/478 is merged - if (! isset($user_info['sub']) || $user_info['sub'] !== $this->oidc->getIdTokenPayload()->sub) { - // Invalid subject in user info response - throw new \Exception('Invalid Subject in user info response'); - } + try { + $user_info_response = $this->oidc->requestUserInfo(); + if (!$user_info_response) { + throw new \Exception('Empty user info response'); + } + $user_info = get_object_vars($user_info_response); + $oidc_user = new OIDCUser($user_info); + + // Check subject + // @todo: Remove when https://github.com/jumbojett/OpenID-Connect-PHP/pull/478 is merged + if (! isset($user_info['sub']) || $user_info['sub'] !== $this->oidc->getIdTokenPayload()->sub) { + // Invalid subject in user info response + throw new \Exception('Invalid Subject in user info response'); + } + } catch (\GuzzleHttp\Exception\RequestException $e) { + Log::error('OIDC userinfo request failed', ['error' => $e->getMessage()]); + throw new \Exception('Failed to retrieve user information from OIDC provider'); + }
87-91: 🛠️ Refactor suggestionImplement session security best practices for token storage.
The code stores sensitive OIDC information in the session but doesn't set appropriate session flags or expiration for these tokens.
- session(['session_data' => [ - ['key' => 'oidc_sub', 'value' => $user_info['sub']], - ]]); - - session()->put('oidc_id_token', $this->oidc->getIdToken()); + // Store OIDC subject in session data table for backchannel logout + session(['session_data' => [ + ['key' => 'oidc_sub', 'value' => $user_info['sub']], + ]]); + + // Store ID token with appropriate expiration based on token's exp claim + $idToken = $this->oidc->getIdToken(); + $tokenPayload = $this->oidc->getIdTokenPayload(); + $tokenExpiration = isset($tokenPayload->exp) ? $tokenPayload->exp - time() : 3600; + + // Ensure token expiration doesn't exceed session lifetime + $sessionLifetime = config('session.lifetime') * 60; + $expiration = min($tokenExpiration, $sessionLifetime); + + session()->put('oidc_id_token', $idToken); + // Optionally mark this session value as sensitive to prevent exposure in debug modes + session()->increment('_token_sensitivity');
97-97:⚠️ Potential issuePrevent open-redirect by validating the session's
redirect_url.There is no validation of the redirect URL retrieved from the session, which could lead to an open redirect vulnerability.
- return redirect($request->session()->has('redirect_url') ? ($url.'?redirect='.urlencode($request->session()->get('redirect_url'))) : $url); + $redirect = $request->session()->pull('redirect_url'); + // Validate that the redirect URL is safe + if ($redirect + && filter_var($redirect, FILTER_VALIDATE_URL) + && Str::of(parse_url($redirect, PHP_URL_PATH))->startsWith('/') + && parse_url($redirect, PHP_URL_HOST) === request()->getHost() + ) { + $url .= '?redirect='.urlencode($redirect); + } + return redirect($url);
113-118: 🛠️ Refactor suggestionVerify and handle backchannel logout token validation more robustly.
The code doesn't handle malformed subjects or provide detailed logging for debugging purposes.
- $sub = $this->oidc->getSubjectFromBackChannel(); - if (! isset($sub)) { - Log::warning('Getting subject from backchannel failed'); - - return; - } + try { + $sub = $this->oidc->getSubjectFromBackChannel(); + if (! isset($sub) || empty($sub)) { + Log::warning('Invalid or empty subject in backchannel logout token'); + return response('Bad Request: Invalid subject', 400); + } + + Log::debug('Processing backchannel logout for subject', ['sub' => $sub]); + } catch (\Exception $e) { + Log::error('Exception in backchannel logout subject extraction', [ + 'error' => $e->getMessage(), + 'trace' => $e->getTraceAsString() + ]); + return response('Internal Server Error', 500); + }
131-134:⚠️ Potential issueValidate frontchannel logout parameters to prevent security issues.
The signout method accepts parameters from the request without validation, which could lead to security issues.
public function signout(Request $request) { + // Validate the request parameters + $validated = $request->validate([ + 'id_token' => 'required|string', + 'logout_url' => 'required|url' + ]); + + // Make sure the logout URL is allowed + $allowed_domains = [config('app.url')]; // Add your allowed domains + $logout_url_host = parse_url($request['logout_url'], PHP_URL_HOST); + if (!in_array($logout_url_host, $allowed_domains)) { + Log::warning('Invalid logout URL domain', ['url' => $request['logout_url']]); + return redirect('/external_login?error=invalid_logout_url'); + } $this->oidc->signOut($request['id_token'], $request['logout_url']); }
136-148:⚠️ Potential issueValidate the logout URL in signoutRedirectURL method.
The method should validate that the provided logout URL is safe before including it in the redirect.
public function signoutRedirectURL(string $logout_url) { if (! $this->oidc->hasEndSessionEndpoint()) { return false; } + // Validate that the logout URL is safe + $allowed_domains = [parse_url(config('app.url'), PHP_URL_HOST)]; // Add your allowed domains + $logout_url_host = parse_url($logout_url, PHP_URL_HOST); + if (!in_array($logout_url_host, $allowed_domains)) { + Log::warning('Invalid logout URL domain in signoutRedirectURL', ['url' => $logout_url]); + return false; + } $params = [ 'id_token' => session('oidc_id_token'), 'logout_url' => $logout_url, ]; - return route('auth.oidc.signout', $params); + return route('auth.oidc.signout', $params); }
🧹 Nitpick comments (3)
docs/docs/administration/08-advanced/01-external-authentication.md (3)
135-142: Add language specifier to the code block.Add a language specifier to the code block for better syntax highlighting and to comply with Markdown standards.
-``` +```bash # OpenID Connect config OIDC_ENABLED=true OIDC_ISSUER=http://idp.university.org OIDC_CLIENT_ID=my_client_id OIDC_CLIENT_SECRET=my_client_secret OIDC_SCOPES="profile,email"🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
135-135: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
144-148: Format URLs to avoid bare URL markdown warning.Use Markdown link format for the URLs to comply with Markdown best practices.
-In your IDP you should configure the following: +In your IDP, you should configure the following: -- Redirect URI: https://your-domain.com/auth/oidc/callback -- Post Logout Redirect URI: https://your-domain.com/logout -- Backchannel Logout URI: https://your-domain.com/auth/oidc/logout +- Redirect URI: [https://your-domain.com/auth/oidc/callback](https://your-domain.com/auth/oidc/callback) +- Post Logout Redirect URI: [https://your-domain.com/logout](https://your-domain.com/logout) +- Backchannel Logout URI: [https://your-domain.com/auth/oidc/logout](https://your-domain.com/auth/oidc/logout)🧰 Tools
🪛 LanguageTool
[uncategorized] ~144-~144: Possible missing comma found.
Context: ...IDC_SCOPES="profile,email" ``` In your IDP you should configure the following: - ...(AI_HYDRA_LEO_MISSING_COMMA)
🪛 markdownlint-cli2 (0.17.2)
146-146: Bare URL used
null(MD034, no-bare-urls)
147-147: Bare URL used
null(MD034, no-bare-urls)
148-148: Bare URL used
null(MD034, no-bare-urls)
281-281: Fix grammatical error in documentation.Correct "are send" to "are sent" for proper grammar.
-The attribute names are the header names in which the attribute values are send by the apache mod_shib to the application. +The attribute names are the header names in which the attribute values are sent by the apache mod_shib to the application.🧰 Tools
🪛 LanguageTool
[uncategorized] ~281-~281: This verb may not be in the correct form. Consider using a different form for this context.
Context: ...names in which the attribute values are send by the apache mod_shib to the applicati...(AI_EN_LECTOR_REPLACEMENT_VERB_FORM)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
.env.example(1 hunks)app/Auth/OIDC/OIDCController.php(1 hunks)app/Auth/OIDC/OIDCUser.php(1 hunks)app/Auth/OIDC/OpenIDConnectClient.php(1 hunks)app/Http/Controllers/api/v1/auth/LoginController.php(3 hunks)app/Http/Middleware/VerifyCsrfToken.php(1 hunks)app/Http/Resources/Config.php(1 hunks)composer.json(1 hunks)config/services.php(2 hunks)docs/docs/administration/08-advanced/01-external-authentication.md(5 hunks)lang/de/admin.php(1 hunks)lang/de/auth.php(1 hunks)lang/en/admin.php(1 hunks)lang/en/auth.php(1 hunks)lang/fr/admin.php(1 hunks)lang/fr/auth.php(1 hunks)resources/js/components/MainNav.vue(1 hunks)resources/js/views/ExternalLogin.vue(1 hunks)resources/js/views/Login.vue(3 hunks)resources/js/views/Logout.vue(1 hunks)routes/web.php(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- composer.json
- .env.example
🚧 Files skipped from review as they are similar to previous changes (17)
- resources/js/views/ExternalLogin.vue
- app/Http/Resources/Config.php
- app/Http/Middleware/VerifyCsrfToken.php
- lang/de/auth.php
- lang/en/auth.php
- lang/en/admin.php
- lang/fr/admin.php
- lang/fr/auth.php
- lang/de/admin.php
- app/Auth/OIDC/OIDCUser.php
- routes/web.php
- resources/js/views/Logout.vue
- config/services.php
- resources/js/views/Login.vue
- app/Http/Controllers/api/v1/auth/LoginController.php
- app/Auth/OIDC/OpenIDConnectClient.php
- resources/js/components/MainNav.vue
🧰 Additional context used
🪛 LanguageTool
docs/docs/administration/08-advanced/01-external-authentication.md
[uncategorized] ~14-~14: A comma may be missing after the conjunctive/linking adverb ‘Therefore’.
Context: ...practical to manage all users in PILOS. Therefore PILOS can be connected to external auth...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[uncategorized] ~133-~133: You might be missing the article “the” here.
Context: ...set. If you need more scopes to get all required attributes, add them as a comma separat...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~144-~144: Possible missing comma found.
Context: ...IDC_SCOPES="profile,email" ``` In your IDP you should configure the following: - ...
(AI_HYDRA_LEO_MISSING_COMMA)
[typographical] ~152-~152: It appears that a comma is missing.
Context: ...## Configure mapping For each external authenticator the attribute and role mapping needs to...
(DURING_THAT_TIME_COMMA)
[typographical] ~230-~230: It appears that a comma is missing.
Context: ...s #### LDAP ##### Attributes In this example the LDAP schema uses the common name (C...
(DURING_THAT_TIME_COMMA)
[uncategorized] ~230-~230: You might be missing the article “a” here.
Context: ...DAP schema uses the common name (CN) as username and has the group memberships in the me...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~281-~281: This verb may not be in the correct form. Consider using a different form for this context.
Context: ...names in which the attribute values are send by the apache mod_shib to the applicati...
(AI_EN_LECTOR_REPLACEMENT_VERB_FORM)
[typographical] ~332-~332: It appears that a comma is missing.
Context: ...enID Connect ##### Attributes In this example the OpenID Connect provider returns the...
(DURING_THAT_TIME_COMMA)
🪛 markdownlint-cli2 (0.17.2)
docs/docs/administration/08-advanced/01-external-authentication.md
135-135: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
146-146: Bare URL used
null
(MD034, no-bare-urls)
147-147: Bare URL used
null
(MD034, no-bare-urls)
148-148: Bare URL used
null
(MD034, no-bare-urls)
SamuelWei
force-pushed
the
300-implement-oidc
branch
from
8bc5394 to
a5a7617
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (8)
app/Auth/OIDC/OIDCServiceProvider.php (1)
10-12: Fix the incorrect comment.The comment mentions "Shibboleth provider" but this method registers the OIDC provider.
Apply this diff to correct the comment:
/** - * Register the Shibboleth provider. + * Register the OIDC provider. */app/Auth/OIDC/EventsChecker.php (2)
10-10: Remove unused constructor parameter.The
$eventparameter is never used in the class. Consider removing it or utilizing it if it's intended for future extensibility.Apply this diff to remove the unused parameter:
- public function __construct(protected string $event) {} + public function __construct() {}
14-14: Remove unnecessary empty line.The empty line at line 14 is unnecessary and should be removed for cleaner code.
Apply this diff:
public function checkClaim($value): void { - $events = (array) $value;docs/docs/administration/08-advanced/01-external-authentication.md (3)
14-14: Consider minor grammar improvements.The static analysis tools flagged several minor grammar issues that could improve readability:
- Line 14: Consider adding a comma after "Therefore"
- Line 152: Consider adding a comma after "For each external authenticator"
Apply these diffs for better readability:
-In large environments it is impractical to manage all users in PILOS. Therefore PILOS can be connected to external authentication systems. +In large environments, it is impractical to manage all users in PILOS. Therefore, PILOS can be connected to external authentication systems.-For each external authenticator the attribute and role mapping needs to be configured. +For each external authenticator, the attribute and role mapping needs to be configured.
135-142: Add language specification to code block.The code block should have a language specified for proper syntax highlighting.
Apply this diff:
-``` +```bash # OpenID Connect config
146-148: Use proper markdown formatting for URLs.The bare URLs should be properly formatted in markdown.
Apply this diff:
-- Redirect URI: https://your-domain.com/auth/oidc/callback -- Post Logout Redirect URI: https://your-domain.com/logout -- Backchannel Logout URI: https://your-domain.com/auth/oidc/logout +- Redirect URI: `https://your-domain.com/auth/oidc/callback` +- Post Logout Redirect URI: `https://your-domain.com/logout` +- Backchannel Logout URI: `https://your-domain.com/auth/oidc/logout`app/Auth/OIDC/OpenIDConnectClient.php (2)
74-1023: Consider breaking down this large class into smaller, focused components.This 1000+ line class violates the Single Responsibility Principle. Consider extracting:
- JWT/JWS handling into a
JWTProcessorclass- HTTP client operations into a
OIDCHttpClientclass- Well-known discovery into a
DiscoveryServiceclass- Token management into a
TokenManagerclassThis would improve maintainability, testability, and readability.
512-512: Consider using cryptographically secure random generation.While
Str::random()is generally secure in Laravel, for critical security tokens like nonce and state, consider explicitly using a cryptographically secure method.- $nonce = $this->setNonce(Str::random()); + $nonce = $this->setNonce(Str::random(32));- $state = $this->setState(Str::random()); + $state = $this->setState(Str::random(32));Also applies to: 514-514
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (31)
.env.example(1 hunks)app/Auth/OIDC/AccessTokenHashChecker.php(1 hunks)app/Auth/OIDC/EventsChecker.php(1 hunks)app/Auth/OIDC/IssuerChecker.php(1 hunks)app/Auth/OIDC/OIDCController.php(1 hunks)app/Auth/OIDC/OIDCProvider.php(1 hunks)app/Auth/OIDC/OIDCServiceProvider.php(1 hunks)app/Auth/OIDC/OIDCUser.php(1 hunks)app/Auth/OIDC/OpenIDConnectClient.php(1 hunks)app/Auth/OIDC/OpenIDConnectClientException.php(1 hunks)app/Http/Controllers/api/v1/auth/LoginController.php(3 hunks)app/Http/Middleware/VerifyCsrfToken.php(1 hunks)app/Http/Resources/Config.php(1 hunks)composer.json(3 hunks)config/app.php(1 hunks)config/services.php(2 hunks)docker-compose-dev.yml(1 hunks)docs/docs/administration/08-advanced/01-external-authentication.md(5 hunks)lang/de/admin.php(1 hunks)lang/de/auth.php(1 hunks)lang/en/admin.php(1 hunks)lang/en/auth.php(1 hunks)lang/fr/admin.php(1 hunks)lang/fr/auth.php(1 hunks)metrics/prometheus.yml(1 hunks)public/vendor/telescope/mix-manifest.json(1 hunks)resources/js/components/MainNav.vue(1 hunks)resources/js/views/ExternalLogin.vue(1 hunks)resources/js/views/Login.vue(3 hunks)resources/js/views/Logout.vue(1 hunks)routes/web.php(2 hunks)
✅ Files skipped from review due to trivial changes (4)
- metrics/prometheus.yml
- app/Auth/OIDC/OpenIDConnectClientException.php
- public/vendor/telescope/mix-manifest.json
- .env.example
🚧 Files skipped from review as they are similar to previous changes (18)
- app/Http/Resources/Config.php
- app/Http/Middleware/VerifyCsrfToken.php
- lang/de/admin.php
- lang/fr/admin.php
- resources/js/views/ExternalLogin.vue
- lang/en/admin.php
- resources/js/views/Logout.vue
- resources/js/components/MainNav.vue
- lang/de/auth.php
- lang/en/auth.php
- composer.json
- resources/js/views/Login.vue
- app/Http/Controllers/api/v1/auth/LoginController.php
- app/Auth/OIDC/OIDCUser.php
- routes/web.php
- lang/fr/auth.php
- config/services.php
- app/Auth/OIDC/OIDCController.php
🧰 Additional context used
🧬 Code Graph Analysis (3)
config/app.php (1)
app/Auth/OIDC/OIDCServiceProvider.php (1)
OIDCServiceProvider(8-28)
app/Auth/OIDC/EventsChecker.php (2)
app/Auth/OIDC/IssuerChecker.php (2)
checkClaim(12-19)supportedClaim(21-24)app/Auth/OIDC/AccessTokenHashChecker.php (2)
checkClaim(12-27)supportedClaim(29-32)
app/Auth/OIDC/IssuerChecker.php (1)
app/Auth/OIDC/OpenIDConnectClient.php (3)
OpenIDConnectClient(74-1023)getIssuer(870-878)getWellKnownIssuer(862-865)
🪛 PHPMD (2.15.0)
app/Auth/OIDC/OIDCServiceProvider.php
15-15: Avoid unused parameters such as '$app'. (Unused Code Rules)
(UnusedFormalParameter)
🪛 Gitleaks (8.26.0)
docker-compose-dev.yml
126-126: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 LanguageTool
docs/docs/administration/08-advanced/01-external-authentication.md
[uncategorized] ~14-~14: Possible missing comma found.
Context: ...activated. External users In large environments it is impractical to manage all users i...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~14-~14: A comma may be missing after the conjunctive/linking adverb ‘Therefore’.
Context: ...practical to manage all users in PILOS. Therefore PILOS can be connected to external auth...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[uncategorized] ~144-~144: Possible missing comma found.
Context: ...IDC_SCOPES="profile,email" ``` In your IDP you should configure the following: - ...
(AI_HYDRA_LEO_MISSING_COMMA)
[typographical] ~152-~152: It appears that a comma is missing.
Context: ...## Configure mapping For each external authenticator the attribute and role mapping needs to...
(DURING_THAT_TIME_COMMA)
[typographical] ~248-~248: It appears that a comma is missing.
Context: ...s #### LDAP ##### Attributes In this example the LDAP schema uses the common name (C...
(DURING_THAT_TIME_COMMA)
[grammar] ~299-~299: Consider using either the past participle “sent” or the present participle “sending” here.
Context: ...names in which the attribute values are send by the apache mod_shib to the applicati...
(BEEN_PART_AGREEMENT)
[typographical] ~350-~350: It appears that a comma is missing.
Context: ...enID Connect ##### Attributes In this example the OpenID Connect provider returns the...
(DURING_THAT_TIME_COMMA)
🪛 markdownlint-cli2 (0.17.2)
docs/docs/administration/08-advanced/01-external-authentication.md
135-135: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
146-146: Bare URL used
(MD034, no-bare-urls)
147-147: Bare URL used
(MD034, no-bare-urls)
148-148: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Docker Build
- GitHub Check: Backend
🔇 Additional comments (10)
config/app.php (1)
243-243: Clean integration of OIDC service provider.The addition of
OIDCServiceProviderto the providers array follows the established pattern and is properly positioned alongside other authentication providers.app/Auth/OIDC/OIDCServiceProvider.php (1)
15-26: Well-structured service provider implementation.The singleton registration properly configures the OpenIDConnectClient with all necessary parameters and scopes before wrapping it in the OIDCProvider. The implementation follows Laravel service provider patterns correctly.
app/Auth/OIDC/EventsChecker.php (1)
15-19: Correct implementation of backchannel logout event validation.The claim checker properly validates the presence of the backchannel logout event and ensures it's an object as required by the OpenID Connect specification.
app/Auth/OIDC/IssuerChecker.php (1)
12-19: Robust issuer claim validation.The implementation correctly validates the issuer claim against three possible values (configured issuer, well-known issuer, and well-known issuer with trailing slash), providing comprehensive coverage for different OIDC provider configurations.
docs/docs/administration/08-advanced/01-external-authentication.md (2)
129-149: Comprehensive OIDC documentation.The OIDC configuration section provides clear guidance on required environment variables, scopes, and IDP configuration. The examples and explanations are well-structured and follow the established documentation pattern.
356-378: Clear OIDC mapping example.The OpenID Connect attribute and role mapping example is well-structured and demonstrates practical usage patterns for role-based access control with regex matching.
app/Auth/OIDC/OIDCProvider.php (1)
94-150: Well-implemented backchannel logout with proper security measures.The implementation correctly handles:
- JWT verification
- Replay attack prevention using JTI caching
- Proper session cleanup based on sid/sub claims
- Appropriate HTTP responses with cache control headers
app/Auth/OIDC/OpenIDConnectClient.php (3)
460-485: Excellent implementation of well-known discovery with proper caching.The implementation correctly:
- Caches responses based on Cache-Control headers
- Falls back to discovery when configuration is not available
- Handles HTTP errors appropriately
538-557: Proper implementation of token exchange with correct authentication.Good security practices:
- URL encoding of client credentials
- Using client_secret_basic authentication as per spec
- Proper error handling with throw()
601-603: Excellent security check preventing self-signed JWK attacks.This validation prevents a critical vulnerability where an attacker could include their own JWK in the JWT header to bypass signature verification.
| KC_DB: postgres | ||
| KC_DB_URL: jdbc:postgresql://keycloakdb:5432/keycloak | ||
| KC_DB_USERNAME: keycloak | ||
| KC_DB_PASSWORD: password |
There was a problem hiding this comment.
Extract hardcoded Keycloak credentials to environment variables.
Hardcoded credentials pose a security risk and should be externalized, even in development environments.
- KC_DB_USERNAME: keycloak
- KC_DB_PASSWORD: password
+ KC_DB_USERNAME: "${KEYCLOAK_DB_USERNAME:-keycloak}"
+ KC_DB_PASSWORD: "${KEYCLOAK_DB_PASSWORD:-password}"- KEYCLOAK_ADMIN: admin
- KEYCLOAK_ADMIN_PASSWORD: admin
+ KEYCLOAK_ADMIN: "${KEYCLOAK_ADMIN:-admin}"
+ KEYCLOAK_ADMIN_PASSWORD: "${KEYCLOAK_ADMIN_PASSWORD:-admin}"Also applies to: 139-140
🤖 Prompt for AI Agents
In docker-compose-dev.yml at lines 128 and also 139-140, the Keycloak
credentials are hardcoded, which is a security risk. Replace the hardcoded
values with environment variable references by using the syntax ${ENV_VAR_NAME}
so that the credentials are loaded from environment variables instead of being
directly in the file. Make sure to define these environment variables in a
secure place like a .env file or your system environment.
| POSTGRES_USER: keycloak | ||
| POSTGRES_PASSWORD: password |
There was a problem hiding this comment.
Extract hardcoded PostgreSQL credentials to environment variables.
Maintain consistency with the Keycloak service by using the same environment variables for database credentials.
- POSTGRES_USER: keycloak
- POSTGRES_PASSWORD: password
+ POSTGRES_USER: "${KEYCLOAK_DB_USERNAME:-keycloak}"
+ POSTGRES_PASSWORD: "${KEYCLOAK_DB_PASSWORD:-password}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| POSTGRES_USER: keycloak | |
| POSTGRES_PASSWORD: password | |
| POSTGRES_USER: "${KEYCLOAK_DB_USERNAME:-keycloak}" | |
| POSTGRES_PASSWORD: "${KEYCLOAK_DB_PASSWORD:-password}" |
🤖 Prompt for AI Agents
In docker-compose-dev.yml at lines 151 to 152, the PostgreSQL credentials are
hardcoded as 'keycloak' and 'password'. Replace these hardcoded values with
environment variable references, matching the variables used in the Keycloak
service for database username and password. This ensures consistency and better
security by externalizing sensitive information.
| image: quay.io/keycloak/keycloak:26.2 | ||
| container_name: idp.test | ||
| links: | ||
| - "app:dock-swrh68.ges.thm.de" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make the link alias configurable.
The hardcoded domain dock-swrh68.ges.thm.de should be extracted to an environment variable to make the configuration more flexible across different development environments.
- - "app:dock-swrh68.ges.thm.de"
+ - "app:${KEYCLOAK_APP_ALIAS:-localhost}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - "app:dock-swrh68.ges.thm.de" | |
| - "app:${KEYCLOAK_APP_ALIAS:-localhost}" |
🤖 Prompt for AI Agents
In docker-compose-dev.yml at line 123, replace the hardcoded domain
"dock-swrh68.ges.thm.de" with an environment variable reference. Define this
environment variable in the appropriate .env file or environment configuration,
then update the docker-compose file to use this variable for the link alias,
enabling flexible configuration across different development setups.
| $alg = $this->openIDConnectClient->getIdTokenHeader()['alg']; | ||
|
|
||
| $bit = match ($alg) { | ||
| 'EdDSA' => '512', | ||
| default => substr($alg, 2, 3), | ||
| }; |
There was a problem hiding this comment.
Add validation for algorithm format and handle unknown algorithms.
The current implementation assumes the algorithm follows the pattern where positions 2-4 contain the bit size (e.g., RS256). This could fail for malformed algorithms.
public function checkClaim($value): void
{
$alg = $this->openIDConnectClient->getIdTokenHeader()['alg'];
+
+ if (!is_string($alg)) {
+ throw new InvalidClaimException('Algorithm must be a string', 'at_hash', $value);
+ }
$bit = match ($alg) {
'EdDSA' => '512',
- default => substr($alg, 2, 3),
+ default => strlen($alg) >= 5 ? substr($alg, 2, 3) : throw new InvalidClaimException('Invalid algorithm format: ' . $alg, 'at_hash', $value),
};
+
+ if (!is_numeric($bit)) {
+ throw new InvalidClaimException('Unable to determine hash size for algorithm: ' . $alg, 'at_hash', $value);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $alg = $this->openIDConnectClient->getIdTokenHeader()['alg']; | |
| $bit = match ($alg) { | |
| 'EdDSA' => '512', | |
| default => substr($alg, 2, 3), | |
| }; | |
| public function checkClaim($value): void | |
| { | |
| $alg = $this->openIDConnectClient->getIdTokenHeader()['alg']; | |
| if (!is_string($alg)) { | |
| throw new InvalidClaimException('Algorithm must be a string', 'at_hash', $value); | |
| } | |
| $bit = match ($alg) { | |
| 'EdDSA' => '512', | |
| default => strlen($alg) >= 5 | |
| ? substr($alg, 2, 3) | |
| : throw new InvalidClaimException( | |
| 'Invalid algorithm format: ' . $alg, | |
| 'at_hash', | |
| $value | |
| ), | |
| }; | |
| if (!is_numeric($bit)) { | |
| throw new InvalidClaimException( | |
| 'Unable to determine hash size for algorithm: ' . $alg, | |
| 'at_hash', | |
| $value | |
| ); | |
| } | |
| // ... rest of method unchanged ... | |
| } |
🤖 Prompt for AI Agents
In app/Auth/OIDC/AccessTokenHashChecker.php around lines 14 to 19, the code
assumes the algorithm string format is always valid and extracts bits by
substring without validation. Add validation to check if the algorithm string
matches the expected pattern before extracting bits, and handle unknown or
malformed algorithms gracefully by providing a default case or throwing an
informative exception.
app/Auth/OIDC/OIDCProvider.php
Outdated
| // try { | ||
| if (! $this->openIDConnectClient->authenticate()) { | ||
| return $this->redirect($request); | ||
| } | ||
|
|
||
| $claims = $this->openIDConnectClient->getVerifiedClaims(); | ||
|
|
||
| /*} catch (\Exception $e) { | ||
| Log::error($e); | ||
|
|
||
| return redirect('/external_login?error=openid_connect_exception'); | ||
| } | ||
| */ |
There was a problem hiding this comment.
Critical: Uncomment and properly implement exception handling.
The commented-out exception handling blocks are a critical security issue. Authentication flows must have proper error handling to prevent information disclosure and ensure secure failure modes.
public function login(Request $request)
{
- // try {
+ try {
if (! $this->openIDConnectClient->authenticate()) {
return $this->redirect($request);
}
$claims = $this->openIDConnectClient->getVerifiedClaims();
- /*} catch (\Exception $e) {
+ } catch (\Exception $e) {
Log::error($e);
return redirect('/external_login?error=openid_connect_exception');
}
-*/
// Create new open-id connect user
- // try {
+ try {
$user_info = get_object_vars($this->openIDConnectClient->requestUserInfo());
$oidc_user = new OIDCUser($user_info);
- /* } catch (MissingAttributeException $e) {
+ } catch (MissingAttributeException $e) {
return redirect('/external_login?error=missing_attributes');
} catch (\Exception $e) {
return redirect('/external_login?error=openid_connect_exception');
- }*/
+ }Also applies to: 57-65
🤖 Prompt for AI Agents
In app/Auth/OIDC/OIDCProvider.php around lines 42 to 54 and 57 to 65, the
exception handling code is commented out, which is a critical security issue.
Uncomment the try-catch blocks around the authentication and claims retrieval
code, and ensure that exceptions are caught properly. In the catch block, log
the exception securely using Log::error($e) and redirect the user to a safe
error page such as '/external_login?error=openid_connect_exception' to handle
failures securely without exposing sensitive information.
app/Auth/OIDC/OIDCProvider.php
Outdated
| public function logout($redirect): false|string | ||
| { | ||
| if (! $this->openIDConnectClient->hasEndSessionEndpoint()) { | ||
| return false; | ||
| } | ||
|
|
||
| return $this->openIDConnectClient->getSignOutUrl(session('oidc_id_token'), $redirect); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Validate the redirect URL to prevent open redirect vulnerabilities.
The logout method accepts a redirect URL without validation, which could lead to open redirect attacks.
🏁 Script executed:
#!/bin/bash
# Description: Check if there's URL validation elsewhere in the codebase for OIDC redirects
# Search for URL validation patterns in OIDC-related files
rg -A 5 "redirect.*validat|validat.*redirect|isValidRedirect|validateUrl" --glob "*.php"
# Check if there's a whitelist of allowed redirect URLs
rg -A 5 "allowed.*redirect|redirect.*whitelist|trusted.*redirect" --glob "*.php"Length of output: 506
Validate the redirect URL in logout() to prevent open redirects
The logout() method in app/Auth/OIDC/OIDCProvider.php currently forwards whatever is passed in $redirect to the OIDC sign-out endpoint, creating a potential open redirect vulnerability. You should validate or whitelist this parameter before use.
• File: app/Auth/OIDC/OIDCProvider.php
Lines: 22–29
- Ensure
$redirectis either a relative path or matches a predefined list of allowed URLs/domains. - Examples of checks you can apply:
• Use PHP’sfilter_var($redirect, FILTER_VALIDATE_URL)and then verify the host against an allow-list.
• Restrict to relative paths only (strpos($redirect, '/') === 0).
• Leverage Laravel’s validator with a custom rule or enum of permitted redirect URLs.
public function logout($redirect): false|string
{
// Example whitelist check
if (! $this->isAllowedRedirect($redirect)) {
return false;
}
if (! $this->openIDConnectClient->hasEndSessionEndpoint()) {
return false;
}
return $this->openIDConnectClient->getSignOutUrl(
session('oidc_id_token'),
$redirect
);
}
// In the same class (or a helper/validator service)
protected function isAllowedRedirect(string $url): bool
{
// Only allow relative paths
if (str_starts_with($url, '/')) {
return true;
}
// Or validate full URLs against a whitelist
$host = parse_url($url, PHP_URL_HOST);
return in_array($host, ['example.com', 'app.example.com'], true);
}🤖 Prompt for AI Agents
In app/Auth/OIDC/OIDCProvider.php lines 22 to 29, the logout() method uses the
$redirect parameter directly, risking open redirect vulnerabilities. Fix this by
adding validation to ensure $redirect is either a relative path starting with
'/' or a full URL whose host matches a predefined whitelist of allowed domains.
Implement a helper method like isAllowedRedirect() to perform these checks and
return false from logout() if validation fails before proceeding with the
sign-out URL generation.
| public function redirect($redirect = null) | ||
| { | ||
| if ($redirect) { | ||
| \Session::put('redirect_url', $redirect); | ||
| } | ||
|
|
||
| return redirect($this->openIDConnectClient->getAuthenticationRequestUrl()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate redirect URL and use imported Session facade.
The redirect URL should be validated before storing in session, and use the consistent Session facade syntax.
public function redirect($redirect = null)
{
if ($redirect) {
- \Session::put('redirect_url', $redirect);
+ // TODO: Implement URL validation to ensure redirect is to a trusted domain
+ Session::put('redirect_url', $redirect);
}
return redirect($this->openIDConnectClient->getAuthenticationRequestUrl());
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function redirect($redirect = null) | |
| { | |
| if ($redirect) { | |
| \Session::put('redirect_url', $redirect); | |
| } | |
| return redirect($this->openIDConnectClient->getAuthenticationRequestUrl()); | |
| } | |
| public function redirect($redirect = null) | |
| { | |
| if ($redirect) { | |
| // TODO: Implement URL validation to ensure redirect is to a trusted domain | |
| Session::put('redirect_url', $redirect); | |
| } | |
| return redirect($this->openIDConnectClient->getAuthenticationRequestUrl()); | |
| } |
🤖 Prompt for AI Agents
In app/Auth/OIDC/OIDCProvider.php around lines 31 to 38, the redirect URL passed
to the redirect method should be validated to ensure it is a safe and valid URL
before storing it in the session. Also, replace the global \Session call with
the imported Session facade for consistency. Add validation logic to check the
URL format and only store it if valid, then use Session::put instead of
\Session::put.
| if (isset($_REQUEST['error'])) { | ||
| $desc = isset($_REQUEST['error_description']) ? ' Description: '.$_REQUEST['error_description'] : ''; | ||
| throw new OpenIDConnectClientException('Error: '.$_REQUEST['error'].$desc); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid exposing detailed error information to users.
The error description from the OIDC provider is directly exposed to the user, which could leak sensitive information.
if (isset($_REQUEST['error'])) {
- $desc = isset($_REQUEST['error_description']) ? ' Description: '.$_REQUEST['error_description'] : '';
- throw new OpenIDConnectClientException('Error: '.$_REQUEST['error'].$desc);
+ Log::error('OIDC authentication error', [
+ 'error' => $_REQUEST['error'],
+ 'description' => $_REQUEST['error_description'] ?? null
+ ]);
+ throw new OpenIDConnectClientException('Authentication failed');
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (isset($_REQUEST['error'])) { | |
| $desc = isset($_REQUEST['error_description']) ? ' Description: '.$_REQUEST['error_description'] : ''; | |
| throw new OpenIDConnectClientException('Error: '.$_REQUEST['error'].$desc); | |
| } | |
| if (isset($_REQUEST['error'])) { | |
| Log::error('OIDC authentication error', [ | |
| 'error' => $_REQUEST['error'], | |
| 'description' => $_REQUEST['error_description'] ?? null | |
| ]); | |
| throw new OpenIDConnectClientException('Authentication failed'); | |
| } |
🤖 Prompt for AI Agents
In app/Auth/OIDC/OpenIDConnectClient.php around lines 200 to 203, the code
exposes detailed error descriptions from the OIDC provider directly to users,
risking sensitive information leakage. Modify the error handling to log the
detailed error description internally but only throw a generic error message to
the user without including the detailed description.
a5a7617 to
ad3282a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
docker-compose-dev.yml (3)
126-127: Credentials still hard-coded – please externalise.
This was already raised in a previous review and remains unresolved.- KC_DB_USERNAME: keycloak - KC_DB_PASSWORD: password + KC_DB_USERNAME: "${KEYCLOAK_DB_USERNAME:-keycloak}" + KC_DB_PASSWORD: "${KEYCLOAK_DB_PASSWORD:-password}"
137-138: Admin credentials hard-coded – same issue.Reuse the same env-var pattern as above to keep secrets out of VCS.
145-151: PostgreSQL credentials hard-coded – still pending.Same comment as before; please externalise:
- POSTGRES_USER: keycloak - POSTGRES_PASSWORD: password + POSTGRES_USER: "${KEYCLOAK_DB_USERNAME:-keycloak}" + POSTGRES_PASSWORD: "${KEYCLOAK_DB_PASSWORD:-password}"app/Auth/OIDC/OIDCProvider.php (1)
29-40: Security: Validate redirect URL to prevent open redirect vulnerabilities.The logout method accepts and uses the
$redirectparameter without validation, creating a potential open redirect vulnerability. An attacker could redirect users to malicious sites.Add validation before using the redirect URL:
public function logout($redirect): false|string { + // Validate redirect URL to prevent open redirects + if ($redirect && !$this->isValidRedirectUrl($redirect)) { + return false; + } + if (! $this->openIDConnectClient->hasEndSessionEndpoint()) { return false; }Consider implementing a
isValidRedirectUrl()method that checks against a whitelist of allowed domains or restricts to relative paths only.app/Auth/OIDC/OpenIDConnectClient.php (1)
227-230: Security: Avoid exposing detailed error information.The error description from the OIDC provider is directly exposed to the user, which could leak sensitive information about the authentication system.
if ($request->has('error')) { - $desc = $request->has('error_description') ? ' Description: '.$request->input('error_description') : ''; - throw new OpenIDConnectProviderException('Authentication Error Response'.$desc, $request->input('error')); + Log::error('OIDC authentication error', [ + 'error' => $request->input('error'), + 'description' => $request->input('error_description') + ]); + throw new OpenIDConnectProviderException('Authentication failed', $request->input('error')); }
🧹 Nitpick comments (8)
docker-compose-dev.yml (4)
121-122: Replace deprecatedlinkswith a network alias.
linksis long-deprecated in Compose v3+ and brings no benefit over the default service-name DNS plusdepends_on. Prefer:networks: default: aliases: - ${KEYCLOAK_APP_ALIAS:-app.local}and drop the whole
linkssection.
This avoids future breakage whenlinkssupport is finally removed.
120-120: Avoid hard-codingcontainer_namein dev stacks.
container_name: idp.testprevents Compose from adding a project-specific prefix (pilos-dev_), making parallel stacks impossible. Unless some external script relies on that exact name, simply remove the key and let Compose generate the name.
142-143: Port clash risk on8080:8080.Many dev stacks already use 8080 (e.g., phpMyAdmin earlier in this file).
Consider mapping to an alternate host port to avoid conflicts:- - 8080:8080 + - "${KEYCLOAK_PORT:-18080}:8080"
145-151: Missing healthcheck forkeycloakdb.Adding a basic healthcheck keeps
keycloak_webfrom starting before Postgres is ready:healthcheck: test: ["CMD-SHELL", "pg_isready -U $$POSTGRES_USER -d $$POSTGRES_DB"] interval: 5s retries: 12 timeout: 5sapp/Auth/OIDC/OIDCProvider.php (1)
48-49: Use imported Session facade for consistency.The code uses the global
\Sessionclass instead of the imported Session facade. This is inconsistent with Laravel conventions and the import statement is missing.+ use Illuminate\Support\Facades\Session;And update the usage:
if ($redirect) { - \Session::put('redirect_url', $redirect); + Session::put('redirect_url', $redirect); }app/Auth/OIDC/OpenIDConnectClient.php (3)
486-507: Improve cache key security and error handling.The well-known configuration URL is used directly as a cache key, which could be problematic if URLs contain sensitive information. Also consider adding timeout for HTTP requests.
if (! $this->wellKnown) { $well_known_config_url = Str::finish($this->getProviderURL(), '/').'.well-known/openid-configuration'; + $cache_key = 'oidc_well_known_'.hash('sha256', $well_known_config_url); - if (\Cache::has($well_known_config_url)) { - $this->wellKnown = \Cache::get($well_known_config_url); + if (\Cache::has($cache_key)) { + $this->wellKnown = \Cache::get($cache_key); } else { try { $response = $this->getHttpClient()->get($well_known_config_url)->throw(); } catch (\Throwable $e) { throw new OpenIDConnectNetworkException('Unable to fetch openid configuration: '.$e->getMessage(), $e->getCode()); } // ... rest of caching logic with $cache_key
645-666: Optimize JWKS caching with secure key generation.Similar to the well-known config, using the JWKS URI directly as a cache key could expose sensitive information in cache systems.
+$jwks_cache_key = 'oidc_jwks_'.hash('sha256', $jwksUri); -if (\Cache::has($jwksUri)) { - $jwkSetResponse = \Cache::get($jwksUri); +if (\Cache::has($jwks_cache_key)) { + $jwkSetResponse = \Cache::get($jwks_cache_key); } else { // ... fetch logic if ($maxAge > 0) { - \Cache::put($jwksUri, $response->body(), $maxAge); + \Cache::put($jwks_cache_key, $response->body(), $maxAge); }
1049-1053: Use native base64url encoding for better security.Consider using Laravel's built-in base64url encoding or a more secure implementation to avoid potential edge cases.
public function base64url_encode($data) { - // Convert Base64 to Base64URL by replacing "+" with "-" and "/" with "_" and remove tailing "=" if any - return rtrim(strtr(base64_encode($data), '+/', '-_'), '='); + return Str::toBase64Url($data); }If
Str::toBase64Url()is not available in your Laravel version, the current implementation is acceptable but consider adding input validation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
app/Auth/ExternalUser.php(1 hunks)app/Auth/OIDC/AccessTokenHashChecker.php(1 hunks)app/Auth/OIDC/EventsChecker.php(1 hunks)app/Auth/OIDC/IssuerChecker.php(1 hunks)app/Auth/OIDC/OIDCController.php(1 hunks)app/Auth/OIDC/OIDCProvider.php(1 hunks)app/Auth/OIDC/OIDCServiceProvider.php(1 hunks)app/Auth/OIDC/OpenIDConnectClient.php(1 hunks)app/Auth/OIDC/OpenIDConnectClientException.php(1 hunks)app/Auth/OIDC/OpenIDConnectCodeMissingException.php(1 hunks)app/Auth/OIDC/OpenIDConnectNetworkException.php(1 hunks)app/Auth/OIDC/OpenIDConnectProviderException.php(1 hunks)app/Auth/OIDC/OpenIDConnectValidationException.php(1 hunks)app/Auth/Shibboleth/ShibbolethProvider.php(2 hunks)app/Http/Controllers/api/v1/auth/LoginController.php(3 hunks)composer.json(3 hunks)config/app.php(1 hunks)config/services.php(2 hunks)docker-compose-dev.yml(1 hunks)lang/en/auth.php(2 hunks)resources/js/views/ExternalLogin.vue(1 hunks)routes/web.php(2 hunks)
✅ Files skipped from review due to trivial changes (6)
- app/Auth/Shibboleth/ShibbolethProvider.php
- app/Auth/OIDC/OpenIDConnectProviderException.php
- app/Auth/OIDC/OpenIDConnectValidationException.php
- app/Auth/OIDC/OpenIDConnectNetworkException.php
- app/Auth/OIDC/OpenIDConnectCodeMissingException.php
- app/Auth/ExternalUser.php
🚧 Files skipped from review as they are similar to previous changes (12)
- app/Auth/OIDC/OpenIDConnectClientException.php
- resources/js/views/ExternalLogin.vue
- config/app.php
- app/Http/Controllers/api/v1/auth/LoginController.php
- composer.json
- app/Auth/OIDC/IssuerChecker.php
- routes/web.php
- app/Auth/OIDC/EventsChecker.php
- lang/en/auth.php
- app/Auth/OIDC/OIDCController.php
- config/services.php
- app/Auth/OIDC/AccessTokenHashChecker.php
🧰 Additional context used
🧬 Code Graph Analysis (1)
app/Auth/OIDC/OpenIDConnectClient.php (7)
app/Auth/OIDC/AccessTokenHashChecker.php (2)
__construct(10-10)AccessTokenHashChecker(8-33)app/Auth/OIDC/EventsChecker.php (2)
__construct(10-10)EventsChecker(8-27)app/Auth/OIDC/IssuerChecker.php (2)
__construct(10-10)IssuerChecker(8-25)app/Auth/OIDC/OpenIDConnectProviderException.php (1)
OpenIDConnectProviderException(5-5)app/Auth/OIDC/OpenIDConnectValidationException.php (1)
OpenIDConnectValidationException(5-5)app/Auth/OIDC/OpenIDConnectClientException.php (1)
OpenIDConnectClientException(5-5)app/Auth/OIDC/OpenIDConnectNetworkException.php (1)
OpenIDConnectNetworkException(5-5)
🪛 PHPMD (2.15.0)
app/Auth/OIDC/OIDCServiceProvider.php
16-16: Avoid unused parameters such as '$app'. (Unused Code Rules)
(UnusedFormalParameter)
🪛 Gitleaks (8.26.0)
docker-compose-dev.yml
126-126: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Docker Build
- GitHub Check: Backend
🔇 Additional comments (5)
app/Auth/OIDC/OIDCServiceProvider.php (1)
16-31: LGTM! Solid service provider implementation.The singleton registration correctly configures the OpenIDConnectClient with all necessary OIDC parameters and wraps it in an OIDCProvider. The static analysis warning about the unused
$appparameter is a false positive - it's used for type hinting in the closure which is a standard Laravel pattern.app/Auth/OIDC/OIDCProvider.php (2)
72-106: Good implementation of authentication flow with proper error handling.The login method properly handles the authentication flow with appropriate exception declarations and user synchronization. The session management for OIDC-specific data (sub, sid, id_token) is well implemented.
108-154: Excellent back-channel logout implementation with security measures.The back-channel logout implementation includes proper:
- JWT verification and claim validation
- Replay attack prevention using JTI caching
- Session cleanup based on sid/sub claims
- Comprehensive error handling and logging
This follows OIDC back-channel logout specification correctly.
app/Auth/OIDC/OpenIDConnectClient.php (2)
617-694: Strong JWT signature verification implementation.The signature verification logic correctly handles multiple algorithms, validates JWKS, implements proper key selection with restrictions, and includes security checks like rejecting self-signed JWK headers. The fallback to client secret for HMAC algorithms is appropriate.
796-813: Proper UserInfo response validation against token substitution attacks.The implementation correctly validates the
subclaim in UserInfo responses against the ID token to prevent token substitution attacks, following OIDC security best practices.
| if (isset($token_json->error)) { | ||
| $desc = isset($token_json->error_description) ? ' Description: '.$token_json->error_description : ''; | ||
| throw new OpenIDConnectProviderException('Token Error Response'.$desc, $token_json->error); | ||
| } |
There was a problem hiding this comment.
Security: Avoid exposing detailed token error information.
Similar to the authentication error, the token error response exposes detailed error descriptions that could reveal sensitive system information.
if (isset($token_json->error)) {
- $desc = isset($token_json->error_description) ? ' Description: '.$token_json->error_description : '';
- throw new OpenIDConnectProviderException('Token Error Response'.$desc, $token_json->error);
+ Log::error('OIDC token error', [
+ 'error' => $token_json->error,
+ 'description' => $token_json->error_description ?? null
+ ]);
+ throw new OpenIDConnectProviderException('Token request failed', $token_json->error);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (isset($token_json->error)) { | |
| $desc = isset($token_json->error_description) ? ' Description: '.$token_json->error_description : ''; | |
| throw new OpenIDConnectProviderException('Token Error Response'.$desc, $token_json->error); | |
| } | |
| if (isset($token_json->error)) { | |
| Log::error('OIDC token error', [ | |
| 'error' => $token_json->error, | |
| 'description' => $token_json->error_description ?? null | |
| ]); | |
| throw new OpenIDConnectProviderException('Token request failed', $token_json->error); | |
| } |
🤖 Prompt for AI Agents
In app/Auth/OIDC/OpenIDConnectClient.php around lines 250 to 253, the token
error response exposes detailed error descriptions that may reveal sensitive
information. Modify the code to throw a generic error message without including
the detailed error description from the token response, ensuring sensitive
system details are not exposed in exceptions.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/Backend/Feature/api/v1/OIDCTest.php (3)
17-77: Extract complex JSON mapping to improve maintainability.The 60-line JSON mapping configuration embedded as a string property makes the test file harder to read and maintain. Consider extracting this to a separate fixture file or factory method.
Create a separate fixture file or method:
- private $mapping = ' - { - "attributes": { - "external_id": "principalname", - ... - } - } - '; + private function getTestMapping(): string + { + return file_get_contents(__DIR__ . '/fixtures/oidc-mapping.json'); + }Or alternatively, create a factory method that returns the decoded JSON directly:
- private $mapping = '...'; + private function getTestMappingConfig(): object + { + return (object) [ + 'attributes' => (object) [ + 'external_id' => 'principalname', + 'first_name' => 'givenname', + // ... rest of configuration + ], + 'roles' => [ + // ... role configurations + ] + ]; + }
79-115: Consider extracting configuration setup to improve test maintainability.The setUp method is doing a lot of configuration work. While functional, it could be more maintainable by extracting some of the configuration into helper methods.
Extract OIDC configuration to a helper method:
protected function setUp(): void { parent::setUp(); + $this->setupOIDCConfiguration(); + $this->createTestRoles(); + $this->mockOIDCProvider(); +} + +private function setupOIDCConfiguration(): void +{ Config::set('services.oidc.enabled', true); Config::set('services.oidc.client_id', 'fake-client-id'); Config::set('services.oidc.client_secret', 'fake-client-secret'); Config::set('services.oidc.issuer', 'https://example.org'); Config::set('services.oidc.scopes', ['profile', 'email']); Config::set('services.oidc.leeway', 60); Config::set('services.oidc.timeout', 10); Config::set('services.oidc.cache_config_max_age', 0); Config::set('services.oidc.cache_jwks_max_age', 0); Config::set('services.oidc.mapping', json_decode($this->mapping)); Config::set('app.enabled_locales', ['de' => ['name' => 'Deutsch', 'dateTimeFormat' => []], 'en' => ['name' => 'English', 'dateTimeFormat' => []], 'fr' => ['name' => 'Français', 'dateTimeFormat' => []]]); +}
96-96: Verify JSON mapping configuration is valid.The JSON mapping is decoded without error handling. Consider adding validation to ensure the mapping configuration is valid before setting it.
Add validation for the JSON mapping:
-Config::set('services.oidc.mapping', json_decode($this->mapping)); +$mappingConfig = json_decode($this->mapping); +if (json_last_error() !== JSON_ERROR_NONE) { + throw new \InvalidArgumentException('Invalid JSON mapping configuration: ' . json_last_error_msg()); +} +Config::set('services.oidc.mapping', $mappingConfig);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/Auth/OIDC/OIDCController.php(1 hunks)tests/Backend/Feature/api/v1/OIDCTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Auth/OIDC/OIDCController.php
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/Backend/Feature/api/v1/OIDCTest.php (2)
tests/Backend/TestCase.php (1)
TestCase(17-80)app/Auth/OIDC/OIDCController.php (1)
redirect(21-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Backend
- GitHub Check: Docker Build
🔇 Additional comments (4)
tests/Backend/Feature/api/v1/OIDCTest.php (4)
1-16: LGTM! Clean test class structure.The imports, namespace, and class setup are well-organized. The use of
RefreshDatabaseandWithFakertraits is appropriate for feature tests that need database interactions.
135-144: Good error handling test coverage.The test properly verifies that network exceptions are handled gracefully by redirecting to the external login page with an appropriate error parameter.
152-175: Excellent comprehensive redirect test.This test thoroughly validates the authorization URL construction, including all required OAuth2/OIDC parameters. The verification of session-stored nonce and state values is particularly important for security.
194-199: Verified intentional use of custom HTTP 420 responseThe 420 status is consistently returned by the RedirectIfAuthenticated middleware and asserted across all related authentication tests, including OIDC, Shibboleth, login, logout, and password routes. No change to a standard status code is needed.
• app/Http/Middleware/RedirectIfAuthenticated.php:24 –
setStatusCode(420, 'Guests only')
• tests/Backend/Feature/api/v1/* – multiple->assertStatus(420)calls in authentication callback and password/reset tests
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (5)
app/Auth/OIDC/OIDCController.php (1)
70-74: Potential open redirect vulnerability in redirect URL handling.The code uses the
redirect_urlfrom the session without validation, which could lead to open redirect attacks. This was also mentioned in past reviews but appears unaddressed.The redirect URL should be validated before use:
if ($request->session()->has('redirect_url')) { + $redirect = $request->session()->get('redirect_url'); + // Validate that the redirect URL is safe + if (!$this->isValidRedirectUrl($redirect)) { + return redirect($url); + } return redirect(\Uri::of($url) - ->withQuery(['redirect', $request->session()->get('redirect_url')]) + ->withQuery(['redirect', $redirect]) ->value()); }Add a validation method to the controller:
private function isValidRedirectUrl(string $url): bool { // Only allow relative paths or same-origin URLs if (str_starts_with($url, '/')) { return true; } $parsedUrl = parse_url($url); $currentHost = request()->getHost(); return $parsedUrl && isset($parsedUrl['host']) && $parsedUrl['host'] === $currentHost; }app/Auth/OIDC/OIDCProvider.php (2)
29-42: URL validation missing in logout method.The logout method accepts a redirect URL without validation, which could lead to open redirect attacks as mentioned in past reviews.
Validate the redirect URL before passing it to the OIDC client:
public function logout(string $redirect): false|string { + // Validate redirect URL to prevent open redirects + if (!$this->isValidRedirectUrl($redirect)) { + return false; + } + if (! $this->openIDConnectClient->hasEndSessionEndpoint()) { return false; } try { return $this->openIDConnectClient->getSignOutUrl(session('oidc_id_token'), $redirect); } catch (\Throwable $e) { \Log::error($e->getMessage()); return false; } }Add URL validation helper:
private function isValidRedirectUrl(string $url): bool { // Only allow relative paths or same-origin URLs if (str_starts_with($url, '/')) { return true; } $parsedUrl = parse_url($url); $currentHost = request()->getHost(); return $parsedUrl && isset($parsedUrl['host']) && $parsedUrl['host'] === $currentHost; }
48-55: URL validation and Session facade consistency needed.The redirect method stores the redirect URL without validation and uses
\Session::put()instead of the imported facade.public function redirect($redirect = null) { if ($redirect) { + // Validate redirect URL to prevent open redirects + if (!$this->isValidRedirectUrl($redirect)) { + throw new \InvalidArgumentException('Invalid redirect URL'); + } - \Session::put('redirect_url', $redirect); + Session::put('redirect_url', $redirect); } return redirect($this->openIDConnectClient->getAuthenticationRequestUrl()); }Also add the Session facade import:
+use Illuminate\Support\Facades\Session;app/Auth/OIDC/OpenIDConnectClient.php (2)
227-230: Security: Avoid exposing detailed error information to users.Similar to the previous review feedback, this error handling exposes detailed error descriptions from the OIDC provider directly to users, which could leak sensitive information.
Apply this diff to log detailed errors internally while showing generic messages to users:
- if ($request->has('error')) { - $desc = $request->has('error_description') ? ' Description: '.$request->input('error_description') : ''; - throw new OpenIDConnectProviderException('Authentication Error Response'.$desc, $request->input('error')); - } + if ($request->has('error')) { + \Log::error('OIDC authentication error', [ + 'error' => $request->input('error'), + 'description' => $request->input('error_description') + ]); + throw new OpenIDConnectProviderException('Authentication failed', $request->input('error')); + }
574-578: Security: Avoid exposing detailed token error information.This token error response handling also exposes detailed error descriptions that may reveal sensitive system information.
Apply this diff to log detailed errors internally while showing generic messages to users:
- try { - $this->tokenResponse = $response->throw()->object(); - } catch (\Throwable $e) { - throw new OpenIDConnectProviderException('Token Error Response '.$e->getMessage(), $e->getCode()); - } + try { + $this->tokenResponse = $response->throw()->object(); + } catch (\Throwable $e) { + \Log::error('OIDC token error', [ + 'error' => $e->getMessage(), + 'code' => $e->getCode() + ]); + throw new OpenIDConnectProviderException('Token request failed', $e->getCode()); + }
🧹 Nitpick comments (8)
docs/docs/administration/08-advanced/01-external-authentication.md (5)
3-3: Fix hyphenation consistency in the description.The documentation should use consistent hyphenation for "OpenID Connect" throughout. Based on the official OpenID Connect specification, it should be "OpenID Connect" without hyphens.
-description: Guide how to connect PILOS to external authentication systems like LDAP, Shibboleth and OpenID Connect. +description: Guide how to connect PILOS to external authentication systems like LDAP, Shibboleth, and OpenID Connect.
14-14: Improve grammar and consistency in the interface description.The sentence needs better punctuation and consistent terminology.
-In large environments it is impractical to manage all users in PILOS. Therefore PILOS can be connected to external authentication systems. LDAP, Shibboleth and OpenID-Connect are available as interfaces. All three authentication providers can be operated in parallel, but none of them more than once. +In large environments it is impractical to manage all users in PILOS. Therefore, PILOS can be connected to external authentication systems. LDAP, Shibboleth, and OpenID Connect are available as interfaces. All three authentication providers can be operated in parallel, but none of them more than once.
133-133: Fix grammar in the scopes description.Use proper hyphenation for compound adjectives.
-The required `openid` scope is always present, even if not explicitly set. If you need more scopes to get all required attributes, add them as a comma separated list. The default value for `OIDC_SCOPES` is `profile,email`. +The required `openid` scope is always present, even if not explicitly set. If you need more scopes to get all required attributes, add them as a comma-separated list. The default value for `OIDC_SCOPES` is `profile,email`.
146-148: Format URLs as proper markdown links.The bare URLs should be formatted as proper markdown links for better readability and to address the markdownlint warnings.
-- Redirect URI: https://your-domain.com/auth/oidc/callback -- Post Logout Redirect URI: https://your-domain.com/logout -- Back-Channel Logout URI: https://your-domain.com/auth/oidc/logout +- Redirect URI: `https://your-domain.com/auth/oidc/callback` +- Post Logout Redirect URI: `https://your-domain.com/logout` +- Back-Channel Logout URI: `https://your-domain.com/auth/oidc/logout`
365-365: Fix grammar in the example description.Add proper comma usage for clarity.
-In this example the OpenID Connect provider returns the claim `preferred_username` which contains the username and an additional claim `roles` with an array of roles. +In this example, the OpenID Connect provider returns the claim `preferred_username` which contains the username, and an additional claim `roles` with an array of roles.app/Auth/OIDC/OpenIDConnectClient.php (3)
683-685: Improve error handling for JWK selection.The null check for
$jwkis good, but the error message could be more specific about what went wrong during JWK selection.Consider this improvement:
- if ($jwk === null) { - throw new OpenIDConnectClientException('Unable to find JWK for algorithm: '.$alg); - } + if ($jwk === null) { + throw new OpenIDConnectClientException('Unable to find suitable JWK for algorithm: '.$alg.'. Check provider JWKS configuration.'); + }
947-1002: Session management uses Laravel facades directly.The session management uses Laravel's Session facade directly, which is acceptable but could be improved for testability and dependency injection.
Consider injecting the session manager or using a service class for session operations:
+use Illuminate\Session\SessionManager; + +public function __construct( + string $providerUrl, + private string $clientID, + private string $clientSecret, + private string $redirectURL, + private SessionManager $session +) {Then update the session methods to use
$this->sessioninstead of theSessionfacade.
1057-1061: Consider using existing library for base64url encoding.The custom
base64url_encodemethod reimplements functionality that might be available in existing libraries or Laravel utilities.Consider using Laravel's
Str::base64urlEncode()method if available, or move this to a dedicated utility class:- public function base64url_encode($data) - { - // Convert Base64 to Base64URL by replacing "+" with "-" and "/" with "_" and remove tailing "=" if any - return rtrim(strtr(base64_encode($data), '+/', '-_'), '='); - } + public function base64url_encode($data) + { + return \Illuminate\Support\Str::base64urlEncode($data); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/Auth/OIDC/OIDCController.php(1 hunks)app/Auth/OIDC/OIDCProvider.php(1 hunks)app/Auth/OIDC/OIDCServiceProvider.php(1 hunks)app/Auth/OIDC/OpenIDConnectClient.php(1 hunks)docs/docs/administration/08-advanced/01-external-authentication.md(6 hunks)tests/Backend/Feature/api/v1/OIDCTest.php(1 hunks)tests/Backend/Unit/OpenIDConnectClientTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Auth/OIDC/OIDCServiceProvider.php
🧰 Additional context used
🧬 Code Graph Analysis (2)
app/Auth/OIDC/OIDCController.php (4)
app/Auth/OIDC/OIDCProvider.php (2)
redirect(48-55)logout(29-42)app/Auth/Shibboleth/ShibbolethProvider.php (2)
redirect(67-75)logout(19-22)_ide_helper.php (3)
now(14005-14009)save(13781-13785)value(17989-17993)app/Http/Controllers/api/v1/auth/LoginController.php (1)
logout(68-95)
tests/Backend/Unit/OpenIDConnectClientTest.php (6)
app/Auth/OIDC/OpenIDConnectClient.php (5)
OpenIDConnectClient(76-1062)verifyJWSSignature(613-690)unserializeJWS(711-714)verifyIdTokenClaims(302-317)verifyLogoutTokenClaims(409-433)app/Auth/OIDC/OpenIDConnectClientException.php (1)
OpenIDConnectClientException(5-5)app/Auth/OIDC/OpenIDConnectValidationException.php (1)
OpenIDConnectValidationException(5-5)tests/Backend/TestCase.php (1)
TestCase(17-80)tests/Backend/Feature/api/v1/OIDCTest.php (3)
setUp(107-128)createJWS(130-161)signClaims(163-170)_ide_helper.php (3)
preventStrayRequests(7454-7458)fake(7417-7421)method(9995-9999)
🪛 PHPMD (2.15.0)
tests/Backend/Feature/api/v1/OIDCTest.php
734-734: Avoid unused local variables such as '$response'. (Unused Code Rules)
(UnusedLocalVariable)
🪛 Gitleaks (8.26.0)
tests/Backend/Unit/OpenIDConnectClientTest.php
461-461: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 LanguageTool
docs/docs/administration/08-advanced/01-external-authentication.md
[grammar] ~14-~14: Use a comma after introductory words or phrases
Context: ...practical to manage all users in PILOS. Therefore PILOS can be connected to external auth...
(QB_NEW_EN_OTHER_ERROR_IDS_19)
[grammar] ~14-~14: Add a comma
Context: ... external authentication systems. LDAP, Shibboleth and OpenID-Connect are available as int...
(QB_NEW_EN_OTHER_ERROR_IDS_22)
[grammar] ~14-~14: Use hyphens correctly
Context: ...ntication systems. LDAP, Shibboleth and OpenID-Connect are available as interfaces. All three ...
(QB_NEW_EN_OTHER_ERROR_IDS_29)
[grammar] ~14-~14: Use correct spacing
Context: ...in parallel, but none of them more than once. ## Setup of external authenticators ### L...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~129-~129: Use correct spacing
Context: ...ION_CHECK_ENABLED=false ``` ### OpenID Connect To enable OpenID Connect, you need to a...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~131-~131: Use correct spacing
Context: ...s in the .env file and adjust to your needs. The required openid scope is always p...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~133-~133: Use hyphens correctly
Context: ... all required attributes, add them as a comma separated list. The default value for `O...
(QB_NEW_EN_OTHER_ERROR_IDS_29)
[grammar] ~133-~133: Use correct spacing
Context: ... The default value for OIDC_SCOPES is profile,email. bash # OpenID Connect config OIDC_ENABLED=true OIDC_ISSUER=http://idp.university.org OIDC_CLIENT_ID=my_client_id OIDC_CLIENT_SECRET=my_client_secret OIDC_SCOPES="profile,email" In your IDP you should configure the fo...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~144-~144: Use correct spacing
Context: ...` In your IDP you should configure the following: - Redirect URI: https://your-domain.com/a...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~148-~148: Use correct spacing
Context: ...n.com/logout - Back-Channel Logout URI: https://your-domain.com/auth/oidc/logout #### Configuration Options | Option ...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~150-~150: Use correct spacing
Context: ...om/auth/oidc/logout #### Configuration Options | Option | Default...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~154-~154: Use correct spacing
Context: ...| Enable OpenID Connect authentication. | | OIDC_ISSUER | null ...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~155-~155: Use correct spacing
Context: ...e issuer URL of the OpenID Connect IDP. | | OIDC_CLIENT_ID | null ...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~156-~156: Use correct spacing
Context: ... ID for the OpenID Connect application. | | OIDC_CLIENT_SECRET | null ...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~157-~157: Use correct spacing
Context: ...ret for the OpenID Connect application. | | OIDC_SCOPES | `profile,...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~158-~158: Use correct spacing
Context: ...to request from the OpenID Connect IDP. | | OIDC_LEEWAY | 300 ...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~159-~159: Use correct spacing
Context: ...idating the ID token's expiration time. | | OIDC_TIMEOUT | 10 ...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~160-~160: Use correct spacing
Context: ...econds for the OpenID Connect requests. | | OIDC_VERIFY_PEER | true ...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~161-~161: There might be a problem here.
Context: ...nnect IDP. This can only be disabled in local env. | | OIDC_CACHE_CONFIG_MAX_AGE | 0 ...
(QB_NEW_EN_MERGED_MATCH)
[grammar] ~163-~163: Use correct spacing
Context: ...duration > 0 | ## Configure mapping For each external au...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~165-~165: There might be a mistake here.
Context: ... | ## Configure mapping For each external authenticator the att...
(QB_NEW_EN_OTHER)
[grammar] ~167-~167: Add a comma
Context: ...## Configure mapping For each external authenticator the attribute and role mapping needs to...
(QB_NEW_EN_OTHER_ERROR_IDS_22)
[grammar] ~168-~168: Use correct spacing
Context: ...irectory app/Auth/config of the pilos installation. | Authenticator | Filename ...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~174-~174: Use correct spacing
Context: ...penID Connect | oidc_mapping.json | ### Attribute mapping #### Required attrib...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~263-~263: Use a comma after introductory words or phrases
Context: ...s #### LDAP ##### Attributes In this example the LDAP schema uses the common name (C...
(QB_NEW_EN_OTHER_ERROR_IDS_19)
[grammar] ~263-~263: Use articles correctly
Context: ...DAP schema uses the common name (CN) as username and has the group memberships in the me...
(QB_NEW_EN_OTHER_ERROR_IDS_11)
[grammar] ~263-~263: There might be a problem here.
Context: ...s the group memberships in the memberof attribute. ##### Roles - The "superuser" role is assigned to any...
(QB_NEW_EN_MERGED_MATCH)
[grammar] ~314-~314: Use the right verb tense
Context: ...names in which the attribute values are send by the apache mod_shib to the applicati...
(QB_NEW_EN_OTHER_ERROR_IDS_13)
[grammar] ~314-~314: There might be a problem here.
Context: ... are send by the apache mod_shib to the application. ##### Roles - The "superuser" role is assigned to any...
(QB_NEW_EN_MERGED_MATCH)
[grammar] ~361-~361: There might be a problem here.
Context: ... ] } ] } ``` #### OpenID Connect ##### Attributes In this example the OpenID Connect prov...
(QB_NEW_EN_MERGED_MATCH)
[grammar] ~365-~365: Use a comma after introductory words or phrases
Context: ...enID Connect ##### Attributes In this example the OpenID Connect provider returns the...
(QB_NEW_EN_OTHER_ERROR_IDS_19)
[grammar] ~365-~365: Correctly pair commas and coordinating conjunctions
Context: ...preferred_username which contains the username and an additional claim roles with an...
(QB_NEW_EN_OTHER_ERROR_IDS_14)
[grammar] ~365-~365: There might be a problem here.
Context: ...ditional claim roles with an array of roles. ##### Roles - The "superuser" role is assigned to any...
(QB_NEW_EN_MERGED_MATCH)
[grammar] ~369-~369: Use correct spacing
Context: ... any user who has the "pilos-superuser" role. json { "attributes": { "external_id": "preferred_username", "first_name": "given_name", "last_name": "family_name", "email": "email", "roles": "roles" }, "roles": [ { "name": "superuser", "disabled": false, "rules": [ { "attribute": "roles", "regex": "/^pilos-superuser$/im" } ] } ] }
(QB_NEW_EN_OTHER_ERROR_IDS_5)
🪛 markdownlint-cli2 (0.17.2)
docs/docs/administration/08-advanced/01-external-authentication.md
146-146: Bare URL used
(MD034, no-bare-urls)
147-147: Bare URL used
(MD034, no-bare-urls)
148-148: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Docker Build
- GitHub Check: Backend
🔇 Additional comments (25)
docs/docs/administration/08-advanced/01-external-authentication.md (1)
129-393: Excellent comprehensive documentation for OIDC integration.The OpenID Connect documentation section is well-structured and provides:
- Clear configuration instructions with environment variables
- Comprehensive configuration options table
- Practical examples for attribute and role mapping
- Consistent formatting with existing LDAP and Shibboleth sections
This documentation will be valuable for administrators configuring OIDC authentication.
app/Auth/OIDC/OIDCController.php (4)
13-16: Clean constructor with proper dependency injection.The constructor properly injects the
OIDCProviderdependency and applies the guest middleware, which is appropriate for authentication flows.
21-34: Good exception handling in redirect method.The method properly handles both specific network exceptions and general exceptions, with appropriate logging and user-friendly error redirects.
39-77: Well-structured callback method with comprehensive exception handling.The method properly handles various OIDC-specific exceptions and general errors, with appropriate logging and user-friendly redirects. The user login flow and timestamp updates are handled correctly.
82-85: Simple and appropriate logout delegation.The logout method correctly delegates back-channel logout handling to the provider, which is the appropriate separation of concerns.
app/Auth/OIDC/OIDCProvider.php (2)
74-108: Comprehensive login method with proper user handling.The login method properly:
- Authenticates with the OIDC client
- Retrieves and validates claims
- Creates/finds user and syncs attributes
- Handles session data including JTI and SID for logout
- Stores ID token appropriately
The implementation is thorough and follows OIDC best practices.
110-156: Excellent back-channel logout implementation.The back-channel logout method properly:
- Verifies the logout token
- Implements JTI replay protection using cache
- Handles both SID and SUB-based session termination
- Returns appropriate HTTP responses with no-store headers
- Provides comprehensive error handling
This implementation follows OIDC back-channel logout specifications correctly.
tests/Backend/Unit/OpenIDConnectClientTest.php (6)
34-55: Well-structured test setup with proper mocking.The test setup properly:
- Prevents stray HTTP requests
- Mocks the OpenID Connect discovery endpoint
- Provides a realistic discovery configuration
- Sets up a clean test environment
79-120: Excellent helper methods for JWT testing.The
createJWSandsignClaimshelper methods provide:
- Support for all major JWT algorithms (RSA, ECDSA, EdDSA, HMAC)
- Proper algorithm manager configuration
- Flexible header and payload handling
- Clean serialization to compact format
These helpers enable comprehensive cryptographic testing.
121-426: Comprehensive JWT signature verification tests.The signature verification tests provide excellent coverage:
- RSASSA algorithms: RS256, RS384, RS512 with proper key ID handling
- RSASSA-PSS algorithms: PS256, PS384, PS512 with padding verification
- ECDSA algorithms: ES256, ES384, ES512 with elliptic curve validation
- EdDSA algorithm: Ed25519 with Edwards curve signatures
- HMAC algorithms: HS256, HS384, HS512 with symmetric key validation
Each test verifies both successful verification and proper error handling for invalid signatures and wrong key IDs.
428-608: Thorough ID token claims validation testing.The ID token claims tests cover essential validation scenarios:
- Audience validation: Single and multiple audience claims
- Time-based claims: Issued-at and expiration time validation with leeway
- Nonce validation: Proper nonce matching and mismatch handling
- Access token hash: Correct
at_hashvalidation- Mandatory claims: Proper handling of missing required claims
The data provider approach ensures comprehensive edge case coverage.
610-856: Excellent logout token claims validation testing.The logout token claims tests properly verify:
- Session identification: SID and SUB claim requirements
- Event validation: Back-channel logout event presence
- Prohibited claims: Proper rejection of nonce in logout tokens
- JTI validation: Unique token identifier requirements
- Time validation: Proper issued-at and expiration handling
This ensures robust back-channel logout implementation.
461-461: False positive: Test token from OpenID Connect specification.The static analysis tool flagged this as a potential API key, but this is actually a legitimate test token from the OpenID Connect Core 1.0 specification examples (section 3.1.3.7). It's safe to use in tests as it's publicly documented and intended for testing purposes.
app/Auth/OIDC/OpenIDConnectClient.php (4)
633-634: Good security practice: Self-signed JWK rejection.The implementation correctly rejects self-signed JWK headers, which is a security best practice to prevent malicious token injection.
425-432: Excellent logout token validation.The logout token validation correctly implements the OIDC back-channel logout specification by:
- Ensuring no nonce claim is present
- Requiring either sid or sub claims
- Proper error handling with specific exceptions
167-181: Comprehensive algorithm support.The implementation supports a wide range of JWT signature algorithms (RS, PS, ES, HS, EdDSA) which provides good compatibility with different OIDC providers.
477-498: Cache key derivation is safe – resolving comment.The well-known configuration and JWKS cache keys are built from internal, server-managed URLs (provider URL and JWKS URI) and never from untrusted user input, so there’s no risk of cache poisoning. Other cache uses (locale, JTI, Shibboleth session) similarly employ non-user-controlled keys.
tests/Backend/Feature/api/v1/OIDCTest.php (8)
1-40: Well-structured test class with comprehensive imports.The class properly extends TestCase and includes all necessary imports for OIDC testing, including comprehensive JWT signature algorithm support. The use of RefreshDatabase and WithFaker traits is appropriate for feature testing.
42-128: Comprehensive test configuration setup.The OIDC provider configuration and user attribute mapping are well-structured and realistic. The setUp method properly initializes the test environment with appropriate configuration values and creates necessary role fixtures.
130-176: Correctly implemented JWT utility methods.The JWT creation and signing methods properly support multiple signature algorithms and follow JWT standards. The base64url_encode implementation correctly handles URL-safe Base64 encoding as required by the JWT specification.
178-257: Comprehensive redirect route testing.The tests effectively cover the authorization redirect flow including disabled state, network errors, and successful redirects. The URL parsing and parameter validation ensure proper OAuth2 compliance.
872-1041: Excellent security testing coverage.The tests for token substitution attacks (wrong sub claims) and issuer validation demonstrate strong security awareness. These tests are crucial for preventing malicious token reuse and ensuring proper OIDC compliance.
1043-1563: Comprehensive error handling test coverage.The tests effectively cover various error scenarios including network failures, invalid tokens, and missing parameters. The verification of proper error redirects and guest state maintenance ensures robust error handling.
1565-1766: Thorough logout functionality testing.The RP-initiated logout tests properly verify logout URL construction, parameter passing, and the complete logout flow. The coverage of both enabled and disabled states ensures robust configuration handling.
1768-2253: Sophisticated back-channel logout testing.The back-channel logout tests demonstrate excellent understanding of the specification, including proper JTI replay protection, nonce validation, and different logout token scenarios. The comprehensive coverage of error cases and security constraints is particularly valuable.
| class OpenIDConnectClient | ||
| { | ||
| /** | ||
| * @var array holds the provider configuration | ||
| */ | ||
| private array $providerConfig = []; | ||
|
|
||
| /** | ||
| * @var bool Verify SSL peer on transactions | ||
| */ | ||
| private bool $verifyPeer = true; | ||
|
|
||
| /** | ||
| * @var string if we acquire an access token it will be stored here | ||
| */ | ||
| protected string $accessToken; | ||
|
|
||
| /** | ||
| * @var JWS if we acquire an id token it will be stored here | ||
| */ | ||
| protected JWS $idToken; | ||
|
|
||
| /** | ||
| * @var object stores the token response | ||
| */ | ||
| private object $tokenResponse; | ||
|
|
||
| /** | ||
| * @var array holds scopes | ||
| */ | ||
| private array $scopes = ['openid']; | ||
|
|
||
| /** | ||
| * @var mixed holds well-known openid server properties | ||
| */ | ||
| private mixed $wellKnown = false; | ||
|
|
||
| /** | ||
| * @var int timeout (seconds) | ||
| */ | ||
| protected int $timeOut = 60; | ||
|
|
||
| /** | ||
| * @var int leeway (seconds) | ||
| */ | ||
| private int $leeway = 300; | ||
|
|
||
| /** | ||
| * @var int fallback cache max age (seconds) for openid configuration | ||
| */ | ||
| private int $cacheConfigMaxAge = 0; | ||
|
|
||
| /** | ||
| * @var int fallback cache max age (seconds) for jwks | ||
| */ | ||
| private int $cacheJwksMaxAge = 0; | ||
|
|
||
| /** | ||
| * @var object holds verified jwt claims | ||
| */ | ||
| protected object $verifiedClaims; | ||
|
|
||
| /** | ||
| * @var string if we acquire a sid in back-channel logout it will be stored here | ||
| */ | ||
| private ?string $backChannelSid = null; | ||
|
|
||
| /** | ||
| * @var string if we acquire a sub in back-channel logout it will be stored here | ||
| */ | ||
| private ?string $backChannelSubject = null; | ||
|
|
||
| /** | ||
| * @var string jti (JWT ID) of back-channel logout it will be stored here | ||
| */ | ||
| private string $backChannelJti; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider breaking down the monolithic class.
This class has multiple responsibilities (authentication, token validation, session management, HTTP client management, etc.) which violates the Single Responsibility Principle. Consider breaking it into smaller, focused classes.
Consider creating separate classes for:
- JWT signature verification
- Session management
- HTTP client operations
- Cache management
- Token validation
This would improve maintainability, testability, and adherence to SOLID principles.
🤖 Prompt for AI Agents
In app/Auth/OIDC/OpenIDConnectClient.php between lines 76 and 151, the
OpenIDConnectClient class handles multiple responsibilities such as
authentication, token validation, session management, HTTP client operations,
and caching, violating the Single Responsibility Principle. Refactor by
extracting these concerns into separate classes: create a dedicated class for
JWT signature verification, another for session management, one for HTTP client
operations, one for cache management, and one for token validation. Then, update
OpenIDConnectClient to delegate these responsibilities to the new classes,
improving maintainability and adherence to SOLID principles.
| */ | ||
| public function getJWSClaims(JWS $jws): object | ||
| { | ||
| return json_decode($jws->getPayload()); |
There was a problem hiding this comment.
Add error handling for JSON decoding.
The json_decode() call lacks error handling, which could cause issues if the JWT payload is malformed.
Apply this diff to add proper error handling:
- public function getJWSClaims(JWS $jws): object
- {
- return json_decode($jws->getPayload());
- }
+ public function getJWSClaims(JWS $jws): object
+ {
+ try {
+ return json_decode($jws->getPayload(), flags: JSON_THROW_ON_ERROR);
+ } catch (JsonException $e) {
+ throw new OpenIDConnectValidationException('Invalid JWT payload: ' . $e->getMessage(), $e->getCode(), $e);
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return json_decode($jws->getPayload()); | |
| public function getJWSClaims(JWS $jws): object | |
| { | |
| try { | |
| return json_decode($jws->getPayload(), flags: JSON_THROW_ON_ERROR); | |
| } catch (JsonException $e) { | |
| throw new OpenIDConnectValidationException( | |
| 'Invalid JWT payload: ' . $e->getMessage(), | |
| $e->getCode(), | |
| $e | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/Auth/OIDC/OpenIDConnectClient.php at line 599, the json_decode call on
the JWT payload lacks error handling, which can cause issues if the payload is
malformed. Modify the code to check if json_decode returns null or an error, and
handle this case appropriately by either throwing an exception or returning a
meaningful error response to prevent silent failures.
| ] | ||
| ); | ||
| $public_key = $private_key->toPublic(); | ||
|
|
||
| // Generate random values for the ID token | ||
| $kid = Str::random(); | ||
| $code = Str::random(); | ||
| $nonce = Str::random(); | ||
| $state = Str::random(); | ||
| $firstName = $this->faker->firstName(); | ||
| $lastName = $this->faker->lastName(); | ||
| $email = $this->faker->email(); | ||
| $sub = 'johnd'; | ||
| $sid = $this->faker->uuid(); | ||
|
|
||
| // Create claims for the ID token | ||
| $claims = [ | ||
| 'exp' => time() + 60, | ||
| 'iat' => time(), | ||
| 'iss' => 'https://example.org', | ||
| 'aud' => 'fake-client-id', | ||
| 'at_hash' => $this->base64url_encode(substr(hash('sha256', 'fake-access-token', true), 0, 16)), | ||
| 'sub' => $sub, | ||
| 'sid' => $sid, | ||
| 'given_name' => $firstName, | ||
| 'family_name' => $lastName, | ||
| 'email' => $email, | ||
| 'nonce' => $nonce, | ||
| ]; | ||
|
|
||
| // Create id token | ||
| $idToken = $this->signClaims($claims, $private_key, 'RS256', ['kid' => $kid]); | ||
|
|
||
| $userInfoClaims = [ | ||
| 'iss' => 'https://example.org', | ||
| 'aud' => 'fake-client-id', | ||
| 'sub' => $sub, | ||
| 'given_name' => $firstName, | ||
| 'family_name' => $lastName, | ||
| 'email' => $email, | ||
| 'groups' => ['student', 'staff'], | ||
| ]; | ||
|
|
||
| // Sign user info claims | ||
| $userInfoResponse = $this->signClaims($userInfoClaims, $private_key, 'RS256', ['kid' => $kid]); | ||
|
|
||
| // List of JWKs to be returned by the JWKS endpoint | ||
| $jwks = [[ | ||
| 'kid' => $kid, | ||
| ...$public_key->jsonSerialize(), | ||
| ]]; | ||
|
|
||
| $tokenResponse = [ | ||
| 'access_token' => 'fake-access-token', | ||
| 'token_type' => 'Bearer', | ||
| 'id_token' => $idToken, | ||
| ]; | ||
|
|
||
| Http::fake([ | ||
| 'https://example.org/.well-known/openid-configuration' => Http::response($this->discovery), | ||
| 'https://example.org/jwks' => Http::response([ | ||
| 'keys' => $jwks, | ||
| ]), | ||
| 'https://example.org/token' => Http::response($tokenResponse), | ||
| 'https://example.org/userinfo' => Http::response($userInfoResponse, 200, [ | ||
| 'Content-Type' => 'application/jwt', | ||
| ]), | ||
| ]); | ||
|
|
||
| // Simulate the state and nonce have been set in the session | ||
| Session::put('openid_connect_state', $state); | ||
| Session::put('openid_connect_nonce', $nonce); | ||
|
|
||
| $header = [ | ||
| 'Accept-Language' => 'fr', | ||
| ]; | ||
|
|
||
| $response = $this->get(route('auth.oidc.callback', [ | ||
| 'code' => $code, | ||
| 'state' => $state, | ||
| ]), $header); | ||
|
|
||
| $this->assertAuthenticated(); | ||
|
|
||
| $user = Auth::user(); | ||
|
|
||
| // Check if user attributes have been overwritten | ||
| $this->assertEquals('oidc', $user->authenticator); | ||
| $this->assertEquals($sub, $user->external_id); | ||
| $this->assertEquals($firstName, $user->firstname); | ||
| $this->assertEquals($lastName, $user->lastname); | ||
| $this->assertEquals($email, $user->email); | ||
|
|
||
| // Check if default attributes (not provided by oidc) have not been overwritten | ||
| $this->assertEquals('de', $user->locale); | ||
| $this->assertEquals('Europe/Berlin', $user->timezone); | ||
|
|
||
| // Check if automatic roles have been overwritten, but not the manually assigned role | ||
| $this->assertEquals($user->roles()->pluck('name')->toArray(), ['admin', 'user']); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract common test setup to reduce code duplication.
The callback test methods contain significant code duplication in JWT creation, key generation, and HTTP mocking setup. This pattern repeats across multiple test methods, making the code harder to maintain and understand.
Consider extracting common setup into helper methods:
private function createTestJwtKeys(): array
{
$private_key = JWKFactory::createRSAKey(2048, ['alg' => 'RS256', 'use' => 'sig']);
$public_key = $private_key->toPublic();
$kid = Str::random();
return [$private_key, $public_key, $kid];
}
private function createStandardClaims(string $sub, string $sid, string $nonce): array
{
return [
'exp' => time() + 60,
'iat' => time(),
'iss' => 'https://example.org',
'aud' => 'fake-client-id',
'sub' => $sub,
'sid' => $sid,
'nonce' => $nonce,
];
}
private function setupHttpMocking($jwks, $tokenResponse, $userInfoResponse = null): void
{
$fakes = [
'https://example.org/.well-known/openid-configuration' => Http::response($this->discovery),
'https://example.org/jwks' => Http::response(['keys' => $jwks]),
'https://example.org/token' => Http::response($tokenResponse),
];
if ($userInfoResponse) {
$fakes['https://example.org/userinfo'] = Http::response($userInfoResponse);
}
Http::fake($fakes);
}This would reduce each test method from 50-100+ lines to 10-20 lines and improve maintainability.
🧰 Tools
🪛 PHPMD (2.15.0)
734-734: Avoid unused local variables such as '$response'. (Unused Code Rules)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
In tests/Backend/Feature/api/v1/OIDCTest.php between lines 294 and 756, there is
significant duplication of code for JWT key creation, claims setup, and HTTP
mocking across multiple test methods. To fix this, extract the repeated logic
into private helper methods such as createTestJwtKeys for key generation,
createStandardClaims for claims creation, and setupHttpMocking for HTTP response
fakes. Then refactor each test method to call these helpers, reducing code
duplication and improving readability and maintainability.
| $response = $this->get(route('auth.oidc.callback', [ | ||
| 'code' => $code, | ||
| 'state' => $state, | ||
| ]), $header); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address unused response variable.
The $response variable is assigned but never used in assertions, as flagged by static analysis. Either add assertions to verify the HTTP response or remove the variable assignment.
Add response verification:
$response = $this->get(route('auth.oidc.callback', [
'code' => $code,
'state' => $state,
]), $header);
$response->assertRedirect('http://localhost/external_login');Or if the response verification isn't needed, use the method without storing the result:
$this->get(route('auth.oidc.callback', [
'code' => $code,
'state' => $state,
]), $header);🧰 Tools
🪛 PHPMD (2.15.0)
734-734: Avoid unused local variables such as '$response'. (Unused Code Rules)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
In tests/Backend/Feature/api/v1/OIDCTest.php around lines 734 to 737, the
$response variable is assigned the result of the get() call but never used,
causing an unused variable warning. To fix this, either add assertions on
$response to verify the HTTP response, such as asserting a redirect to the
expected URL, or remove the assignment and call get() without storing the result
if no verification is needed.
| 'sub' => $sub, | ||
| 'sid' => $sid, | ||
| 'given_name' => $firstName, | ||
| 'family_name' => $lastName, | ||
| 'email' => $email, | ||
| 'nonce' => $nonce, | ||
| ]; | ||
|
|
||
| // Create id token | ||
| $idToken = $this->signClaims($claims, $private_key, 'RS256', ['kid' => $kid]); | ||
|
|
||
| $userInfoClaims = [ | ||
| 'iss' => 'https://example.org', | ||
| 'aud' => 'fake-client-id', | ||
| 'sub' => $sub, | ||
| 'given_name' => $firstName, | ||
| 'family_name' => $lastName, | ||
| 'email' => $email, | ||
| 'groups' => ['student', 'staff'], | ||
| ]; | ||
|
|
||
| // List of JWKs to be returned by the JWKS endpoint | ||
| $jwks = [[ | ||
| 'kid' => $kid, | ||
| ...$public_key->jsonSerialize(), | ||
| ]]; | ||
|
|
||
| $tokenResponse = [ | ||
| 'access_token' => 'fake-access-token', | ||
| 'token_type' => 'Bearer', | ||
| 'id_token' => $idToken, | ||
| ]; | ||
|
|
||
| $discovery = $this->discovery; | ||
| $discovery['end_session_endpoint'] = 'https://example.org/logout'; | ||
|
|
||
| Http::fake([ | ||
| 'https://example.org/.well-known/openid-configuration' => Http::response($discovery), | ||
| 'https://example.org/jwks' => Http::response([ | ||
| 'keys' => $jwks, | ||
| ]), | ||
| 'https://example.org/token' => Http::response($tokenResponse), | ||
| 'https://example.org/userinfo' => Http::response($userInfoClaims), | ||
| ]); | ||
|
|
||
| // Simulate the state and nonce have been set in the session | ||
| Session::put('openid_connect_state', $state); | ||
| Session::put('openid_connect_nonce', $nonce); | ||
|
|
||
| $header = [ | ||
| 'Accept-Language' => 'fr', | ||
| ]; | ||
|
|
||
| // Check no session exists yet | ||
| $this->assertCount(0, \App\Models\Session::all()); | ||
|
|
||
| $response = $this->get(route('auth.oidc.callback', [ | ||
| 'code' => $code, | ||
| 'state' => $state, | ||
| ]), $header); | ||
| $response = $this->withCookies([session()->getName() => \App\Models\Session::first()->id])->get($response->getTargetUrl(), $header); | ||
| $this->assertAuthenticated(); | ||
|
|
||
| Auth::logout(); | ||
|
|
||
| // Check session still exists | ||
| $this->assertCount(1, \App\Models\Session::all()); | ||
|
|
||
| // Create logout token | ||
| $logoutToken = $this->signClaims([ | ||
| 'iss' => 'https://example.org', | ||
| 'aud' => 'fake-client-id', | ||
| 'iat' => time(), | ||
| 'exp' => time() + 60, | ||
| 'jti' => Str::random(), | ||
| 'events' => json_decode('{"http://schemas.openid.net/event/backchannel-logout": {}}'), | ||
| ], $private_key, 'RS256', ['kid' => $kid]); | ||
|
|
||
| // Simulate back-channel logout | ||
| $this->post(route('auth.oidc.logout'), ['logout_token' => $logoutToken]) | ||
| ->assertStatus(400); | ||
|
|
||
| // Check if session is not terminated | ||
| $this->assertCount(1, \App\Models\Session::all()); | ||
| } | ||
|
|
||
| public function test_back_channel_logout_without_logout_token() | ||
| { | ||
| // Simulate back-channel logout | ||
| $this->post(route('auth.oidc.logout')) | ||
| ->assertStatus(400); | ||
| } | ||
|
|
||
| public function test_back_channel_logout_disabled() | ||
| { | ||
| Config::set('services.oidc.enabled', false); | ||
| $this->post(route('auth.oidc.logout')) | ||
| ->assertNotFound(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Comprehensive OIDC test suite with excellent security coverage.
This test file provides exceptional coverage of OIDC authentication flows, including sophisticated security testing for token substitution attacks, proper error handling, and complete logout flows. The attention to security details and edge cases demonstrates strong understanding of OIDC vulnerabilities.
However, the significant code duplication across test methods impacts maintainability. Consider refactoring common setup patterns into helper methods to improve code organization and reduce maintenance overhead.
🧰 Tools
🪛 PHPMD (2.15.0)
734-734: Avoid unused local variables such as '$response'. (Unused Code Rules)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
In tests/Backend/Feature/api/v1/OIDCTest.php spanning lines 1 to 2253, the test
suite contains extensive duplicated code across many test methods, especially in
the setup of RSA keys, claims, tokens, and HTTP fakes. To fix this, refactor by
extracting these repeated setup steps into reusable helper methods or setup
functions within the test class. This will improve maintainability by
centralizing common logic, reducing redundancy, and making the tests easier to
update and understand.
|
Superseded by #2281, to make PR more readable |
Fixes #300
Type (Highlight the corresponding type)
Checklist
Changes
Other information
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores