fix(oidc): delayed user details refresh #7035
Conversation
ArtifactsThese changes are published for testing on Buildkite, DockerHub and GitHub Container Registry. Docker Container
|
WalkthroughThe updates focus on enhancing OIDC functionality and session validation in an authentication framework. Changes include adding methods to user details structures, refining OIDC claim handling, improving session validation logic, and updating access control regex pattern matching behavior. These modifications aim to streamline authentication and authorization processes for a more robust system. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
james-d-elliott
force-pushed
the
fix-oidc-details-refresh
branch
from
51ce803
to
81686af
Compare
✅ Deploy Preview for authelia-staging ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
james-d-elliott
force-pushed
the
fix-oidc-details-refresh
branch
2 times, most recently
from
200314d
to
47119a4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- internal/handlers/handler_authz_authn.go (5 hunks)
- internal/handlers/handler_oidc_authorization.go (1 hunks)
- internal/handlers/handler_oidc_userinfo.go (3 hunks)
- internal/handlers/oidc.go (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- internal/handlers/handler_authz_authn.go
- internal/handlers/handler_oidc_authorization.go
- internal/handlers/handler_oidc_userinfo.go
- internal/handlers/oidc.go
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7035 +/- ##
==========================================
- Coverage 73.36% 73.36% -0.01%
==========================================
Files 345 345
Lines 29798 29907 +109
Branches 839 839
==========================================
+ Hits 21862 21940 +78
- Misses 7065 7095 +30
- Partials 871 872 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
james-d-elliott
force-pushed
the
fix-oidc-details-refresh
branch
2 times, most recently
from
47119a4
to
44b5013
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (9)
- docs/content/configuration/security/access-control.md (1 hunks)
- internal/authentication/types.go (1 hunks)
- internal/handlers/handler_authz_authn.go (5 hunks)
- internal/handlers/handler_oidc_authorization.go (1 hunks)
- internal/handlers/handler_oidc_userinfo.go (3 hunks)
- internal/handlers/oidc.go (1 hunks)
- internal/oidc/const.go (1 hunks)
- internal/oidc/types.go (1 hunks)
- internal/session/user_session.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- internal/handlers/handler_authz_authn.go
- internal/handlers/handler_oidc_authorization.go
- internal/handlers/handler_oidc_userinfo.go
Additional comments (23)
internal/session/user_session.go (1)
- 94-108: The newly added methods
GetUsername,GetGroups,GetDisplayName, andGetEmailsin theUserSessionstruct are straightforward and follow good practices by providing a clean API for accessing user session details. These methods enhance the readability and maintainability of the code by encapsulating the access to specific fields of theUserSessionstruct.internal/authentication/types.go (1)
- 80-94: The newly added methods
GetUsername,GetGroups,GetDisplayName, andGetEmailsin theUserDetailsstruct are well-implemented and follow good practices by providing a clean API for accessing user details. These methods enhance the readability and maintainability of the code by encapsulating the access to specific fields of theUserDetailsstruct.internal/handlers/oidc.go (1)
- 161-161: Please ensure that the cuddling of the if statement with the assignment is corrected to adhere to Go coding standards. This was previously flagged by static analysis and should be addressed for consistency and readability.
internal/oidc/const.go (1)
- 25-25: The addition of the
ClaimStateHashconstant is a good practice for managing OIDC claims. It enhances the clarity and maintainability of the code by providing a named constant for the state hash claim, which is likely to be used in multiple places within the OIDC flow.docs/content/configuration/security/access-control.md (18)
- 420-425: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-591]
The document thoroughly explains the configuration options for access control in Authelia, including the significant change in regex pattern matching behavior for resource rules in version 4.27.0. However, it's essential to ensure that this change is highlighted appropriately within the document to alert users of the behavior change and guide them on how to adjust their configurations if necessary. Consider adding a specific section or note that directly addresses this change, providing examples of how resource rules should be written post 4.27.0 to include query parameters in the match. This will help users understand the impact of the change and how to adapt their configurations accordingly.
- 420-425: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-591]
Overall, the document is well-structured and provides comprehensive information on configuring access control in Authelia. The explanations are clear, and the examples are helpful in understanding how to apply the configurations. It's important to ensure that all configuration options and policies are up-to-date and accurately reflect the current capabilities of Authelia. Additionally, consider reviewing the document for consistency in formatting and terminology to enhance readability and user comprehension.
- 420-425: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [409-411]
The
resourcessection explains how to match the path and query of the request using regular expressions. Given the change in regex pattern matching behavior for resource rules in version 4.27.0, it's crucial to ensure that this section includes clear guidance on how to include query parameters in the match. Consider adding examples that demonstrate the new behavior and explicitly state the version from which this behavior applies. This will help users understand how to write resource rules that take advantage of the updated matching behavior.
- 420-425: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [71-71]
Consider starting the sentence with an uppercase letter for consistency and readability.
- 420-425: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [82-82]
Consider starting the sentence with an uppercase letter for consistency and readability.
- 420-425: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [97-97]
Consider starting the sentence with an uppercase letter for consistency and readability.
- 420-425: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [124-124]
Consider starting the sentence with an uppercase letter for consistency and readability.
- 420-425: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [195-195]
Consider starting the sentence with an uppercase letter for consistency and readability.
- 420-425: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [241-241]
Consider starting the sentence with an uppercase letter for consistency and readability.
- 420-425: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [250-250]
Consider starting the sentence with an uppercase letter for consistency and readability.
- 420-425: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [316-316]
Consider starting the sentence with an uppercase letter for consistency and readability.
- 420-425: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [354-354]
Consider starting the sentence with an uppercase letter for consistency and readability.
- 420-425: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [409-409]
Consider starting the sentence with an uppercase letter for consistency and readability.
- 420-425: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [443-443]
Consider starting the sentence with an uppercase letter for consistency and readability.
- 420-425: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [505-505]
Consider starting the sentence with an uppercase letter for consistency and readability.
- 420-425: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [513-513]
Consider starting the sentence with an uppercase letter for consistency and readability.
- 420-425: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [521-521]
Consider starting the sentence with an uppercase letter for consistency and readability.
- 420-425: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [528-528]
Consider starting the sentence with an uppercase letter for consistency and readability.
internal/oidc/types.go (1)
- 202-207: The addition of the
UserDetailerinterface aligns well with the PR's objectives to enhance the refresh mechanism of user details in OIDC flows. It's crucial to ensure that this interface is implemented effectively and utilized in the OIDC flow enhancements to achieve the intended improvements in user detail refreshment.
james-d-elliott
force-pushed
the
fix-oidc-details-refresh
branch
from
44b5013
to
e420b53
Compare
There was a problem hiding this comment.
Actionable comments posted: 15
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (9)
- docs/content/configuration/security/access-control.md (1 hunks)
- internal/authentication/types.go (1 hunks)
- internal/handlers/handler_authz_authn.go (5 hunks)
- internal/handlers/handler_oidc_authorization.go (1 hunks)
- internal/handlers/handler_oidc_userinfo.go (3 hunks)
- internal/handlers/oidc.go (1 hunks)
- internal/oidc/const.go (1 hunks)
- internal/oidc/types.go (1 hunks)
- internal/session/user_session.go (1 hunks)
Files skipped from review as they are similar to previous changes (8)
- internal/authentication/types.go
- internal/handlers/handler_authz_authn.go
- internal/handlers/handler_oidc_authorization.go
- internal/handlers/handler_oidc_userinfo.go
- internal/handlers/oidc.go
- internal/oidc/const.go
- internal/oidc/types.go
- internal/session/user_session.go
Additional comments not posted (1)
docs/content/configuration/security/access-control.md (1)
420-425: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [77-77]
The term "Authelia" is spelled correctly; however, the static analysis tool flagged it as a possible spelling mistake. This is a false positive.
james-d-elliott
force-pushed
the
fix-oidc-details-refresh
branch
from
e420b53
to
6ef53af
Compare
There was a problem hiding this comment.
Actionable comments posted: 15
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (9)
- docs/content/configuration/security/access-control.md (1 hunks)
- internal/authentication/types.go (1 hunks)
- internal/handlers/handler_authz_authn.go (5 hunks)
- internal/handlers/handler_oidc_authorization.go (1 hunks)
- internal/handlers/handler_oidc_userinfo.go (3 hunks)
- internal/handlers/oidc.go (1 hunks)
- internal/oidc/const.go (1 hunks)
- internal/oidc/types.go (1 hunks)
- internal/session/user_session.go (1 hunks)
Files skipped from review as they are similar to previous changes (8)
- internal/authentication/types.go
- internal/handlers/handler_authz_authn.go
- internal/handlers/handler_oidc_authorization.go
- internal/handlers/handler_oidc_userinfo.go
- internal/handlers/oidc.go
- internal/oidc/const.go
- internal/oidc/types.go
- internal/session/user_session.go
The user details refresh does not naturally occur via OpenID Connect 1.0 flows and instead relies on alternative activity. This helps ensure the details are more frequently updated via normal OAuth 2.0 flows. Signed-off-by: James Elliott <james-d-elliott@users.noreply.github.com>
james-d-elliott
force-pushed
the
fix-oidc-details-refresh
branch
from
6ef53af
to
c6e7c1e
Compare
There was a problem hiding this comment.
Actionable comments posted: 15
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (10)
- docs/content/configuration/security/access-control.md (1 hunks)
- internal/authentication/types.go (1 hunks)
- internal/handlers/handler_authz_authn.go (5 hunks)
- internal/handlers/handler_oidc_authorization.go (1 hunks)
- internal/handlers/handler_oidc_userinfo.go (3 hunks)
- internal/handlers/oidc.go (1 hunks)
- internal/handlers/oidc_test.go (2 hunks)
- internal/oidc/const.go (1 hunks)
- internal/oidc/types.go (1 hunks)
- internal/session/user_session.go (1 hunks)
Files skipped from review as they are similar to previous changes (7)
- internal/authentication/types.go
- internal/handlers/handler_authz_authn.go
- internal/handlers/handler_oidc_authorization.go
- internal/handlers/handler_oidc_userinfo.go
- internal/oidc/const.go
- internal/oidc/types.go
- internal/session/user_session.go
Additional comments not posted (11)
internal/handlers/oidc.go (8)
5-5: Adding imports for
github.com/authelia/authelia/v4/internal/authentication,github.com/authelia/authelia/v4/internal/middlewares, andgithub.com/authelia/authelia/v4/internal/utilsaligns with the enhancements made in OIDC and OAuth 2.0 flows. Ensure these packages are used effectively in the updated logic.
18-30: The
oidcGrantRequestsfunction now returnsextraClaims, which is a significant change. Ensure that all calls to this function throughout the codebase have been updated to handle the returnedextraClaimsappropriately. This change is crucial for the enhanced user details refresh mechanism.Verification successful
The calls to
oidcGrantRequestsacross the codebase have been updated to handle the returnedextraClaimsappropriately, as evidenced by the assignment of the return value toextraClaimsin both test and handler files. This indicates that the necessary adjustments have been made to accommodate the change in the function's return type.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to oidcGrantRequests and ensure they handle extraClaims. ast-grep --lang go --pattern $'oidcGrantRequests($_, $_, $_)'Length of output: 924
33-53: The
oidcApplyScopeClaimsfunction introduces logic to update claims based on the granted scopes. This is a key part of the user details refresh mechanism. Ensure that the claims are correctly applied and that the function is tested thoroughly, especially for edge cases like empty scopes or unexpected scope values.
55-83: The
oidcGetAudiencefunction adds logic to handle audience claims, which is crucial for OIDC flows. Ensure that the audience is correctly determined and that this function is covered by unit tests to verify its behavior with different claim types.
86-110: The
oidcApplyUserInfoClaimsfunction updates claims with user information, which is central to the user details refresh mechanism. It's important to ensure that this function does not overwrite essential OIDC claims and that it's tested for various scenarios, including missing or additional claims.
112-129: The
oidcApplyUserInfoDetailsClaimsfunction is a new addition that applies user details to claims based on scopes. This function relies on a resolver to fetch user details, which introduces a dependency on external systems. Ensure error handling is robust and that there are fallbacks or logs for when user details cannot be resolved.Consider adding logging or error metrics for cases where user details cannot be resolved, to aid in debugging and monitoring.
131-154: The
oidcApplyUserInfoDetailsClaimsGetSubjectfunction extracts the subject from claims and ensures it's a valid UUID. This is a critical validation step in the OIDC flow. Ensure that this function is thoroughly tested, particularly for cases with invalid or missing subject claims.
157-174: The
oidcCtxDetailResolverfunction provides a resolver for fetching user details based on a subject UUID. This function interacts with external providers, so it's important to ensure that error handling is comprehensive and that there are tests covering various failure scenarios.internal/handlers/oidc_test.go (3)
4-4: Adding imports for
fmt,authelia.com/provider/oauth2, andgithub.com/google/uuidaligns with the new test functions added for OIDC claims mapping. Ensure these packages are used effectively in the test logic.
113-255: The
TestOIDCApplyUserInfoClaimsfunction and its associated test cases are crucial for ensuring the correct application of OIDC user info claims. It's important to verify that all scenarios are covered, including edge cases like missing subjects or errors during user detail resolution. Consider adding more test cases if any scenarios are not yet covered.
257-261: The
oidcTestDetailerFromSubjectfunction provides a mock detail resolver for testing, which is a good practice for isolating tests from external dependencies. Ensure that this function is used consistently across tests that require user detail resolution.
Signed-off-by: James Elliott <james-d-elliott@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (2)
- internal/handlers/handler_oidc_authorization.go (4 hunks)
- internal/handlers/oidc.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- internal/handlers/handler_oidc_authorization.go
- internal/handlers/oidc.go
Additional Context Used
The user details refresh does not naturally occur via OpenID Connect 1.0 flows and instead relies on alternative activity. This helps ensure the details are more frequently updated via normal OAuth 2.0 flows. Signed-off-by: James Elliott <james-d-elliott@users.noreply.github.com>
The user details refresh does not naturally occur via OpenID Connect 1.0 flows and instead relies on alternative activity. This helps ensure the details are more frequently updated via normal OAuth 2.0 flows.
Summary by CodeRabbit
sector_identifier_urivalidation in OIDC configuration, including error messaging and handling of various URI components.