feat: add oidc provider using doorkeeper gem#7668
Conversation
![accesslint[bot]](https://avatars.githubusercontent.com/in/1502?s=60&v=4){kind=small}
| <p>This application will be able to:</p> | ||
| <ul> | ||
| <% @pre_auth.scopes.each do |scope| %> | ||
| <li><%= scope %></li> |
There was a problem hiding this comment.
WCAG 1.3.1: <li> is not contained in a <ul>, <ol>, or <menu>.
<li> elements must be contained in a <ul>, <ol>, or <menu>.
Details
List items (<li>) only have semantic meaning inside a list container (<ul>, <ol>, or <menu>). Outside of these containers, assistive technologies cannot convey the list relationship. Wrap <li> elements in the appropriate list container.
vet Summary ReportThis report is generated by vet Policy Checks
Malicious Package AnalysisActive malicious package analysis was disabled. Learn more about enabling active package analysis Malicious Package Analysis Report
Changed PackagesChanged Packages
|
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds Doorkeeper OAuth 2 and OpenID Connect support, including dependencies, database tables, foreign keys, provider configuration, routes, locales, administration resources, authorization views, and hybrid API authentication. Adds JWT and OAuth request handling, OAuth application policies, and request specs covering bearer-token API access and PKCE authorization-code exchange. Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| <div class="col-md-8"> | ||
| <div class="card"> | ||
| <div class="card-header"> | ||
| <h3 class="mb-0"><%= t("doorkeeper.authorizations.title") %></h3> |
There was a problem hiding this comment.
Best Practice: Heading is empty. Add text content or remove the heading element.
Headings must have discernible text.
Details
Screen reader users navigate pages by headings, so empty headings create confusing navigation points. Ensure all headings contain visible text or accessible names. If a heading is used purely for visual styling, use CSS instead of heading elements.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
config/initializers/doorkeeper.rb (1)
173-176: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider enabling
force_pkcefor non-confidential clients.
oauth_applications.confidentialsupports public clients, butforce_pkceis left disabled (the generator default). Without it, non-confidential clients aren't required to use PKCE on the authorization_code flow, leaving them exposed to authorization-code interception.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a350fc8a-c621-482b-a376-6ef7c459e94b
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
.gitignoreGemfileapp/avo/resources/doorkeeper_application.rbapp/controllers/api/v1/base_controller.rbapp/policies/doorkeeper/application_policy.rbapp/views/doorkeeper/authorizations/new.html.erbapp/views/layouts/doorkeeper.html.erbconfig/initializers/doorkeeper.rbconfig/initializers/doorkeeper_openid_connect.rbconfig/locales/doorkeeper.en.ymlconfig/locales/doorkeeper_openid_connect.en.ymlconfig/routes.rbdb/migrate/20260719133550_create_doorkeeper_tables.rbdb/migrate/20260719140000_validate_doorkeeper_foreign_keys.rbdb/migrate/20260719140001_create_doorkeeper_openid_connect_tables.rbdb/migrate/20260719140002_validate_doorkeeper_openid_connect_foreign_keys.rbdb/schema.rbspec/requests/api/v1/oauth_access_spec.rbspec/requests/oauth_pkce_flow_spec.rb
| def user_from_doorkeeper | ||
| return nil unless defined?(Doorkeeper) && doorkeeper_token&.resource_owner_id | ||
|
|
||
| User.find_by(id: doorkeeper_token.resource_owner_id) | ||
| end |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm that `doorkeeper_token` does not check for expiration or revocation on its own.
# Inspect Doorkeeper's valid_doorkeeper_token? and doorkeeper_token methods.
# We skip cloning since Doorkeeper source is not in the repo, but we can check if there are any usages locally.
rg "doorkeeper_token" --type rbRepository: CircuitVerse/CircuitVerse
Length of output: 190
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant controller and inspect the surrounding methods.
wc -l app/controllers/api/v1/base_controller.rb
sed -n '1,220p' app/controllers/api/v1/base_controller.rb
# Search for Doorkeeper-related authentication paths in the repo.
rg -n "doorkeeper_token|doorkeeper_authorize!|authenticate_user!|acceptable\?|accessible\?" app config lib spec testRepository: CircuitVerse/CircuitVerse
Length of output: 7612
🌐 Web query:
Doorkeeper doorkeeper_token acceptable? accessible? revoked expired helper semantics
💡 Result:
In the Doorkeeper gem, several key methods and helpers manage the lifecycle and validation of access tokens [1][2]. doorkeeper_token This helper method retrieves the current access token from the request [3][1]. It performs authentication by attempting to extract and locate the token based on the methods configured in Doorkeeper (e.g., Authorization header, request parameters) [1][4][2]. accessible? This method indicates whether an access token is valid for use [5]. It returns true if and only if the token is both not expired and not revoked [5]. Internally, it is defined as!expired? &&!revoked? [5]. acceptable?(scopes) This method determines if the token is valid for specific requested scopes [6]. It returns true only if the token is accessible? (not expired, not revoked) AND includes the required scopes [7][6]. Revoked and Expired Doorkeeper tracks revocation via a revoked_at timestamp column [8]. A token is considered revoked if revoked_at is present [5][8]. Expiration is determined by comparing the current time against the token's expiration time [8]. Note that expired tokens may still technically exist in the database unless cleaned up, and there have been discussions in the community regarding the specific behavior of revocation endpoints when applied to tokens that are already expired [9]. Summary of Semantics Helper: doorkeeper_token extracts the token [1]. Validation: accessible? checks basic validity (active/non-expired) [5]. Authorization: acceptable?(scopes) checks both validity and authorization scope compliance [6][1].
Citations:
- 1: https://github.com/doorkeeper-gem/doorkeeper/blob/main/lib/doorkeeper/rails/helpers.rb
- 2: https://www.rubydoc.info/github/doorkeeper-gem/doorkeeper/Doorkeeper/OAuth/Token
- 3: https://github.com/doorkeeper-gem/doorkeeper/blob/main/lib/doorkeeper/helpers/controller.rb
- 4: https://www.rubydoc.info/github/doorkeeper-gem/doorkeeper/Doorkeeper%2FRails%2FHelpers:doorkeeper_token
- 5: https://www.rubydoc.info/github/doorkeeper-gem/doorkeeper/Doorkeeper/Models/Accessible
- 6: https://www.rubydoc.info/github/doorkeeper-gem/doorkeeper/Doorkeeper/AccessTokenMixin
- 7: https://github.com/doorkeeper-gem/doorkeeper/blob/main/lib/doorkeeper/models/access_token_mixin.rb
- 8: https://www.rubydoc.info/github/doorkeeper-gem/doorkeeper/Doorkeeper/Orm/ActiveRecord/Mixins/AccessToken/ClassMethods
- 9:
oauth/revokeendpoint doesn't revoke expired access tokens doorkeeper-gem/doorkeeper#1579
Reject revoked/expired Doorkeeper tokens in user_from_doorkeeper. doorkeeper_token can still be present after expiry or revocation; gate this path with doorkeeper_token&.accessible? before mapping it to a user so authenticate_user! doesn’t accept invalid OAuth tokens.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6fb80fa3-c2b7-44de-8e29-26100ba070bc
📒 Files selected for processing (3)
app/controllers/api/v1/base_controller.rbapp/views/doorkeeper/authorizations/new.html.erbconfig/locales/en.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- app/views/doorkeeper/authorizations/new.html.erb
- app/controllers/api/v1/base_controller.rb
| project: | ||
| show_more: "Show More" | ||
| options: | ||
| users: "Users" | ||
| projects: "Projects" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix the duplicate key and preserve the search-bar translation contract.
options is declared twice under components.search_bar, and project is empty, so the intended nesting was not applied. This can make locale loading fail or overwrite the earlier mapping. Also, app/components/search/search_bar_component.rb:52-76 still reads components.search_bar.options.users/projects; either preserve that path by removing the duplicate block, or update the consumer together with a correctly nested project.options structure.
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 72-72: duplication of key "options" in mapping
(key-duplicates)
Source: Linters/SAST tools
|
checking |
| <div class="col-md-8"> | ||
| <div class="card"> | ||
| <div class="card-header"> | ||
| <h3 class="mb-0"><%= t("doorkeeper.authorizations.title") %></h3> |
There was a problem hiding this comment.
Best Practice: Heading is empty. Add text content or remove the heading element.
Headings must have discernible text.
Details
Screen reader users navigate pages by headings, so empty headings create confusing navigation points. Ensure all headings contain visible text or accessible names. If a heading is used purely for visual styling, use CSS instead of heading elements.
| <p><%= t("doorkeeper.authorizations.scope_intro") %></p> | ||
| <ul> | ||
| <% @pre_auth.scopes.each do |scope| %> | ||
| <li><%= scope %></li> |
There was a problem hiding this comment.
WCAG 1.3.1: <li> is not contained in a <ul>, <ol>, or <menu>.
<li> elements must be contained in a <ul>, <ol>, or <menu>.
Details
List items (<li>) only have semantic meaning inside a list container (<ul>, <ol>, or <menu>). Outside of these containers, assistive technologies cannot convey the list relationship. Wrap <li> elements in the appropriate list container.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/initializers/doorkeeper.rb (1)
239-243:⚠️ Potential issue | 🟠 MajorRegister the required
openidscope.OIDC authorization requests using
scope=openidwill be rejected because onlypublic,profile, and:openidto the configured scopes, typically as an optional scope.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ee941781-6b23-40a7-bb51-ed033275f98d
📒 Files selected for processing (5)
app/views/doorkeeper/authorizations/new.html.erbconfig/initializers/doorkeeper.rbconfig/locales/en.ymlspec/requests/api/v1/oauth_access_spec.rbspec/requests/oauth_pkce_flow_spec.rb
🚧 Files skipped from review as they are similar to previous changes (3)
- spec/requests/api/v1/oauth_access_spec.rb
- config/locales/en.yml
- spec/requests/oauth_pkce_flow_spec.rb
2 participants
Fixes #
Describe the changes you have made in this PR -
Screenshots of the UI changes (If any) -
Code Understanding and AI Usage
Did you use AI assistance (ChatGPT, Claude, Copilot, etc.) to write any part of this code?
If you used AI assistance:
Explain your implementation approach:
Checklist before requesting a review
Note: Please check Allow edits from maintainers if you would like us to assist in the PR.
Summary by CodeRabbit