Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: oauth2 authorization bearer #6774

Merged
merged 2 commits into from
Mar 5, 2024
Merged

Conversation

james-d-elliott
Copy link
Member

@james-d-elliott james-d-elliott commented Mar 4, 2024

This implements user authorization utilizing the OAuth 2.0 bearer scheme (i.e. RFC6750) for both the authorize code grant and client credentials grant. This effectively allows application "passwords" when used with the client credentials grant.

Closes #2023, Closes #188.

Summary by CodeRabbit

  • New Features

    • Updated Swagger UI to version 5.11.9.
    • Added new authorization schemes and strategies for server endpoints.
    • Introduced functionality for handling OAuth 2.0 and OpenID Connect bearer tokens.
    • Added new validation patterns for tokens.
    • Enhanced OpenID Connect configuration with clearer scope values and issuer key specifics.
    • Added special scopes including authelia.bearer.authz.
  • Documentation

    • Revised and updated various configuration guides and integration instructions.
    • Clarified allowed scope values for OpenID Connect clients and provider configurations.
    • Added documentation for new schemes field in server configuration.
  • Bug Fixes

    • Fixed and updated date fields in identity validation documentation.
    • Corrected error message formats related to OIDC provider and client configurations.
  • Refactor

    • Refactored authorization handling and validation logic for improved clarity and functionality.
    • Updated internal models and handlers for authorization schemes.
  • Tests

    • Added and modified tests to cover new features and bug fixes, ensuring robustness and reliability.
  • Chores

    • Updated deprecation entries and constants for clearer configuration management.

@authelia
Copy link

authelia bot commented Mar 4, 2024

Artifacts

These changes are published for testing on Buildkite, DockerHub and GitHub Container Registry.

Docker Container

  • docker pull authelia/authelia:feat-authz-oauth-bearer
  • docker pull ghcr.io/authelia/authelia:feat-authz-oauth-bearer

Copy link
Contributor

coderabbitai bot commented Mar 4, 2024

Walkthrough

This update brings significant enhancements across Authelia's configuration, authorization, and OpenID Connect integration. Key changes include updating Swagger UI, clarifying OpenID Connect scopes, revising identity validation dates, introducing new authorization prefixes and schemes, refining OAuth 2.0 bearer token usage, adding environment variable templating functions, and updating OIDC client credential flows. These changes aim to streamline user authentication, improve configuration clarity, and enhance security and integration capabilities.

Changes

Files Change Summary
cmd/authelia-scripts/.../gen.go Updated Swagger UI version.
docs/content/en/configuration/... Clarified OpenID Connect scopes; Revised issuer_private_keys, updated date fields, added schemes field, introduced prefixes for subjects, and added special scopes section.
internal/authorization/..., internal/configuration/..., internal/handlers/..., internal/model/..., internal/oidc/... Enhanced authorization logic, OIDC client credential flows, session handling, and introduced new authorization schemes.
internal/templates/funcs.go, web/src/views/LoginPortal/ConsentView/ConsentView.tsx Added mustEnv function and updated UI elements for new scopes.

Assessment against linked issues

Objective Addressed Explanation
Enable users to create API tokens via the login portal (#2023) No direct mention of user interface changes or token creation mechanisms in the login portal.
Expose the Authorization: Bearer <token> header to the backend (#2922)
Streamline authentication and authorization flow with Authelia (#2922)

Related issues

  • Issue authelia/authelia#2922 could be linked to this PR as the changes introduced, especially around the Authorization: Bearer header and OAuth 2.0 bearer token usage, align with the objectives of simplifying the authentication and authorization flow.

Poem

In the realm of code, where secrets hide,
A rabbit hopped, with strides so wide.
🐇💻 Through changes vast, it left its mark,
Authelia's path, it lit and stark.
🛡️🔒 With tokens and scopes, it danced around,
In security and ease, solutions found.
"To safer realms we hop," it said,
With every line of code it spread.

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?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit-tests for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit tests for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit tests.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

Copy link

netlify bot commented Mar 4, 2024

Deploy Preview for authelia-staging ready!

Name Link
🔨 Latest commit 9732caa
🔍 Latest deploy log https://app.netlify.com/sites/authelia-staging/deploys/65e5a58abf75d10008c3dff2
😎 Deploy Preview https://deploy-preview-6774--authelia-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 20

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 6c35a20 and 0823989.
Files ignored due to path filters (6)
  • config.template.yml is excluded by: !**/*.yml
  • docs/data/configkeys.json is excluded by: !**/*.json
  • docs/static/schemas/latest/json-schema/configuration.json is excluded by: !**/*.json
  • docs/static/schemas/v4.38/json-schema/configuration.json is excluded by: !**/*.json
  • internal/configuration/config.template.yml is excluded by: !**/*.yml
  • internal/server/locales/en/portal.json is excluded by: !**/*.json
Files selected for processing (55)
  • cmd/authelia-scripts/cmd/gen.go (1 hunks)
  • docs/content/en/configuration/identity-providers/openid-connect/clients.md (1 hunks)
  • docs/content/en/configuration/identity-providers/openid-connect/provider.md (2 hunks)
  • docs/content/en/configuration/identity-validation/_index.md (1 hunks)
  • docs/content/en/configuration/identity-validation/elevated-session.md (1 hunks)
  • docs/content/en/configuration/identity-validation/introduction.md (1 hunks)
  • docs/content/en/configuration/identity-validation/reset-password.md (1 hunks)
  • docs/content/en/configuration/methods/secrets.md (1 hunks)
  • docs/content/en/configuration/miscellaneous/server-endpoints-authz.md (2 hunks)
  • docs/content/en/configuration/miscellaneous/server.md (1 hunks)
  • docs/content/en/configuration/security/access-control.md (1 hunks)
  • docs/content/en/integration/openid-connect/introduction.md (1 hunks)
  • docs/content/en/integration/openid-connect/oauth-2.0-bearer-token-usage.md (1 hunks)
  • docs/content/en/reference/guides/templating.md (1 hunks)
  • internal/authorization/access_control_subjects.go (1 hunks)
  • internal/authorization/authorizer_test.go (2 hunks)
  • internal/authorization/const.go (1 hunks)
  • internal/authorization/types.go (1 hunks)
  • internal/authorization/util.go (2 hunks)
  • internal/configuration/deprecation.go (1 hunks)
  • internal/configuration/helpers_test.go (1 hunks)
  • internal/configuration/schema/const.go (1 hunks)
  • internal/configuration/schema/identity_providers.go (2 hunks)
  • internal/configuration/schema/keys.go (1 hunks)
  • internal/configuration/schema/server.go (2 hunks)
  • internal/configuration/schema/types.go (1 hunks)
  • internal/configuration/validator/access_control.go (1 hunks)
  • internal/configuration/validator/const.go (8 hunks)
  • internal/configuration/validator/identity_providers.go (11 hunks)
  • internal/configuration/validator/identity_providers_test.go (10 hunks)
  • internal/configuration/validator/server.go (3 hunks)
  • internal/configuration/validator/server_test.go (3 hunks)
  • internal/configuration/validator/util.go (1 hunks)
  • internal/handlers/handler_authz.go (5 hunks)
  • internal/handlers/handler_authz_authn.go (7 hunks)
  • internal/handlers/handler_authz_builder.go (3 hunks)
  • internal/handlers/handler_authz_test.go (5 hunks)
  • internal/handlers/handler_authz_types.go (3 hunks)
  • internal/handlers/handler_oidc_token.go (1 hunks)
  • internal/handlers/handler_oidc_userinfo.go (1 hunks)
  • internal/model/authorization.go (1 hunks)
  • internal/model/authorization_test.go (1 hunks)
  • internal/model/const.go (1 hunks)
  • internal/model/oidc_test.go (1 hunks)
  • internal/oidc/amr.go (1 hunks)
  • internal/oidc/const.go (2 hunks)
  • internal/oidc/flow_client_credentials.go (2 hunks)
  • internal/oidc/flow_client_credentials_test.go (2 hunks)
  • internal/oidc/flow_refresh.go (1 hunks)
  • internal/oidc/session.go (2 hunks)
  • internal/oidc/types_test.go (2 hunks)
  • internal/oidc/util.go (2 hunks)
  • internal/oidc/util_blackbox_test.go (1 hunks)
  • internal/templates/funcs.go (3 hunks)
  • web/src/views/LoginPortal/ConsentView/ConsentView.tsx (3 hunks)
Files skipped from review as they are similar to previous changes (38)
  • cmd/authelia-scripts/cmd/gen.go
  • docs/content/en/configuration/identity-validation/_index.md
  • internal/authorization/access_control_subjects.go
  • internal/authorization/authorizer_test.go
  • internal/authorization/const.go
  • internal/authorization/types.go
  • internal/authorization/util.go
  • internal/configuration/deprecation.go
  • internal/configuration/helpers_test.go
  • internal/configuration/schema/const.go
  • internal/configuration/schema/identity_providers.go
  • internal/configuration/schema/keys.go
  • internal/configuration/schema/server.go
  • internal/configuration/schema/types.go
  • internal/configuration/validator/access_control.go
  • internal/configuration/validator/const.go
  • internal/configuration/validator/identity_providers.go
  • internal/configuration/validator/server.go
  • internal/configuration/validator/server_test.go
  • internal/configuration/validator/util.go
  • internal/handlers/handler_authz_builder.go
  • internal/handlers/handler_authz_test.go
  • internal/handlers/handler_authz_types.go
  • internal/handlers/handler_oidc_token.go
  • internal/model/authorization.go
  • internal/model/authorization_test.go
  • internal/model/const.go
  • internal/model/oidc_test.go
  • internal/oidc/amr.go
  • internal/oidc/const.go
  • internal/oidc/flow_client_credentials.go
  • internal/oidc/flow_client_credentials_test.go
  • internal/oidc/flow_refresh.go
  • internal/oidc/session.go
  • internal/oidc/util.go
  • internal/oidc/util_blackbox_test.go
  • internal/templates/funcs.go
  • web/src/views/LoginPortal/ConsentView/ConsentView.tsx
Additional comments: 23
docs/content/en/configuration/identity-validation/introduction.md (1)
  • 5-5: The update of the date field to 2024-03-04T20:29:11+11:00 correctly reflects the recent changes made to the document. This ensures that readers are aware of the document's currency.
docs/content/en/configuration/identity-validation/reset-password.md (1)
  • 5-5: The update of the date field to 2024-03-04T20:29:11+11:00 correctly reflects the recent changes made to the document. This ensures that readers are aware of the document's currency.
docs/content/en/configuration/identity-validation/elevated-session.md (1)
  • 5-5: The update of the date field to 2024-03-04T20:29:11+11:00 correctly reflects the recent changes made to the document. This ensures that readers are aware of the document's currency.
docs/content/en/configuration/miscellaneous/server-endpoints-authz.md (1)
  • 29-44: The addition of the schemes field under authn_strategies for HeaderProxyAuthorization, CookieSession, and HeaderAuthRequestProxyAuthorization is a valuable enhancement. It allows for specifying allowed schemes (Basic, Bearer), providing administrators with more control over the authorization process. Ensure that the documentation clearly explains the usage and implications of this field to help users configure it correctly.
docs/content/en/reference/guides/templating.md (1)
  • 102-104: The introduction of the mustEnv function is well-documented, providing a clear explanation of its behavior and use case. This function enhances the templating capabilities by ensuring that templates fail fast if required environment variables are not set, which is a good practice for error handling.
internal/oidc/types_test.go (1)
  • 2-14: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]

The adjustments in TestNewSessionWithAuthorizeRequest to streamline test setup and assertions are a positive step towards improving test clarity and maintainability. Ensure that all relevant test scenarios are still adequately covered after these changes.

internal/handlers/handler_oidc_userinfo.go (2)
  • 47-47: The modification to use oidc.RFC6750Header for constructing the WWW-Authenticate header is a good practice as it centralizes the logic for creating this header, ensuring consistency and reducing the likelihood of errors in header formatting.
  • 44-50: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [67-73]

It's noted that the client variable might be modified and later used, which could lead to potential nil dereference issues if not handled properly. Ensure that client is always in a valid state before accessing its methods or properties to avoid runtime panics.

docs/content/en/configuration/miscellaneous/server.md (1)
  • 42-42: The restructuring of the authz section to use a generic empty object {} with a reference to a dedicated configuration guide is a clear and concise way to direct users to more detailed information. This change simplifies the configuration documentation and makes it easier for users to find the information they need.
docs/content/en/integration/openid-connect/oauth-2.0-bearer-token-usage.md (1)
  • 95-133: The YAML configuration examples for enabling the authorization scheme are clear and well-structured. However, it's crucial to ensure that these examples are tested and validated in a real-world setup to avoid potential misconfigurations.

Consider adding a note or disclaimer encouraging users to test these configurations in a staging environment before applying them in production.

internal/handlers/handler_authz_authn.go (2)
  • 5-5: The import of "context" is a good addition, ensuring that functions can leverage context for cancellation, timeouts, and passing request-scoped values.
  • 13-13: The import of "github.com/ory/fosite" is crucial for OAuth2 and OpenID Connect frameworks, indicating that the changes likely involve advanced authorization handling.
docs/content/en/configuration/identity-providers/openid-connect/provider.md (2)
  • 119-119: The change from optional to required for issuer_private_keys is significant and should be clearly communicated to users to ensure they understand the necessity of providing at least one RSA Private key with the RS256 algorithm. This change enhances security by enforcing the presence of secure keys for token signing.
  • 116-169: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-590]

Overall, the documentation is clear and informative, effectively guiding users through the configuration of an OpenID Connect 1.0 Provider in Authelia. The updates enhance the document's clarity, especially regarding the issuer_private_keys configuration. Ensure that all examples and configurations are up-to-date and reflect best practices in security and usability.

docs/content/en/integration/openid-connect/introduction.md (1)
  • 130-142: The addition of the section on special scopes, particularly authelia.bearer.authz, is a valuable update. It clearly outlines the purpose and usage of this scope in the context of bearer authorization schemes for endpoints protected via Authelia. This enhancement aligns well with the objectives of enhancing OAuth 2.0 bearer scheme support and improving authorization capabilities.
internal/configuration/validator/identity_providers_test.go (8)
  • 37-38: The error messages in the assertions are clear and correctly specify the missing required options for OIDC server configuration. This ensures that users are informed about the exact configuration issues.
  • 41-61: Adding a new test function TestShouldRaiseErrorWhenInvalidOIDCServerConfigurationBothKeyTypesSpecified is a good practice to ensure that the configuration validation logic correctly handles scenarios where both key types are specified, which is not allowed. This enhances the robustness of the configuration validation.
  • 214-237: The test case BadIDInvalidCharacters correctly checks for invalid characters in the client ID, ensuring that only RFC3986 unreserved characters are allowed. This is important for maintaining consistency and avoiding potential issues with client identification.
  • 254-272: The test case InvalidPolicyCCG correctly identifies an invalid authorization policy for a client credentials grant. This is crucial for ensuring that the authorization policy is compatible with the grant type used by the client.
  • 899-905: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [902-912]

The test case ShouldNotIncludeOldMinimalScope is well-structured and ensures that the old minimal scope is not included in the default scopes. This helps in keeping the scope definitions up to date and relevant.

  • 1087-1096: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1075-1091]

The warning generated for invalid scopes in the test case ShouldRaiseWarningOnInvalidScopes is a good practice. It alerts the user about the presence of unknown scopes, which might be a result of a typo or a misunderstanding of the available scopes. This can help in preventing configuration errors.

  • 2081-2126: The test case ShouldHandleBearerErrorsMisconfiguredPublicClientType is comprehensive and checks for various misconfigurations related to the authelia.bearer.authz scope. This is important for ensuring that clients configured with this scope meet all the necessary requirements for secure operation.
  • 2299-2335: The test case ShouldHandleBearerValidPublicClientType effectively validates a correctly configured public client type with the authelia.bearer.authz scope. This ensures that the validation logic properly recognizes valid configurations, which is essential for user confidence in the configuration process.

internal/handlers/handler_authz_authn.go Show resolved Hide resolved
internal/handlers/handler_authz_authn.go Show resolved Hide resolved
internal/handlers/handler_authz_authn.go Show resolved Hide resolved
internal/handlers/handler_authz_authn.go Show resolved Hide resolved
internal/handlers/handler_authz_authn.go Show resolved Hide resolved
internal/handlers/handler_authz.go Show resolved Hide resolved
internal/handlers/handler_authz.go Show resolved Hide resolved
internal/handlers/handler_oidc_userinfo.go Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 9

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 0823989 and f375cf9.
Files selected for processing (6)
  • docs/content/en/configuration/miscellaneous/server-endpoints-authz.md (3 hunks)
  • docs/content/en/configuration/miscellaneous/server.md (2 hunks)
  • docs/content/en/contributing/development/build-and-test.md (1 hunks)
  • docs/content/en/integration/openid-connect/tailscale/index.md (1 hunks)
  • internal/commands/services.go (1 hunks)
  • internal/server/server.go (1 hunks)
Files skipped from review due to trivial changes (2)
  • internal/commands/services.go
  • internal/server/server.go
Additional comments: 8
docs/content/en/configuration/miscellaneous/server-endpoints-authz.md (4)
  • 29-30: The addition of the schemes field under authn_strategies for HeaderProxyAuthorization with the value 'Basic' is clear and aligns with the objective to enhance authorization configurations. This allows for specifying the authentication schemes that are acceptable for each strategy, improving flexibility and security.
  • 36-37: Similarly, the schemes field is added under HeaderProxyAuthorization for the ext-authz endpoint. This consistency across different authorization endpoints ensures that the configuration is uniform and understandable, which is beneficial for maintainability and ease of use.
  • 43-44: The addition of the schemes field for HeaderAuthRequestProxyAuthorization under the auth-request endpoint follows the same pattern as the previous configurations. This addition is crucial for specifying acceptable authentication schemes, enhancing the security and flexibility of the authorization process.
  • 90-96: The documentation for the schemes field is well-written, providing clear information on its purpose, applicable options (Basic, and Bearer), and its applicability to specific strategies. This documentation is essential for users to understand how to configure their authorization strategies properly. However, it's worth noting that the documentation mentions the schemes field is unavailable with the legacy endpoint, which is an important detail for ensuring correct configuration.
docs/content/en/contributing/development/build-and-test.md (1)
  • 17-17: The correction from "webserver" to "web server" in the description of Authelia's build process is a minor but important change for clarity and adherence to standard terminology. This change ensures that the documentation is professional and easy to understand.
docs/content/en/integration/openid-connect/tailscale/index.md (1)
  • 67-67: The correction from "webserver" to "web server" improves clarity and aligns with standard English usage.
docs/content/en/configuration/miscellaneous/server.md (2)
  • 4-4: The lead sentence mentions "Authelia runs an internal web server." which is correct, but static analysis hinted at a possible spelling mistake with "Authelia". Since this is the correct name of the project, no change is needed here.
  • 42-42: The change to the authz configuration simplifies the previous specific implementations to a generic empty object {}. This change aligns with the PR objectives by streamlining the configuration process for server authorization endpoints. However, it's crucial to ensure that the dedicated "Server Authz Endpoints" configuration guide is comprehensive and easily accessible to users, as this change places more emphasis on external documentation for configuration details.

Ensure the dedicated configuration guide for "Server Authz Endpoints" is updated and easily accessible to users, providing clear instructions on how to configure authorization endpoints with the new structure.

@james-d-elliott james-d-elliott force-pushed the feat-authz-oauth-bearer branch 2 times, most recently from e382dae to 9732caa Compare March 4, 2024 10:42
Copy link

codecov bot commented Mar 4, 2024

Codecov Report

Attention: Patch coverage is 77.93651% with 139 lines in your changes are missing coverage. Please review.

Project coverage is 75.59%. Comparing base (c70c83f) to head (9732caa).

❗ Current head 9732caa differs from pull request most recent head d08d6c5. Consider uploading reports for the commit d08d6c5 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #6774       +/-   ##
===========================================
+ Coverage   54.25%   75.59%   +21.34%     
===========================================
  Files         352      353        +1     
  Lines       30244    30789      +545     
  Branches      793      833       +40     
===========================================
+ Hits        16409    23276     +6867     
+ Misses      13170     6633     -6537     
- Partials      665      880      +215     
Flag Coverage Δ
backend 76.67% <78.43%> (+18.03%) ⬆️
frontend 65.80% <0.00%> (+51.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
internal/authorization/access_control_subjects.go 100.00% <100.00%> (ø)
internal/authorization/types.go 100.00% <ø> (ø)
internal/authorization/util.go 98.76% <100.00%> (+0.04%) ⬆️
internal/commands/services.go 44.64% <ø> (+44.64%) ⬆️
internal/configuration/deprecation.go 100.00% <100.00%> (ø)
internal/configuration/schema/types.go 77.52% <ø> (+3.45%) ⬆️
internal/configuration/validator/access_control.go 100.00% <100.00%> (ø)
...rnal/configuration/validator/identity_providers.go 98.76% <100.00%> (+0.11%) ⬆️
internal/configuration/validator/server.go 100.00% <100.00%> (ø)
internal/configuration/validator/util.go 100.00% <100.00%> (ø)
... and 15 more

... and 157 files with indirect coverage changes

refactor: improvements

Signed-off-by: James Elliott <james-d-elliott@users.noreply.github.com>
Signed-off-by: James Elliott <james-d-elliott@users.noreply.github.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 18

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between c70c83f and d08d6c5.
Files ignored due to path filters (6)
  • config.template.yml is excluded by: !**/*.yml
  • docs/data/configkeys.json is excluded by: !**/*.json
  • docs/static/schemas/latest/json-schema/configuration.json is excluded by: !**/*.json
  • docs/static/schemas/v4.38/json-schema/configuration.json is excluded by: !**/*.json
  • internal/configuration/config.template.yml is excluded by: !**/*.yml
  • internal/server/locales/en/portal.json is excluded by: !**/*.json
Files selected for processing (55)
  • docs/content/en/configuration/identity-providers/openid-connect/clients.md (1 hunks)
  • docs/content/en/configuration/identity-providers/openid-connect/provider.md (2 hunks)
  • docs/content/en/configuration/methods/secrets.md (1 hunks)
  • docs/content/en/configuration/miscellaneous/server-endpoints-authz.md (3 hunks)
  • docs/content/en/configuration/miscellaneous/server.md (6 hunks)
  • docs/content/en/configuration/security/access-control.md (1 hunks)
  • docs/content/en/contributing/development/build-and-test.md (1 hunks)
  • docs/content/en/integration/openid-connect/introduction.md (1 hunks)
  • docs/content/en/integration/openid-connect/oauth-2.0-bearer-token-usage.md (1 hunks)
  • docs/content/en/integration/openid-connect/tailscale/index.md (1 hunks)
  • docs/content/en/reference/guides/templating.md (1 hunks)
  • internal/authorization/access_control_subjects.go (1 hunks)
  • internal/authorization/authorizer_test.go (2 hunks)
  • internal/authorization/const.go (1 hunks)
  • internal/authorization/types.go (1 hunks)
  • internal/authorization/util.go (2 hunks)
  • internal/commands/services.go (1 hunks)
  • internal/configuration/deprecation.go (1 hunks)
  • internal/configuration/helpers_test.go (1 hunks)
  • internal/configuration/schema/const.go (1 hunks)
  • internal/configuration/schema/identity_providers.go (2 hunks)
  • internal/configuration/schema/keys.go (1 hunks)
  • internal/configuration/schema/server.go (2 hunks)
  • internal/configuration/schema/types.go (1 hunks)
  • internal/configuration/validator/access_control.go (1 hunks)
  • internal/configuration/validator/const.go (8 hunks)
  • internal/configuration/validator/identity_providers.go (11 hunks)
  • internal/configuration/validator/identity_providers_test.go (10 hunks)
  • internal/configuration/validator/server.go (3 hunks)
  • internal/configuration/validator/server_test.go (3 hunks)
  • internal/configuration/validator/util.go (1 hunks)
  • internal/handlers/handler_authz.go (5 hunks)
  • internal/handlers/handler_authz_authn.go (7 hunks)
  • internal/handlers/handler_authz_builder.go (3 hunks)
  • internal/handlers/handler_authz_test.go (5 hunks)
  • internal/handlers/handler_authz_types.go (3 hunks)
  • internal/handlers/handler_oidc_token.go (1 hunks)
  • internal/handlers/handler_oidc_userinfo.go (1 hunks)
  • internal/model/authorization.go (1 hunks)
  • internal/model/authorization_test.go (1 hunks)
  • internal/model/const.go (1 hunks)
  • internal/model/oidc_test.go (1 hunks)
  • internal/oidc/amr.go (1 hunks)
  • internal/oidc/const.go (2 hunks)
  • internal/oidc/flow_client_credentials.go (2 hunks)
  • internal/oidc/flow_client_credentials_test.go (2 hunks)
  • internal/oidc/flow_refresh.go (1 hunks)
  • internal/oidc/session.go (2 hunks)
  • internal/oidc/types_test.go (2 hunks)
  • internal/oidc/util.go (2 hunks)
  • internal/oidc/util_blackbox_test.go (1 hunks)
  • internal/random/mathematical_test.go (1 hunks)
  • internal/server/server.go (1 hunks)
  • internal/templates/funcs.go (3 hunks)
  • web/src/views/LoginPortal/ConsentView/ConsentView.tsx (3 hunks)
Files skipped from review as they are similar to previous changes (38)
  • internal/authorization/access_control_subjects.go
  • internal/authorization/authorizer_test.go
  • internal/authorization/const.go
  • internal/authorization/types.go
  • internal/authorization/util.go
  • internal/commands/services.go
  • internal/configuration/deprecation.go
  • internal/configuration/helpers_test.go
  • internal/configuration/schema/const.go
  • internal/configuration/schema/identity_providers.go
  • internal/configuration/schema/keys.go
  • internal/configuration/schema/server.go
  • internal/configuration/schema/types.go
  • internal/configuration/validator/access_control.go
  • internal/configuration/validator/const.go
  • internal/configuration/validator/identity_providers.go
  • internal/configuration/validator/server.go
  • internal/configuration/validator/server_test.go
  • internal/configuration/validator/util.go
  • internal/handlers/handler_authz_authn.go
  • internal/handlers/handler_authz_builder.go
  • internal/handlers/handler_authz_test.go
  • internal/handlers/handler_authz_types.go
  • internal/handlers/handler_oidc_token.go
  • internal/model/authorization.go
  • internal/model/authorization_test.go
  • internal/model/const.go
  • internal/model/oidc_test.go
  • internal/oidc/amr.go
  • internal/oidc/const.go
  • internal/oidc/flow_client_credentials.go
  • internal/oidc/flow_refresh.go
  • internal/oidc/session.go
  • internal/oidc/util.go
  • internal/oidc/util_blackbox_test.go
  • internal/server/server.go
  • internal/templates/funcs.go
  • web/src/views/LoginPortal/ConsentView/ConsentView.tsx
Additional comments: 29
docs/content/en/contributing/development/build-and-test.md (1)
  • 17-17: The correction from "webserver" to "web server" enhances clarity and aligns with standard writing practices for technical documentation. Good catch!
internal/oidc/types_test.go (1)
  • 2-14: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-134]

The refactoring and cleanup in types_test.go appear to streamline the testing process and remove unnecessary code. However, it's important to ensure that the removal of TestPopulateClientCredentialsFlowSessionWithAccessRequest does not negatively impact test coverage, especially for error handling and edge cases. Consider verifying that the removed test cases are either no longer relevant or adequately covered by other tests.

docs/content/en/integration/openid-connect/tailscale/index.md (1)
  • 67-67: The correction from "webserver" to "web server" in the Tailscale integration documentation enhances clarity and aligns with standard writing practices for technical documentation. This change ensures consistency across the documentation.
internal/handlers/handler_oidc_userinfo.go (1)
  • 47-47: The change to how the WWW-Authenticate header is set in the response based on the error received is a good improvement for aligning with the OAuth 2.0 specification (RFC 6750). However, ensure that the error details provided in the RFC6750Header function are appropriate and do not inadvertently expose sensitive information or internal implementation details that could be leveraged by an attacker.
docs/content/en/configuration/miscellaneous/server.md (5)
  • 42-42: The change to the authz configuration simplifies the structure by using an empty object {} and referring to a dedicated guide for more detailed configuration. This approach enhances modularity and maintainability by separating concerns and allowing for more focused documentation on authorization endpoints.
  • 74-76: The asset_path configuration key is correctly defined, and the explanation about serving static assets from an embedded file system is clear. However, ensure that the link to the "Sever Asset Overrides Reference Guide" is correct and accessible to avoid potential confusion for users trying to find more information.
  • 138-139: The explanation about customizing the csp_template is informative and rightly cautions users about the complexity of modifying the Content-Security-Policy header. This guidance helps maintain security while allowing advanced customization.
  • 186-186: The note about buffer sizes and the rationale behind the recommendation for keeping read and write buffer sizes the same is insightful. It provides a clear explanation of Authelia's behavior and offers flexibility for tuning based on individual needs.
  • 191-191: The guidance on replacing the Authelia portal logo with a transparent PNG is helpful for users looking to customize their installation. It's a good practice to recommend image formats that will integrate well with the UI.
docs/content/en/integration/openid-connect/oauth-2.0-bearer-token-usage.md (4)
  • 21-25: The documentation now includes a more detailed explanation of the authelia.bearer.authz scope, enhancing clarity for readers. If further clarification has been added elsewhere in the document or in linked resources, this addresses the previous concern.
  • 88-91: The section on audience requests now provides a clearer overview. If practical examples or detailed guidance on implementing audience requests have been added, this would fully address the previous comment.
  • 170-189: If explanations or links to resources for acronyms like PAR and PKCE have been added, this would make the Client Restrictions section more accessible to readers unfamiliar with OAuth 2.0 terminology.
  • 1-1: The title and description are clear and relevant to the content. The spelling suggestions from the static analysis tool for "Authelia" are incorrect due to it being a proper noun.
internal/oidc/flow_client_credentials_test.go (3)
  • 5-5: The newly added imports are appropriate for the functionality being tested, including error handling, URL parsing, and OIDC/JWT token processing.

Also applies to: 7-7, 13-14

  • 267-358: The test function TestPopulateClientCredentialsFlowSessionWithAccessRequest is well-structured and covers a variety of scenarios. It would be beneficial to add more comments explaining the purpose of each test case for better readability.
  • 360-485: The test function TestPopulateClientCredentialsFlowRequester effectively tests various scenarios with clear and specific error handling. Consider adding comments to explain complex test cases, especially those involving scope and audience validations, to enhance maintainability and understanding.
docs/content/en/configuration/identity-providers/openid-connect/provider.md (2)
  • 131-166: The previous suggestion to add a warning about not using example keys in production environments has been addressed. However, it's crucial to ensure this warning is prominently displayed and easily noticeable by users to prevent accidental use of example keys in production settings.
  • 119-119: The issuer_private_keys configuration key has been correctly updated to be required. This change aligns with the PR objectives to enhance security and configuration clarity for OpenID Connect provider settings.
docs/content/en/integration/openid-connect/introduction.md (1)
  • 130-142: The addition of the section on special scopes, specifically authelia.bearer.authz, is clear and well-explained. It's good to see that it directs readers to another guide for more detailed information, which helps maintain the focus of this document while providing resources for deeper exploration.
internal/configuration/validator/identity_providers_test.go (10)
  • 37-38: The error messages in the assertions accurately reflect the expected validation errors for missing issuer_private_keys and at least one client configuration in the OIDC server configuration.
  • 41-61: This test case correctly identifies and asserts the error condition when both issuer_private_key and issuer_private_keys are specified, which is not allowed. It ensures that the validation logic correctly prevents this misconfiguration.
  • 214-224: The test case BadIDTooLong correctly asserts that an error is raised for a client ID exceeding the maximum allowed length. This ensures that client IDs adhere to the specified constraints for length.
  • 227-236: The test case BadIDInvalidCharacters correctly asserts that an error is raised for a client ID containing invalid characters. This validation is essential for ensuring client IDs conform to RFC3986 unreserved characters.
  • 254-271: The test case InvalidPolicyCCG correctly asserts that an error is raised for an invalid authorization policy in the context of the client credentials grant. This ensures that only valid policies are applied to different grant types.
  • 2081-2126: The test case ShouldHandleBearerErrorsMisconfiguredPublicClientType thoroughly checks for various misconfigurations related to the authelia.bearer.authz scope for a public client type. It ensures that the validation logic correctly identifies and reports multiple configuration issues, enhancing the robustness of the OIDC implementation.
  • 2129-2173: The test case ShouldHandleBearerErrorsMisconfiguredConfidentialClientType effectively tests for various misconfigurations related to the authelia.bearer.authz scope for a confidential client type. It ensures comprehensive validation against incorrect configurations, contributing to the security and correctness of the OIDC feature.
  • 2177-2216: The test case ShouldHandleBearerErrorsMisconfiguredConfidentialClientTypeClientCredentials specifically targets misconfigurations for a confidential client type using the client credentials grant with the authelia.bearer.authz scope. It accurately identifies configuration errors, ensuring that only secure and correct configurations are allowed.
  • 2220-2257: The test case ShouldHandleBearerErrorsNotExplicit checks for various misconfigurations when the authelia.bearer.authz scope is used without explicitly setting necessary configurations. It ensures that the validation logic correctly identifies missing or incorrect configurations, promoting secure and correct usage of bearer tokens.
  • 2261-2332: The test case ShouldHandleBearerValidPublicClientType effectively validates a correctly configured public client type using the authelia.bearer.authz scope. It ensures that the validation logic allows secure and correct configurations, promoting best practices in the use of bearer tokens for authentication.

@james-d-elliott james-d-elliott enabled auto-merge (squash) March 5, 2024 09:08
@james-d-elliott james-d-elliott merged commit fb50f1a into master Mar 5, 2024
46 checks passed
@james-d-elliott james-d-elliott deleted the feat-authz-oauth-bearer branch March 5, 2024 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

User-creatable tokens for authentication Support for application passwords
1 participant