Feature: SOFIA-local accounts with login (with optional 2fa) - #925
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## staging #925 +/- ##
===========================================
+ Coverage 75.27% 77.28% +2.00%
===========================================
Files 50 54 +4
Lines 1076 1347 +271
===========================================
+ Hits 810 1041 +231
- Misses 266 306 +40 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Kan de emails voor account activatie en wachtwoord resetten die daadwerkelijk worden verstuurd niet inzien, want sidekiq weigert lokaal te draaien door een deprecation, zie hieronder. Kan wel de globale preview zien met de url |
A I see, there are a couple more deprecation issues I will have a look if i can fix a couple in a PR. Are you going to look into fixing this issue or should I look into it, Update, I have gotten the error go away using config.load_defaults 7.0 in aplication.rb |
|
TODO: merge conflicts met staging oplossen en RuboCop linting errors oplossen |
|
How is this coming along? |
|
Apologies for the delay, I got busy again and kept putting it off. It is completely done now :) |
WalkthroughAdds self-contained Sofia (streepsysteem) authentication: OmniAuth identity strategy, new SofiaAccount model with OTP and URL helpers, account activation/reset and OTP enable/disable flows, Devise integration and initializer, mailer/templates, views/modals and JS, DB migration/schema changes, policies, many specs, and three new gems. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as User (Browser)
participant LoginJS as Login View (JS)
participant Callbacks as CallbacksController
participant Sofia as SofiaAccount model
participant User as User model
participant Auth as Devise/Auth
Browser->>LoginJS: submit username/password
LoginJS->>Callbacks: POST /users/auth/identity/callback (auth params)
Callbacks->>Sofia: from_omniauth_inspect(auth)
Sofia->>User: find associated user
alt user found and allowed
Callbacks->>Sofia: check otp_enabled
alt otp_enabled
Callbacks-->>LoginJS: JSON { state: "otp_prompt" }
LoginJS->>Callbacks: POST verification_code
Callbacks->>Sofia: verify otp
alt otp valid
Callbacks->>Auth: sign_in(user)
Callbacks-->>LoginJS: JSON { state: "logged_in", redirect_url }
else otp invalid
Callbacks-->>LoginJS: JSON { state: "otp_prompt", error_message }
end
else otp not enabled
Callbacks->>Auth: sign_in(user)
Callbacks-->>LoginJS: JSON { state: "logged_in", redirect_url }
end
else missing/deactivated
Callbacks-->>LoginJS: JSON { state: "password_prompt", error_message }
end
sequenceDiagram
participant Browser as User (Browser)
participant Activation as Activation Page
participant SofiaCtrl as SofiaAccountsController
participant User as User model
participant Sofia as SofiaAccount model
participant Auth as Devise/Auth
Browser->>Activation: visit /sofia_accounts/activate_account?user_id&token
Activation->>SofiaCtrl: GET activate_account
SofiaCtrl->>User: load user, validate token
alt token valid
SofiaCtrl-->>Activation: render activation form (may request email)
Browser->>SofiaCtrl: POST /sofia_accounts (username/password/[email])
SofiaCtrl->>Sofia: build & validate SofiaAccount
alt valid
Sofia->>User: associate and save
SofiaCtrl->>User: clear activation fields
SofiaCtrl->>Auth: sign_in(user)
SofiaCtrl-->>Browser: redirect to user page
else validation fails
SofiaCtrl-->>Activation: render form with errors
end
else token invalid/expired
SofiaCtrl-->>Activation: show error and offer new activation link
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–75 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
config/initializers/devise.rb (1)
1-29: Fixlocate_conditionslambda and remove unnecessary manualrequirein Devise initializerThe review comment identifies two real and actionable issues:
locate_conditionslambda references undefinedmodelvariable (critical)The lambda
{ model.auth_key => req.params['auth_key'] }will raiseNoMethodErrorwhen OmniAuth Identity strategy invokes it, becausemodelis not defined in the closure. This code path is not exercised by the current tests (they POST directly to the callback endpoint), so the bug would only manifest during actual user login via the identity strategy.The fix requires referencing the
SofiaAccountconstant directly:locate_conditions: ->(req) { { SofiaAccount.auth_key => req.params['auth_key'] } }This evaluates to
{ :username => req.params['auth_key'] }, which omniauth-identity uses to locate the account.Manual
requireof SofiaAccount bypasses Rails autoloadingRails 7.2.3 uses Zeitwerk autoloading, so the explicit
require './app/models/sofia_account'is unnecessary and should be removed. SofiaAccount will be autoloaded when first referenced in the Devise configuration.app/models/user.rb (1)
108-115:sofia_accountis an association, not an attribute.Line 110 adds
'sofia_account'to the whitelist of attributes to preserve during archiving, butsofia_accountis an association (defined on line 28), not a column in theattributeshash. This inclusion has no effect.- self[attribute] = nil unless %w[deleted_at updated_at created_at provider sofia_account id uid].include? attribute + self[attribute] = nil unless %w[deleted_at updated_at created_at provider id uid].include? attribute
🟡 Minor comments (8)
spec/controllers/sofia_accounts_controller/forgot_pasword_spec.rb-1-1 (1)
1-1: Fix typo in filename.The file is named
forgot_pasword_spec.rbbut should beforgot_password_spec.rb(missing 's' in password).Rename the file to match the controller action it tests:
spec/controllers/sofia_accounts_controller/forgot_password_spec.rbspec/controllers/sofia_accounts_controller/enable_otp_spec.rb-86-86 (1)
86-86: Fix typo in describe block.The describe block has a typo: "verication_code" should be "verification_code".
Apply this diff to fix the typo:
- describe 'with wrong verication_code' do + describe 'with wrong verification_code' doapp/views/users/_edit_sofia_account_modal.html.erb-7-7 (1)
7-7: Invalid self-closing button tag.Self-closing tags (
/>) are not valid for<button>elements in HTML5. This may cause rendering issues in some browsers.- <button aria-label="close" class="btn-close" data-bs-dismiss="modal" type="button"/> + <button aria-label="close" class="btn-close" data-bs-dismiss="modal" type="button"></button>spec/mailers/previews/user_mailer_preview.rb-8-12 (1)
8-12: SofiaAccount instance is created but not associated with the user.The
SofiaAccount.new(...)on line 10 creates an instance but doesn't assign it to a variable or wire it touser.sofia_account. Ifforgot_password_emailaccessesuser.sofia_account, this preview will fail or show incorrect data.def forgot_password_mail user = User.new(id: 999_999, provider: 'sofia_account', activation_token: 'mockup_activation_token') - SofiaAccount.new(id: 888_888, user:, username: 'mockup_username') + user.build_sofia_account(id: 888_888, username: 'mockup_username') UserMailer.forgot_password_email(user) endOr if the email doesn't require the association:
- SofiaAccount.new(id: 888_888, user:, username: 'mockup_username')app/controllers/users_controller.rb-184-186 (1)
184-186: Based on my verification, I can now provide a rewritten review comment with proper context:Ensure
providerparameter is intentionally permitted and consider whether it should be restricted to specific values.The
permitted_attributesnow allows settingproviderduring user creation. While the form for sofia_account users (in_new_sofia_account_user_modal.html.erb) explicitly sets it via hidden field, the backend now permits ANY provider value. A treasurer could craft a request to setprovider='amber_oauth2'without the user going through OAuth authentication, potentially bypassing the intended authentication flow. Consider:
- Adding a model-level validation to restrict
providerto allowed values ('sofia_account','amber_oauth2',nil)- Or limiting the controller to only permit
provider='sofia_account'when creating users through the APIspec/controllers/callbacks_controller/sofia_account_spec.rb-12-18 (1)
12-18:sofia_account.passwordwill benilafter persistence due to bcrypt's has_secure_password clearing the virtual attribute.OmniAuth::Identity uses bcrypt with
has_secure_password, which stores the hashed password inpassword_digestand clears the virtualpasswordattribute after the record is saved for security reasons. The test accessingsofia_account.passwordon line 15 will receivenil, which will likely cause the login tests to fail.Store the password before creating the account or pass it directly as a string constant:
let(:password) { 'password1234' } let(:sofia_account) do create(:sofia_account, password: password, password_confirmation: password) end let(:request_params) do { auth_key: sofia_account.username, password: password, # Use the stored password string verification_code: sofia_account.otp_code } endspec/controllers/sofia_accounts_controller/create_spec.rb-62-65 (1)
62-65: Duplicate request call in test.The
requestis already called in thebeforeblock (line 50), so calling it again in theitblock (line 63) sends a second HTTP request. This is likely unintentional.it 'redirects after create' do - request expect(response).to be_redirect endapp/controllers/callbacks_controller.rb-56-65 (1)
56-65: Potential information leakage in failure message.Line 62 appends the raw
omniauth.error.typeto the error message for non-invalid_credentialserrors. This could expose internal error details to users. Consider using a generic fallback message instead.error_message << if request.env['omniauth.error.type'].to_s == 'invalid_credentials' ' De ingevulde gegevens zijn incorrect.' else - " #{request.env['omniauth.error.type']}" + ' Er is een onverwachte fout opgetreden.' end
🧹 Nitpick comments (28)
config/initializers/simple_form_bootstrap.rb (1)
361-375: Review :with_hint_link wrapper – spacing and anchor semanticsNice reuse of the existing vertical pattern. Two small points to consider:
- For consistent spacing with the other wrappers, you may want to add a container class (e.g.
class: 'mb-3') on the wrapper itself; right now this one is the odd one out.- Using
tag: :afor the hint wrapper assumes callers will provide a properhrefviahint_html:; if instead they pass an HTML link in the hint (e.g.hint: link_to(...)), you’ll end up with nested<a>tags, and if they forgethrefyou get a non-interactive anchor from an a11y standpoint. Might be worth:
- Standardizing on plain text hints +
hint_html: { href: ... }for this wrapper, or- Keeping
tagas a non-interactive element (e.g.:span) and letting the hint content itself be a link.Example for the spacing tweak:
- config.wrappers :with_hint_link do |b| + config.wrappers :with_hint_link, class: 'mb-3' do |b|app/controllers/application_controller.rb (1)
40-42: Makenormalize_error_messagesmore defensive and i18n‑friendly (optional)Lowercasing full messages and assuming an array is fine for your current callers, but it’s easy for this to be reused in other contexts where:
full_messagesisnilor contains non‑string values, or- full lowercasing hurts readability (proper nouns, acronyms, multi‑language messages).
A small defensive tweak keeps behavior the same while being safer to reuse:
- def normalize_error_messages(full_messages) - full_messages.map(&:downcase).join(', ') - end + def normalize_error_messages(full_messages) + Array(full_messages) + .map { |m| m.to_s.downcase } + .join(', ') + endIf you later hit i18n issues, consider only lowercasing the first character or leaving casing to the translation layer.
app/javascript/users.js (1)
22-35: Guard against missing/invalid dataset JSON when initializing VueThe added datasets (
sofiaAccountUsers,notActivatedUsers,deactivatedUsers) are parsed unconditionally:const sofia_account_users = JSON.parse(element.dataset.sofiaAccountUsers); const not_activated_users = JSON.parse(element.dataset.notActivatedUsers); const deactivated_users = JSON.parse(element.dataset.deactivatedUsers);This is fine as long as the view always sets these
data-*attributes to valid JSON. If any of them are omitted or rendered as an empty string, this will throw and prevent the users table from mounting.Consider defensively defaulting to an empty array on missing values:
- const sofia_account_users = JSON.parse(element.dataset.sofiaAccountUsers); + const sofia_account_users = JSON.parse(element.dataset.sofiaAccountUsers || '[]'); ... - const not_activated_users = JSON.parse(element.dataset.notActivatedUsers); - const deactivated_users = JSON.parse(element.dataset.deactivatedUsers); + const not_activated_users = JSON.parse(element.dataset.notActivatedUsers || '[]'); + const deactivated_users = JSON.parse(element.dataset.deactivatedUsers || '[]');or ensure the corresponding view always renders these attributes as valid JSON (e.g.
'[]') even when empty.db/seeds.rb (1)
43-49: Duplicated seededRolerecords perrole_type; verify this is intentionalSeeding both group-specific and “global” roles:
Role.create(role_type: :treasurer, group_uid: 4) ... Role.create(role_type: :treasurer)means there are now two
Rolerows for eachrole_type. That’s fine if callers always scope bygroup_uid(or explicitly by “global” vs “per-group” roles), but it can become ambiguous for code like:Role.find_by(role_type: :treasurer)which will now hit whichever record the DB returns first.
If the intent is to have distinct global vs group-scoped roles, consider:
- Auditing lookups to ensure they always filter by
group_uidwhere appropriate, or- Introducing explicit semantics (e.g. a scope or flag) so “global” treasurer is uniquely identifiable.
Otherwise, it might be safer to seed only one record per
role_typeor to enforce uniqueness at the model/db level.app/policies/user_policy.rb (1)
26-28: LGTM with a minor clarity suggestion.The authorization logic correctly ensures only the authenticated user can update their own Sofia account. For explicit clarity, consider using
record.id:def update_with_sofia_account? - record == user && User.active.sofia_account.exists?(id: record) + record == user && User.active.sofia_account.exists?(id: record.id) endThis is functionally equivalent but makes the intent clearer.
app/views/sofia_accounts/forgot_password_view.html.erb (1)
12-12: Remove redundantmethod: :poston submit button.The form already declares
method: :poston line 8, making themethod: :poston the submit button redundant.Apply this diff:
- <%= f.button :submit, 'Link aanvragen', method: :post, class: 'btn btn-primary mt-1' %> + <%= f.button :submit, 'Link aanvragen', class: 'btn btn-primary mt-1' %>app/views/sofia_accounts/reset_password_view.html.erb (1)
12-12: Remove redundantmethod: :patchon submit button.The form already declares
method: :patchon line 8, making themethod: :patchon the submit button redundant.Apply this diff:
- <%= f.button :submit, 'Opslaan', method: :patch, class: 'btn btn-primary' %> + <%= f.button :submit, 'Opslaan', class: 'btn btn-primary' %>spec/controllers/sofia_accounts_controller/disable_otp_spec.rb (1)
19-22: Consider verifying the redirect location.The test confirms a 302 status but doesn't verify where the user is redirected after successfully disabling OTP. Consider adding a redirect expectation for completeness.
it 'updates sofia_account' do expect(request.status).to eq 302 + expect(response).to redirect_to(user_path(sofia_account.user)) expect(sofia_account.otp_enabled).to be false endspec/support/mailer_matcher.rb (1)
3-8: Matcher doesn't verify the delivery method is called.Using
allowon line 7 means the test won't fail ifdeliver_later(or the specifiedmailer_when) is never invoked. Consider usingexpectto assert the delivery actually happens.RSpec::Matchers.define :send_email do |mailer_action, mailer_when, mailer_args| match do |mailer_class| message_delivery = instance_double(ActionMailer::MessageDelivery) expect(mailer_class).to receive(mailer_action).with(mailer_args).and_return(message_delivery) # rubocop:disable RSpec/StubbedMock, RSpec/MessageSpies - allow(message_delivery).to receive(mailer_when) + expect(message_delivery).to receive(mailer_when) # rubocop:disable RSpec/MessageSpies end endspec/controllers/users_controller/update_with_sofia_account_spec.rb (2)
7-7: Unused variablesofia_account_attributes.This
letis defined but never referenced in the test. Consider removing it or using it in the request params if that was the intent.- let(:sofia_account_attributes) { { username: 'AAAA' } }
5-6: Confusing test setup with duplicate sofia_account creation.The user is created with the
:sofia_accounttrait (line 5), but then a separatesofia_accountis created withuser:association (line 6). IfUser has_one :sofia_account, this creates ambiguity about which record is the "real" association. Consider using only one approach.- let(:user) { create(:user, :sofia_account, name: 'Old name') } - let(:sofia_account) { create(:sofia_account, user:, username: 'Old username') } + let(:user) { create(:user, name: 'Old name') } + let(:sofia_account) { create(:sofia_account, user:, username: 'Old username') }spec/controllers/sofia_accounts_controller/new_activation_link_spec.rb (2)
47-51: Consider usinguser.reloadinstead ofuser.dupfor database state verification.The assertion
expect(user.dup.attributes).to eq old_user.attributescompares in-memory state. If the intent is to verify the database record wasn't modified, useuser.reloadbefore comparison. The same pattern appears in other contexts (lines 65, 82, 99, 116).it 'shows error message and does not send email' do - expect(user.dup.attributes).to eq old_user.attributes + user.reload + expect(user.attributes).to eq old_user.attributes expect(assigns(:message)).to match(/uw account heeft geen emailadres/) expect(enqueued_jobs.size).to eq(0) end
92-92: Consider using a more robust non-existent user ID.
User.count + 1assumes sequential IDs without gaps. A safer approach would be using a clearly invalid ID.- request_params[:user_id] = User.count + 1 + request_params[:user_id] = -1spec/controllers/sofia_accounts_controller/activate_account_spec.rb (1)
1-61: Consider adding more comprehensive test coverage for the activation flow.The current tests only verify that the activation form renders successfully. Consider adding tests for:
- Successful account activation with valid token
- Rejected activation with expired token
- Rejected activation with invalid token
- Rejected activation with mismatched user_id and token
These tests would improve confidence in the security and correctness of the activation flow.
app/mailers/user_mailer.rb (1)
20-27: Consider defensive coding for missing SofiaAccount association.The method assumes
user.sofia_accountexists. If this mailer is called with a user lacking a SofiaAccount, it will raise a NoMethodError on Line 22-23.Consider adding a guard clause:
def forgot_password_email(user) @user = user + raise ArgumentError, "User must have a SofiaAccount" unless user.sofia_account @username = user.sofia_account.username @reset_password_url = user.sofia_account.reset_password_url(@user.activation_token) @forgot_password_url = SofiaAccount.forgot_password_url @call_to_action = { text: 'Wachtwoord herstellen!', url: @reset_password_url } mail to: user.email, subject: 'Wachtwoordherstel voor het streepsysteem van uw vereniging' endAlternatively, if the caller always ensures the association exists, this is fine as-is.
app/views/sofia_accounts/login.html.erb (2)
27-27: Consider using standard event handling instead ofjavascript:protocol.Line 27 uses
formaction='javascript:authenticate_login();'which is non-standard and can cause issues with Content Security Policy (CSP).Consider using a standard event handler instead:
- <button class="btn btn-primary" type="submit" formaction='javascript:authenticate_login();' id="login_submit_button">Inloggen</button> + <button class="btn btn-primary" type="button" onclick="authenticate_login();" id="login_submit_button">Inloggen</button>Or better, use addEventListener in the script section:
- <button class="btn btn-primary" type="submit" formaction='javascript:authenticate_login();' id="login_submit_button">Inloggen</button> + <button class="btn btn-primary" type="button" id="login_submit_button">Inloggen</button>And in the script:
document.querySelector('#login_submit_button').addEventListener('click', authenticate_login);The same applies to Line 42 with the OTP button.
81-84: Use textContent instead of innerHTML for consistency and security.Line 83 uses
innerHTMLto set an error message. While the current content is static, usingtextContentis safer and more consistent with Line 78.Apply this diff:
- document.querySelector("#authenticate_flash_message").innerHTML = "Inloggen mislukt door een error. Herlaad de pagina en probeer het nog een keer. <br/><i>Werkt het na een paar keer proberen nog steeds niet? Neem dan contact op met de ICT-commissie.</i>"; + document.querySelector("#authenticate_flash_message").textContent = "Inloggen mislukt door een error. Herlaad de pagina en probeer het nog een keer. Werkt het na een paar keer proberen nog steeds niet? Neem dan contact op met de ICT-commissie.";This removes the HTML formatting but improves security by eliminating any potential for injection.
app/policies/sofia_account_policy.rb (1)
2-4: Consider simplifying the authorization check.The
User.exists?(id: record.user)check on Line 3 appears redundant. Ifrecord.useris a loaded association and equalsuser, the User record necessarily exists in the database.Unless there's a specific concern about race conditions or stale data, consider simplifying to:
def update? - record.user == user && User.exists?(id: record.user) + record.user == user && user.present? endOr simply:
def update? - record.user == user && User.exists?(id: record.user) + record.user == user endIf the
User.exists?check is intentionally defensive against specific edge cases, consider adding a comment explaining why.app/views/users/_edit_sofia_account_modal.html.erb (1)
90-114: Addrel="noopener noreferrer"to external links.External links with
target='_blank'should includerel="noopener noreferrer"to prevent potential reverse tabnabbing attacks. While modern browsers mitigate this by default, it's still a recommended best practice.<a href='https://play.google.com/store/apps/details?id=com.google.android.apps.authenticator2' target='_blank' + rel='noopener noreferrer' >Apply similarly to the Apple and Microsoft authenticator links.
app/views/users/show.html.erb (1)
124-134: Remove or document commented-out code.This
=begin/=endblock comments out iDEAL payment and related content. If this feature is intentionally disabled, consider removing the code entirely or adding a TODO/comment explaining why it's preserved and when it should be restored.spec/models/user_spec.rb (1)
492-495: Fragile date comparison using.day.Comparing
activation_token_valid_till.dayto5.days.from_now.daycan fail around month boundaries (e.g., if today is the 28th and 5 days later is the 2nd of next month). Usebe_withinfor more robust time comparison:it do expect(user.activation_token_valid_till).not_to be_nil - expect(user.activation_token_valid_till.day).to eq 5.days.from_now.day + expect(user.activation_token_valid_till).to be_within(1.minute).of(5.days.from_now) endapp/views/users/index.html.erb (2)
58-58: Minor: Missing space in ERB tag.For consistency with the rest of the file and ERB conventions, add a space after
<%:- <%end %> + <% end %>
65-65: Minor: Missing space in ERB tag.Same issue as line 58:
- <%end %> + <% end %>app/controllers/callbacks_controller.rb (1)
14-20: Redundant SofiaAccount lookup.
User.from_omniauth_inspect(auth)already fetches theSofiaAccountviaSofiaAccount.find(auth.uid)(seeapp/models/user.rb, lines 133-135). The subsequentSofiaAccount.find_by(user_id: user.id)on line 20 is a redundant database query.Consider refactoring
from_omniauth_inspectto return both the user and sofia_account, or cache the sofia_account on the user object to avoid the extra query:def identity - user = User.from_omniauth_inspect(request.env['omniauth.auth']) + auth = request.env['omniauth.auth'] + sofia_account = SofiaAccount.find_by(id: auth.uid) + user = sofia_account&.user if user.persisted? if user.deactivated render(json: { state: 'password_prompt', error_message: 'Uw account is gedeactiveerd, dus inloggen is niet mogelijk.' }) else - check_identity_with_user(user, SofiaAccount.find_by(user_id: user.id)) + check_identity_with_user(user, sofia_account) endspec/controllers/sofia_accounts_controller/create_spec.rb (1)
303-319: Fragile test setup for non-existent user scenario.Using
User.countas a non-existentuser_idis fragile—if the count happens to equal an existing user's id (e.g., after deletions), the test could fail unexpectedly or pass for the wrong reason.before do - request_params[:user_id] = User.count + request_params[:user_id] = User.maximum(:id).to_i + 1 old_user request user.reload endapp/models/user.rb (2)
40-41: Clarify purpose of callingagein after_save.Line 41 calls
agebut discards the return value. If this is intended for caching or side effects, add a comment. If it's dead code, remove it.
20-23: Complex active scope - consider readability.The scope logic is correct but the raw SQL with subquery could be simplified using Rails query methods for better maintainability.
scope :active, lambda { sofia_user_ids = SofiaAccount.select(:user_id) where(deactivated: false) .where("provider IS NULL OR provider != 'sofia_account' OR id IN (?)", sofia_user_ids) }Note: The static analysis warning about "hardcoded passphrase" is a false positive—
'sofia_account'is simply a provider identifier string, not a credential.app/controllers/sofia_accounts_controller.rb (1)
108-115: Consider checkingupdatereturn value and adding error handling.While failure here leaves the account in a more secure state (OTP remains enabled), the user should be informed if the operation fails.
def disable_otp @sofia_account = SofiaAccount.find(params[:id]) authorize @sofia_account - @sofia_account.update(otp_enabled: false) - - redirect_to user_path(@sofia_account.user_id) + if @sofia_account.update(otp_enabled: false) + redirect_to user_path(@sofia_account.user_id), flash: { success: 'Two-factor-authenticatie uitgezet.' } + else + redirect_to user_path(@sofia_account.user_id), flash: { error: 'Two-factor-authenticatie uitzetten mislukt.' } + end end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (57)
Gemfile(3 hunks)app/assets/stylesheets/application.scss(1 hunks)app/controllers/application_controller.rb(2 hunks)app/controllers/callbacks_controller.rb(1 hunks)app/controllers/sofia_accounts_controller.rb(1 hunks)app/controllers/users_controller.rb(3 hunks)app/javascript/components/user/UsersTable.vue(3 hunks)app/javascript/users.js(2 hunks)app/mailers/user_mailer.rb(1 hunks)app/models/role.rb(1 hunks)app/models/sofia_account.rb(1 hunks)app/models/user.rb(5 hunks)app/policies/sofia_account_policy.rb(1 hunks)app/policies/user_policy.rb(1 hunks)app/views/partials/_flash.html.erb(1 hunks)app/views/partials/_login_prompt.html.erb(1 hunks)app/views/sofia_accounts/activate_account.html.erb(1 hunks)app/views/sofia_accounts/forgot_password_view.html.erb(1 hunks)app/views/sofia_accounts/login.html.erb(1 hunks)app/views/sofia_accounts/new_activation_link.html.erb(1 hunks)app/views/sofia_accounts/reset_password_view.html.erb(1 hunks)app/views/user_mailer/account_creation_email.html.erb(1 hunks)app/views/user_mailer/forgot_password_email.html.erb(1 hunks)app/views/user_mailer/new_activation_link_email.html.erb(1 hunks)app/views/users/_edit_sofia_account_modal.html.erb(1 hunks)app/views/users/_new_sofia_account_user_modal.html.erb(1 hunks)app/views/users/index.html.erb(2 hunks)app/views/users/show.html.erb(3 hunks)config/initializers/devise.rb(2 hunks)config/initializers/simple_form_bootstrap.rb(1 hunks)config/locales/nl.yml(1 hunks)config/routes.rb(2 hunks)db/migrate/20240413094147_create_sofia_accounts.rb(1 hunks)db/schema.rb(7 hunks)db/seeds.rb(1 hunks)spec/controllers/callbacks_controller/sofia_account_spec.rb(1 hunks)spec/controllers/sofia_accounts_controller/activate_account_spec.rb(1 hunks)spec/controllers/sofia_accounts_controller/create_spec.rb(1 hunks)spec/controllers/sofia_accounts_controller/disable_otp_spec.rb(1 hunks)spec/controllers/sofia_accounts_controller/enable_otp_spec.rb(1 hunks)spec/controllers/sofia_accounts_controller/forgot_pasword_spec.rb(1 hunks)spec/controllers/sofia_accounts_controller/new_activation_link_spec.rb(1 hunks)spec/controllers/sofia_accounts_controller/reset_password_spec.rb(1 hunks)spec/controllers/sofia_accounts_controller/update_password_spec.rb(1 hunks)spec/controllers/users_controller/index_spec.rb(1 hunks)spec/controllers/users_controller/show_spec.rb(5 hunks)spec/controllers/users_controller/update_spec.rb(1 hunks)spec/controllers/users_controller/update_with_sofia_account_spec.rb(1 hunks)spec/factories/sofia_account.rb(1 hunks)spec/factories/user.rb(1 hunks)spec/mailers/previews/user_mailer_preview.rb(1 hunks)spec/models/role_spec.rb(0 hunks)spec/models/sofia_account_spec.rb(1 hunks)spec/models/user_spec.rb(7 hunks)spec/rails_helper.rb(1 hunks)spec/support/devise_helper.rb(1 hunks)spec/support/mailer_matcher.rb(1 hunks)
💤 Files with no reviewable changes (1)
- spec/models/role_spec.rb
🧰 Additional context used
🧬 Code graph analysis (13)
app/policies/user_policy.rb (1)
app/javascript/user.js (1)
user(12-12)
spec/controllers/users_controller/show_spec.rb (1)
app/controllers/sofia_accounts_controller.rb (1)
create(10-28)
spec/controllers/sofia_accounts_controller/new_activation_link_spec.rb (1)
app/controllers/sofia_accounts_controller.rb (1)
create(10-28)
spec/controllers/callbacks_controller/sofia_account_spec.rb (1)
spec/support/devise_helper.rb (1)
signed_in?(2-4)
spec/models/user_spec.rb (3)
app/controllers/sofia_accounts_controller.rb (1)
create(10-28)app/models/user.rb (1)
archive!(108-115)db/migrate/20240413094147_create_sofia_accounts.rb (2)
change(1-22)change(2-21)
app/controllers/callbacks_controller.rb (1)
app/models/user.rb (1)
from_omniauth_inspect(133-136)
spec/models/sofia_account_spec.rb (1)
app/models/sofia_account.rb (4)
activate_account_url(13-17)new_activation_link_url(19-23)forgot_password_url(25-28)reset_password_url(30-34)
app/controllers/users_controller.rb (3)
app/policies/user_policy.rb (1)
show?(14-16)app/models/user.rb (2)
calculate_credits(148-153)treasurer?(86-88)config/application.rb (1)
config(10-93)
app/policies/sofia_account_policy.rb (1)
app/javascript/user.js (1)
user(12-12)
spec/controllers/sofia_accounts_controller/create_spec.rb (5)
webpack.config.js (1)
require(5-5)app/controllers/users_controller.rb (2)
create(76-87)update(89-100)app/controllers/sofia_accounts_controller.rb (1)
create(10-28)app/javascript/user.js (1)
user(12-12)app/models/invoice.rb (1)
db/migrate/20240413094147_create_sofia_accounts.rb (1)
app/controllers/users_controller.rb (1)
index(6-32)
app/models/user.rb (2)
app/policies/application_policy.rb (1)
scope(37-39)app/mailers/user_mailer.rb (2)
account_creation_email(1-28)account_creation_email(2-9)
spec/controllers/users_controller/update_spec.rb (1)
app/models/role.rb (1)
name(8-16)
🪛 ast-grep (0.40.0)
app/models/user.rb
[warning] 20-21: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: where(deactivated: false).where('(provider IS NULL OR provider != ?) OR
(provider = ? AND id IN (?))', 'sofia_account', 'sofia_account', SofiaAccount.select('user_id'))
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
🪛 Gitleaks (8.29.1)
app/models/user.rb
[high] 36-36: 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). (1)
- GitHub Check: Build
🔇 Additional comments (55)
app/controllers/application_controller.rb (1)
4-4: <!-- [scratchpad_start]
[task final_conclusion][observations]
- Gemfile line 25:
omniauth-rails_csrf_protection,~> 1.0is installed
- This gem provides CSRF protection for OmniAuth callbacks
- Handles POST callbacks to OmniAuth safely
- Devise
~>4.9.4is installed- CallbacksController correctly inherits from
Devise::OmniauthCallbacksController- No omniauth.rb initializer, but amber_oauth2.rb exists
[analysis]
The review comment's concerns are INVALID:
OmniAuth callbacks (Amber, Identity, SOFIA):
- Protected by omniauth-rails_csrf_protection gem
- Devise::OmniauthCallbacksController handles CSRF properly
- CallbacksController does NOT inherit ApplicationController, so it doesn't get the global protect_from_forgery
- The gem takes care of CSRF for POST callbacks
- NOT at risk from global protect_from_forgery
JSON/API endpoints:
- No JSON API base class found
- PaymentsController.callback is GET (user redirect, not webhook)
- No external webhooks found
- NOT at risk
The global protect_from_forgery config is SAFE:
- OmniAuth is properly protected by the dedicated gem
- No JSON APIs that would break
- No external webhooks
Conclusion: The review comment's verification request and concerns are INCORRECT and based on incomplete understanding of:
- How CallbacksController inheritance works (doesn't get ApplicationController CSRF)
- That omniauth-rails_csrf_protection is installed and configured
- The actual HTTP methods used in the application
The normalize_error_messages helper is fine - simple and safe.
The CSRF configuration itself is secure and appropriate.
[done]
Mark as INCORRECT_REVIEW_COMMENT
[scratchpad_start] -->CSRF protection is properly configured and OmniAuth/callback endpoints are safe.
The review comment's concerns are based on incomplete analysis. Here's what the verification found:
OmniAuth/Amber/SOFIA callbacks are not at risk. CallbacksController inherits from
Devise::OmniauthCallbacksController, not ApplicationController, so it does not inherit the globalprotect_from_forgerysetting. Additionally,omniauth-rails_csrf_protectiongem (v1.0) is installed in the Gemfile, which provides dedicated CSRF handling for OmniAuth POST callbacks.No JSON/API controllers will break. PaymentsController.callback is routed as GET (a user redirect after payment, not a webhook), and no external webhook endpoints or JSON-only API base classes were found in the codebase.
The normalize_error_messages helper (lines 40–42) is safe. The implementation (
full_messages.map(&:downcase).join(', ')) is straightforward, defensive, and appropriate for error message formatting.The global
protect_from_forgery with: :exception, prepend: trueconfiguration is secure and does not introduce the risks described in the original comment.Likely an incorrect or invalid review comment.
app/models/role.rb (1)
4-4: RelaxedRolevalidation matches new global roles; check callers usinggroup_uidAllowing
group_uidto be nil is consistent with the new seed roles and migration. Just make sure any queries or logic that previously assumedgroup_uidis always present (e.g.Role.find_by(role_type: ...)without agroup_uidcondition) behave correctly now that there can be “global” roles withgroup_uid: nil.app/views/partials/_login_prompt.html.erb (1)
5-29: Login prompt nicely separates Amber vs Sofia account flowsThe conditional list-group layout clearly distinguishes Amber SSO from local Sofia (streepsysteem) accounts, and the
amber_api_url.present?guard ensures the Amber option is only shown when configured. The copy and paths look consistent with the new Sofia login flow.app/javascript/components/user/UsersTable.vue (1)
3-4: Solid handling of empty user lists and safertotalcalculationUsing
v-if="users.length > 0"with av-elseempty state avoids rendering an empty table and gives a clear “no users” message. Updatingtotalto:return this.users.map(user => user.credit) .reduce((current, credit) => parseFloat(current) + parseFloat(credit), 0);prevents
reducefrom throwing on empty arrays and ensures the footer still shows€ 0.00when there are no users. This aligns well with the new Sofia account cohorts where tables can legitimately be empty.Also applies to: 44-48, 83-85
config/locales/nl.yml (1)
35-45: SofiaAccount attribute translations look consistent and completeThe new
activerecord.attributes.sofia_accountentries cover the expected fields (username, email, passwords, activation/verification tokens) and will keep SofiaAccount forms and errors properly localized. Nesting undernl.activerecord.attributes.sofia_accountis correct for Rails’ i18n conventions.spec/rails_helper.rb (1)
36-36: LGTM!The inclusion of
DeviseHelperfor request specs follows the existing pattern and will be properly loaded via the auto-require on line 27.app/assets/stylesheets/application.scss (1)
88-100: LGTM!The styling additions are well-structured:
.sofia-account-inputprovides consistent form input sizing.qr-codeconstrains QR code images appropriately- The
mainpadding correctly matches the footer height (48px) to prevent content overlapapp/views/sofia_accounts/new_activation_link.html.erb (1)
1-10: I'll help you verify the review comment. Let me start by examining the controller to understand what@messagecontains and whether it needs special handling.
<function_calls>
#!/bin/bashFind and examine @message in the sofia_accounts controller
find . -path '*/controllers/sofia_accounts_controller.rb' -type f 2>/dev/null | head -1 | xargs grep -n '@message' -B 3 -A 3 2>/dev/null || echo "File not found, searching more broadly..."
</function_calls>
<function_calls>
#!/bin/bashSearch for sofia_accounts controller more broadly
fd -type f -name 'sofia_accountscontroller*' 2>/dev/null
</function_calls>
<function_calls>
#!/bin/bashSearch for new_activation_link action and @message assignment
rg '@message' --type ruby -B 5 -A 5 | head -100
</function_calls>db/migrate/20240413094147_create_sofia_accounts.rb (1)
19-20: I'll help you verify this review comment about the roles table modifications. Let me start by examining the migration file and understanding the context.
<function_calls>
cat -n db/migrate/20240413094147_create_sofia_accounts.rb
</function_calls>
<function_calls>
wc -l db/migrate/20240413094147_create_sofia_accounts.rb
</function_calls>Now let me search for usages of the composite index on
role_typeandgroup_uid:
<function_calls>
#!/bin/bashSearch for queries combining role_type and group_uid
rg -n 'role_type.*group_uid|group_uid.*role_type' --type ruby -C 2
</function_calls>Let me also check for direct usage of
group_uidcolumn to see if code depends on it being NOT NULL:
<function_calls>
#!/bin/bashSearch for group_uid usage patterns
rg -n '.group_uid\s*[!=><]|where.*group_uid|group_uid.*where' --type ruby -C 2 | head -100
</function_calls>Let me search for scope definitions that might use this index:
<function_calls>
#!/bin/bashLook for Role model and scopes
fd 'roles?.rb' --type f | xargs rg -l 'scope|class Role' 2>/dev/null | head -5
</function_calls>app/views/user_mailer/account_creation_email.html.erb (1)
1-14: LGTM!The email template correctly presents the activation workflow in Dutch, includes the necessary links, and clearly communicates the 5-day validity period for activation tokens as specified in the PR objectives.
app/views/users/_new_sofia_account_user_modal.html.erb (1)
1-27: LGTM!The modal correctly structures the Sofia account user creation form with appropriate required fields, hidden provider selection, and good UX practices (disable_with on submit). The Dutch labels are consistent with the application's localization.
spec/controllers/sofia_accounts_controller/disable_otp_spec.rb (1)
1-64: Test coverage looks comprehensive.The test properly verifies authorization boundaries: only the account owner can disable OTP, while other users (including privileged roles like treasurer and main_bartender) are correctly forbidden.
spec/factories/user.rb (1)
29-31: LGTM!The new
:sofia_accounttrait is consistent with existing provider-related traits (:from_amber,:manual) and follows the factory's conventions.spec/controllers/users_controller/show_spec.rb (1)
6-6: LGTM!The test additions for Sofia account users mirror the existing test structure for Amber and manual users, ensuring consistent authorization behavior across all user types. The authorization policy (treasurer can view, other roles cannot) aligns with the expected security model.
Also applies to: 11-11, 30-35, 53-58, 76-81, 99-104
app/views/sofia_accounts/reset_password_view.html.erb (1)
8-8: I encountered a technical issue accessing the repository directly. However, I can provide verification guidance based on Rails security best practices. Let me provide a rewritten review comment that addresses the concern with practical context:
Verify that
activation_tokenis configured in Rails'filter_parametersto prevent logging in application logs.The activation token is passed as a URL parameter, which may be logged in web server access logs or application logs. This is a security best practice that should be verified. Check that
config/initializers/filter_parameters.rborconfig/application.rbincludesactivation_tokenin theconfig.filter_parameterslist. For example:config.filter_parameters += [:activation_token]If this filtering is not configured, sensitive tokens could appear in Rails logs. Additionally, verify that the web server (nginx, Apache, etc.) is also configured to filter sensitive parameters from access logs.
app/views/sofia_accounts/forgot_password_view.html.erb (1)
10-10: Based on my verification, I can now provide the final assessment:The custom
nameattribute is necessary and correctly implemented.The form is initialized with
simple_form_for :sofia_account(line 8), which would normally cause Simple Form to nest the parameter assofia_account[username]. However, the controller'sforgot_passwordaction explicitly expectsparams.require(:username)— a bare, non-nested parameter. The customname: 'username'attribute is intentional and required to override the default nesting behavior and provide the controller with the parameter structure it expects.Gemfile (1)
3-3: Based on my verification of the three new gems added to support the SOFIA authentication feature, here is my analysis:Version Status:
active_model_otp(latest: 2.3.4) - Gemfile constraint~> 2.3, >= 2.3.1will correctly pull 2.3.4 ✓omniauth-identity(latest: 3.1.5) - Gemfile constraint~> 3.0, >= 3.0.9will pull latest 3.0.x versions ✓rqrcode(latest: 3.1.1) - Gemfile constraint~> 2.2is intentionally pinned to major version 2 ✓Security Advisory Results:
All three gems were checked against the OSV (Open Source Vulnerabilities) database, which aggregates vulnerability data across multiple sources. No known security vulnerabilities were found for any of the three gems.
All three gems are secure and at current versions appropriate for their semantic version constraints.
app/views/user_mailer/forgot_password_email.html.erb (1)
1-18: LGTM!The email template is well-structured with correct ERB syntax for all dynamic URLs and the username display.
spec/support/devise_helper.rb (1)
1-13: LGTM!The helper correctly extracts the signed-in user from Warden's session data for test assertions.
app/views/sofia_accounts/activate_account.html.erb (1)
1-20: LGTM!The activation form is well-structured with appropriate required fields and conditional email input. The form correctly passes
user_idandactivation_tokenas URL parameters.spec/controllers/users_controller/index_spec.rb (1)
10-20: LGTM!The test setup properly creates the various user types needed to verify the index action's authorization and filtering behavior across different roles.
spec/factories/sofia_account.rb (1)
1-11: LGTM!The factory is well-structured with appropriate use of Faker for generating test data. Password length requirements (min 12 characters) meet security best practices.
spec/controllers/sofia_accounts_controller/enable_otp_spec.rb (2)
1-30: LGTM!The test setup and success scenario are well-structured, with appropriate use of factories and clear assertions validating the OTP enablement flow.
32-69: LGTM!Authorization tests comprehensively verify that only the account owner can enable OTP, correctly denying access to other users and privileged roles.
spec/controllers/users_controller/update_spec.rb (1)
12-49: LGTM! Good refactoring for test clarity.The introduction of
action_userimproves test readability by clearly distinguishing between the user being updated and the user performing the action. The split between "as user themselves" and "as another user" scenarios enhances test organization.spec/controllers/sofia_accounts_controller/forgot_pasword_spec.rb (2)
17-33: LGTM!The success flow test properly validates email delivery and token generation with appropriate job queue management.
35-87: LGTM!Error cases are comprehensively tested with proper validation of error messages and verification that no emails are sent or state changes occur.
app/views/sofia_accounts/login.html.erb (1)
48-86: LGTM on CSRF token handling.The manual CSRF token inclusion in the fetch request (Lines 52-57) properly compensates for the disabled form authenticity token and follows Rails best practices.
app/policies/sofia_account_policy.rb (1)
6-20: LGTM!The explicit delegation methods provide clear intent at call sites while maintaining a single source of truth for authorization logic. This design makes future customization straightforward.
spec/models/sofia_account_spec.rb (1)
1-99: Well-structured model spec with comprehensive coverage.The tests cover essential validation scenarios (username presence/empty, password length requirements, uniqueness constraints) and URL helper methods. The use of
build_stubbedfor most tests andcreateonly when database uniqueness checks are needed is appropriate.app/views/users/show.html.erb (1)
72-86: Conditional button logic is well-structured.The priority order correctly shows the Sofia account settings modal for Sofia account owners, while preserving treasurer access to user management for non-Sofia users.
spec/controllers/sofia_accounts_controller/update_password_spec.rb (1)
1-191: Comprehensive test coverage for password update scenarios.The spec covers authorization correctly (owner-only access, 403 for others including treasurer/main-bartender) and all key validation failure paths. Good use of flash message assertions in Dutch matching the locale.
spec/controllers/sofia_accounts_controller/reset_password_spec.rb (1)
1-177: Thorough test coverage for password reset flow.The spec correctly tests the unauthenticated password reset endpoint with comprehensive scenarios: valid reset (clears tokens), expired/invalid/missing tokens, and password validation errors. The assertions verify both the account state and user token fields are handled correctly.
app/controllers/users_controller.rb (2)
43-62: LGTM on the QR code generation logic.The conditional handling of
@sofia_accountpresence and the QR code generation usingRQRCodewith the provisioning URI is well-structured. The fallback toSofiaAccount.newfor users without an account ensures the view has a consistent object to work with.
146-161: LGTM on the update_with_sofia_account action.The dual authorization pattern (authorizing both
@userand@sofia_account) properly gates access. The conditional permit logic correctly restrictsnameanddeactivatedattributes to treasurers while allowing regular users to update their email and username.config/routes.rb (1)
53-68: LGTM on sofia_accounts routing structure.The routes correctly separate collection-level actions (login, activation, forgot password) from member-level actions (reset password, OTP management). The distinction between GET views and POST/PATCH actions for forgot_password and reset_password follows RESTful conventions.
spec/controllers/callbacks_controller/sofia_account_spec.rb (2)
40-56: Good test coverage for the non-OTP login flow.The tests properly verify both the session state (
signed_in?) and the JSON response structure includingstate,error_message, andredirect_url. The test assertions align with the expected Dutch locale messages.
76-148: Comprehensive OTP test scenarios.The OTP-enabled login tests cover all critical paths: valid credentials with correct OTP, wrong password, missing OTP code (triggering
otp_promptstate), and wrong OTP code. This ensures robust coverage of the 2FA flow.spec/models/user_spec.rb (3)
559-571: LGTM on the after_create email test.The test correctly verifies that
UserMailer.account_creation_emailis triggered only for sofia_account users. The use ofclear_enqueued_jobsin theafterblock ensures test isolation.
506-557: Good coverage of the after_save archive! callback.The tests comprehensively cover all conditional branches: triggering
archive!on deactivation (both at creation and update), not triggering when already deactivated without changes, and not triggering when not deactivated. This ensures the callback behaves correctly in all scenarios.
441-452: Review comment is incorrect. The explicitSofiaAccountcreation is necessary and not redundant.The
:sofia_accounttrait only sets theproviderattribute on the User model; it does not create an associatedSofiaAccountrecord. The explicitcreate(:sofia_account, user:)on line 446 is therefore necessary to establish the actual association that the test verifies will be destroyed. This is intentional test setup, not a duplicate.app/views/users/index.html.erb (1)
13-13: LGTM on the expanded data payload.The data attributes properly expose all five user cohorts (
manual_users,sofia_account_users,amber_users,not_activated_users,deactivated_users) to the Vue components, aligning with the controller's JSON generation.app/controllers/callbacks_controller.rb (1)
36-38: Raw HTML in error message - ensure frontend escapes properly.The error message contains raw HTML (
<br/>,<i>). Verify the frontend renders this safely (e.g., usingv-htmlin Vue ordangerouslySetInnerHTMLin React with proper sanitization) to prevent XSS if any user-controlled data could ever be interpolated into similar messages.app/models/sofia_account.rb (1)
13-34: URL helper methods look good.The URL builders correctly use
Rails.application.config.action_mailer.default_url_optionsfor consistent URL generation across environments. The use ofURI::Generic.buildis appropriate for constructing URLs with query parameters.spec/controllers/sofia_accounts_controller/create_spec.rb (1)
1-338: Comprehensive test coverage for account creation flow.The test file covers a wide range of scenarios including:
- Happy paths (with/without email for user)
- Validation failures (missing username, password, invalid email)
- Security cases (expired/wrong activation token, deactivated user, already activated)
This thorough coverage ensures the activation flow is robust.
db/schema.rb (2)
159-170: Sofia accounts table structure looks appropriate.The table correctly includes:
usernameandpassword_digestfor authenticationuser_idforeign key linking to usersotp_secret_keyandotp_enabledfor 2FA supportdeleted_atfor soft deletes- Unique index on
usernameHowever, as noted in the model review, consider adding a unique index on
user_idto enforce the one-to-one relationship at the database level.
188-197: I'll help you verify the malformed column definition in the versions table. Let me search for the migrations that touch this table to understand the source of the issue.
<function_calls>
#!/bin/bashFind migrations that touch the versions table
fd -e rb db/migrate/ | head -20
</function_calls><function_calls>
#!/bin/bashSearch for migrations mentioning "versions" table
rg -n "versions" db/migrate/ --type ruby -B2 -A5 | head -150
</function_calls>app/models/user.rb (1)
14-15: Email validation and association setup look correct.The email format validation with
allow_blank: truecombined with presence validation only whensofia_account.present?correctly enforces that activated Sofia accounts must have an email. Thedependent: :destroyensures cleanup when users are deleted.Also applies to: 28-29
app/controllers/sofia_accounts_controller.rb (7)
2-8: LGTM!Simple redirect helpers for OmniAuth identity flow integration.
10-28: LGTM!The account creation flow is well-structured with proper parameter validation, error normalization, and role-based redirect logic.
64-86: LGTM!Password update flow properly authorizes the resource and validates the old password before allowing changes.
117-123: LGTM!View setup for the activation page is straightforward.
164-173: LGTM!View setup for password reset with proper error handling.
206-210: LGTM!Simple redirect logic for authenticated users.
214-216: LGTM!Standard strong parameters implementation.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
app/models/user.rb (2)
40-43: Unreachablenew_record?inafter_savecallback.This was flagged in a previous review:
new_record?will always returnfalseinsideafter_savesince the record is already persisted. The condition is unreachable.
133-136: Usefind_byinstead offindto avoid raising on missing record.This was flagged in a previous review:
SofiaAccount.find(auth.uid)raisesActiveRecord::RecordNotFoundif the account doesn't exist. Usefind_by(id: auth.uid)for graceful handling.
🧹 Nitpick comments (8)
spec/controllers/sofia_accounts_controller/disable_otp_spec.rb (1)
5-62: Avoid shadowing Rails’requesthelper; use a dedicated helper andresponseUsing
let(:request)here overrides Rails/RSpec’s built‑inrequesthelper and makes the examples harder to read (it’s actually the response). It’s safer and clearer to give the helper a specific name and assert onresponse:- let(:request) do - patch :disable_otp, params: { id: sofia_account.id } - end + let(:perform_request) do + patch :disable_otp, params: { id: sofia_account.id } + end @@ before do sign_in sofia_account.user - request + perform_request sofia_account.reload end @@ it 'updates sofia_account' do - expect(request.status).to eq 302 + expect(response.status).to eq 302 expect(sofia_account.otp_enabled).to be false end @@ before do sign_in create(:user) - request + perform_request sofia_account.reload end @@ it 'does not update sofia_account' do - expect(request.status).to eq 403 + expect(response.status).to eq 403 expect(sofia_account.otp_enabled).to be true end @@ before do sign_in create(:user, :main_bartender) - request + perform_request sofia_account.reload end @@ it 'does not update sofia_account' do - expect(request.status).to eq 403 + expect(response.status).to eq 403 expect(sofia_account.otp_enabled).to be true end @@ before do sign_in create(:user, :treasurer) - request + perform_request sofia_account.reload end @@ it 'does not update sofia_account' do - expect(request.status).to eq 403 + expect(response.status).to eq 403 expect(sofia_account.otp_enabled).to be true endspec/controllers/sofia_accounts_controller/enable_otp_spec.rb (1)
5-99: Renamerequesthelper to avoid shadowing and fix minor spec typoSame as in the disable_otp specs,
let(:request)shadows Rails’requesthelper and actually returns the response. Renaming makes intent clearer and avoids surprises. There’s also a small typo in thedescribetext.- let(:request) do - patch :enable_otp, params: request_params - end + let(:perform_request) do + patch :enable_otp, params: request_params + end @@ before do sign_in user - request + perform_request sofia_account.reload end @@ it 'updates sofia_account' do - expect(request.status).to eq 302 + expect(response.status).to eq 302 expect(sofia_account.otp_enabled).to be true end @@ before do sign_in create(:user) - request + perform_request sofia_account.reload end @@ it 'does not update sofia_account' do - expect(request.status).to eq 403 + expect(response.status).to eq 403 expect(sofia_account.otp_enabled).to be false end @@ before do sign_in create(:user, :main_bartender) - request + perform_request sofia_account.reload end @@ it 'does not update sofia_account' do - expect(request.status).to eq 403 + expect(response.status).to eq 403 expect(sofia_account.otp_enabled).to be false end @@ before do sign_in create(:user, :treasurer) - request + perform_request sofia_account.reload end @@ it 'does not update sofia_account' do - expect(request.status).to eq 403 + expect(response.status).to eq 403 expect(sofia_account.otp_enabled).to be false end @@ before do request_params[:verification_code] = nil sign_in user - request + perform_request sofia_account.reload end @@ it 'does not update sofia_account' do - expect(request.status).to eq 302 + expect(response.status).to eq 302 expect(sofia_account.otp_enabled).to be false expect(flash[:error]).to match(/de verificatie token is niet aanwezig/) end @@ - describe 'with wrong verication_code' do + describe 'with wrong verification_code' do before do request_params[:verification_code] = SecureRandom.urlsafe_base64 sign_in user - request + perform_request sofia_account.reload end @@ it 'does not update sofia_account' do - expect(request.status).to eq 302 + expect(response.status).to eq 302 expect(sofia_account.otp_enabled).to be false expect(flash[:error]).to match(/de verificatie token is ongeldig/) endapp/models/user.rb (3)
108-115:sofia_accountin attribute allowlist is an association, not a column.The
archive!method checks%w[... sofia_account ...]againstattributes.each_key, butsofia_accountis ahas_oneassociation, not a database column. This check is effectively a no-op sincesofia_accountwon't appear inattributes.keys. If the intent is to preserve the association during archival, thedependent: :destroyon line 28 already handles cleanup when the user is destroyed, so this inclusion is unnecessary.def archive! attributes.each_key do |attribute| - self[attribute] = nil unless %w[deleted_at updated_at created_at provider sofia_account id uid].include? attribute + self[attribute] = nil unless %w[deleted_at updated_at created_at provider id uid].include? attribute end
20-23: Consider simplifying theactivescope usingORwith Arel or separate conditions.The raw SQL string with multiple placeholders is harder to read and maintain. Consider using ActiveRecord's
ormethod for clarity:-scope :active, lambda { - where(deactivated: false).where('(provider IS NULL OR provider != ?) OR - (provider = ? AND id IN (?))', 'sofia_account', 'sofia_account', SofiaAccount.select('user_id')) -} +scope :active, lambda { + where(deactivated: false).where.not(provider: 'sofia_account') + .or(where(deactivated: false, provider: 'sofia_account', id: SofiaAccount.select(:user_id))) +}
45-47: Mailer delivery inafter_createcallback may cause issues in tests and transactions.Enqueuing emails directly in model callbacks couples the model to the mailer. If the transaction rolls back after
after_createfires (e.g., due to a later validation in a nested transaction), the email job may still be enqueued. Consider moving this to the controller or usingafter_commitfor safer transactional behavior.-after_create do - UserMailer.account_creation_email(self).deliver_later if User.sofia_account.exists?(id:) -end +after_commit :send_account_creation_email, on: :create + +private + +def send_account_creation_email + UserMailer.account_creation_email(self).deliver_later if User.sofia_account.exists?(id:) +enddb/schema.rb (1)
159-170: Missing foreign key constraint forsofia_accounts.user_id.The
sofia_accountstable has auser_idcolumn with an index, but there's no foreign key constraint tousers. Other tables in this schema (e.g.,credit_mutations,orders) have explicit foreign keys. Consider adding:Add to the foreign keys section at the end of the schema:
add_foreign_key "sofia_accounts", "users"This ensures referential integrity at the database level and prevents orphaned
sofia_accountrecords.spec/controllers/sofia_accounts_controller/reset_password_spec.rb (1)
54-59: Attribute comparison withdup.attributesmay include timestamp differences.The comparison
sofia_account.dup.attributes == old_sofia_account.attributesmay fail intermittently ifupdated_atis touched during the request even when the save fails. Consider comparing only relevant attributes or usingexceptto exclude timestamps.it 'does not update sofia_account' do - expect(sofia_account.dup.attributes).to eq old_sofia_account.attributes + expect(sofia_account.password_digest).to eq old_sofia_account.password_digestspec/controllers/sofia_accounts_controller/update_password_spec.rb (1)
24-27: Consider extracting repeatedlet(:old_sofia_account)into a shared example or before block.The pattern
let(:old_sofia_account) { sofia_account.dup }followed byold_sofia_accountinbeforeis repeated in every context. This could be DRYed up using a shared context or moving to a top-levellet.Also applies to: 41-44, 57-60, 73-76, 89-92, 106-109, 123-126, 140-143, 160-161, 177-179
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
app/models/user.rb(5 hunks)db/schema.rb(3 hunks)spec/controllers/sofia_accounts_controller/activate_account_spec.rb(1 hunks)spec/controllers/sofia_accounts_controller/create_spec.rb(1 hunks)spec/controllers/sofia_accounts_controller/disable_otp_spec.rb(1 hunks)spec/controllers/sofia_accounts_controller/enable_otp_spec.rb(1 hunks)spec/controllers/sofia_accounts_controller/forgot_pasword_spec.rb(1 hunks)spec/controllers/sofia_accounts_controller/new_activation_link_spec.rb(1 hunks)spec/controllers/sofia_accounts_controller/reset_password_spec.rb(1 hunks)spec/controllers/sofia_accounts_controller/update_password_spec.rb(1 hunks)spec/controllers/users_controller/update_with_sofia_account_spec.rb(1 hunks)spec/models/sofia_account_spec.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- spec/controllers/users_controller/update_with_sofia_account_spec.rb
- spec/controllers/sofia_accounts_controller/forgot_pasword_spec.rb
- spec/controllers/sofia_accounts_controller/activate_account_spec.rb
- spec/models/sofia_account_spec.rb
- spec/controllers/sofia_accounts_controller/new_activation_link_spec.rb
🧰 Additional context used
🧬 Code graph analysis (2)
spec/controllers/sofia_accounts_controller/create_spec.rb (1)
app/controllers/sofia_accounts_controller.rb (1)
create(10-28)
spec/controllers/sofia_accounts_controller/update_password_spec.rb (1)
app/controllers/sofia_accounts_controller.rb (1)
create(10-28)
🪛 ast-grep (0.40.0)
app/models/user.rb
[warning] 20-21: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: where(deactivated: false).where('(provider IS NULL OR provider != ?) OR
(provider = ? AND id IN (?))', 'sofia_account', 'sofia_account', SofiaAccount.select('user_id'))
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
🪛 Gitleaks (8.29.1)
app/models/user.rb
[high] 36-36: 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). (1)
- GitHub Check: Build
🔇 Additional comments (7)
db/schema.rb (1)
183-184: LGTM: Activation token fields properly added.The
activation_tokenandactivation_token_valid_tillfields on the users table align with the activation flow requirements. The token expiration field enables time-limited activation links.spec/controllers/sofia_accounts_controller/reset_password_spec.rb (2)
35-40: Potential issue:old_sofia_account.passwordmay benilafterdup.The
passwordattribute is a virtual attribute (fromhas_secure_passwordor OmniAuth Identity) that's not persisted. Afterdup,old_sofia_account.passwordmay benil, makingauthenticate(old_sofia_account.password)returnnilrather thanfalse, which would still pass thebe falsematcher due to falsy comparison but may not test the intended behavior.Consider using the literal string instead:
it 'updates sofia_account' do - expect(sofia_account.authenticate(old_sofia_account.password)).to be false + expect(sofia_account.authenticate('password1234')).to be false expect(sofia_account.authenticate(request_params[:sofia_account][:password])).to be sofia_account
1-24: Good test coverage for password reset flow.The test suite comprehensively covers the reset password scenarios including token validation, password requirements, and confirmation matching. The Dutch flash message assertions align with the localization requirements.
spec/controllers/sofia_accounts_controller/update_password_spec.rb (2)
23-38: Good authorization test - owner can update their password.The test correctly verifies that the account owner can update their password and validates both the old password is invalidated and the new password works.
40-86: Solid authorization boundary tests.Good coverage ensuring other users, main-bartenders, and treasurers cannot modify another user's password, receiving 403 Forbidden responses.
spec/controllers/sofia_accounts_controller/create_spec.rb (2)
1-44: Comprehensive test coverage for account creation flow.The test suite thoroughly covers the create action with various email scenarios, token validation, and error handling. The assertions on flash messages in Dutch align with the localization requirements.
195-210: Good edge case: prevents duplicate sofia_account creation.The test correctly verifies that attempting to create a second sofia_account for an already-activated user fails appropriately.
7a95107 to
5dc3824
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
app/controllers/users_controller.rb (1)
34-36: Remove OTP instance-method mixin from the controller (handled at the model level)
ActiveModel::OneTimePassword::InstanceMethodsOnActivationis meant to be included into the model (SofiaAccount) that holds the OTP secret, not into the controller. Including it here doesn’t provide value and can confuse future readers about where OTP behavior actually lives.You can safely drop this include and keep OTP logic confined to the model.
app/views/users/index.html.erb (1)
42-51: Add authorization guard around “Nieuwe gebruiker” Streepsysteem buttonThe “Streepsysteem gebruikers” “Nieuwe gebruiker” button is still rendered unconditionally, unlike the manual-user button above which is wrapped in
if policy(User).create?. That means any authenticated user who can reach this page will see the button, even if they can’t actually create Sofia-account users.Wrap this button in the same policy check, for example:
<div class="d-flex justify-content-between align-items-center"> <h3>Streepsysteem gebruikers</h3> <% if policy(User).create? %> <button class="btn btn-sm btn-primary" data-bs-target="#new_sofia_account_user_modal" data-bs-toggle="modal" role="button"> <i class="fas fa-plus"></i> <span class="d-none d-md-inline ms-1"> Nieuwe gebruiker </span> </button> <% end %> </div>app/models/user.rb (2)
40-43: Duplicate: Unreachable condition in after_save callback.This issue was previously flagged. The
new_record?check in anafter_savecallback will always returnfalsebecause the record has already been persisted.
133-136: Duplicate: Usefind_byinstead offindto avoid raising on missing record.This issue was previously flagged. Using
findwill raiseActiveRecord::RecordNotFoundif the account doesn't exist, which is inappropriate during authentication where invalid credentials are expected.
🧹 Nitpick comments (7)
app/views/partials/_login_prompt.html.erb (3)
5-5: Consider moving hard‑coded Dutch strings to I18n translationsThe messages like “Log in om door te gaan” and “Log in met een streepsysteem account.” are clear, but they’re hard‑coded Dutch in the view. If the rest of the app is using Rails I18n, consider moving these to locale files so you can localize or tweak copy without touching templates.
Also applies to: 23-25
8-21: Amber login option and separator look fine; optional accessibility/i18n polishThe conditional rendering based on
Rails.application.config.x.amber_api_url.present?is a good way to hide Amber login where not configured, and usingbutton_towithdata: { turbo: "false" }is appropriate for OmniAuth redirects.Two small optional improvements:
- If
config.x.site_associationis also meant to be configurable text, consider moving the full sentence/button label into translations rather than interpolating in the view.- The standalone “OF” separator could be more accessible/semantic if marked as a separator (e.g., visually styled HR with
role="separator"), or at least wrapped in an element witharia-hidden="true"if it’s purely decorative.
22-29: Sofia login option is straightforward; check button styling consistencyThe Sofia login entry correctly provides a clear path via
login_sofia_accounts_pathand uses primary button styling, which matches the Amber option visually.One small UX consistency check: if the
.btn-loginclass on the Amber button carries important shared styling for “main login actions” (spacing, width, etc.), you may want to add it to the Sofia button as well to keep both options visually aligned. If.btn-loginis Amber-specific, current code is fine.app/views/users/show.html.erb (1)
124-137: Avoid=begin/=endblock comments inside ERB for the deposit/iDEAL textUsing Ruby block comments (
=begin/=end) across multiple ERB tags is brittle and makes it harder to see that the “onder vermelding van je naam en 'Inleg Zatladder'” text and the iDEAL link are intentionally disabled.If this content is meant to be removed, delete it; if it’s meant to be kept for possible reuse, prefer ERB or HTML comments, e.g.:
<%# onder vermelding van je naam en 'Inleg Zatladder'. %>or wrap the whole block in
<!-- ... -->so the template stays simple.app/models/user.rb (2)
20-23: Consider optimizing the subquery for better performance.The
IN (?)clause with a subquery will execute the subquery on every call to.active. For large datasets, this may cause performance issues.Consider refactoring with a LEFT JOIN for better query performance:
-scope :active, (lambda { - where(deactivated: false).where('(provider IS NULL OR provider != ?) OR - (provider = ? AND id IN (?))', 'sofia_account', 'sofia_account', SofiaAccount.select('user_id')) -}) +scope :active, (lambda { + left_joins(:sofia_account) + .where(deactivated: false) + .where('users.provider IS NULL OR users.provider != ? OR (users.provider = ? AND sofia_accounts.user_id IS NOT NULL)', + 'sofia_account', 'sofia_account') +})This eliminates the subquery and uses a single JOIN instead.
45-47: Simplify the Sofia account check to avoid extra query.The condition
User.sofia_account.exists?(id:)performs an unnecessary database query. Since the user instance is already available, you can check the provider attribute directly.Apply this diff:
after_create do - UserMailer.account_creation_email(self).deliver_later if User.sofia_account.exists?(id:) + UserMailer.account_creation_email(self).deliver_later if provider == 'sofia_account' enddb/schema.rb (1)
159-170: Consider making otp_secret_key nullable to avoid storing unused secrets.The schema requires
otp_secret_keyto beNOT NULLeven whenotp_enabledisfalse(the default). This forces generation and storage of OTP secrets for all users, even those who never enable 2FA.Consider allowing
otp_secret_keyto be nullable and generating it only when the user enables OTP:t.string "otp_secret_key" # Remove null: falseThis reduces unnecessary secret generation and storage, improving security posture by minimizing the attack surface.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
Gemfile(3 hunks)app/assets/stylesheets/application.scss(1 hunks)app/controllers/users_controller.rb(3 hunks)app/javascript/users.js(2 hunks)app/models/role.rb(1 hunks)app/models/user.rb(5 hunks)app/views/partials/_login_prompt.html.erb(1 hunks)app/views/users/index.html.erb(2 hunks)app/views/users/show.html.erb(3 hunks)config/initializers/devise.rb(2 hunks)config/routes.rb(2 hunks)db/schema.rb(7 hunks)spec/controllers/users_controller/index_spec.rb(1 hunks)spec/controllers/users_controller/show_spec.rb(5 hunks)spec/controllers/users_controller/update_spec.rb(1 hunks)spec/models/role_spec.rb(0 hunks)spec/models/user_spec.rb(7 hunks)spec/rails_helper.rb(1 hunks)
💤 Files with no reviewable changes (1)
- spec/models/role_spec.rb
🚧 Files skipped from review as they are similar to previous changes (6)
- spec/models/user_spec.rb
- config/initializers/devise.rb
- app/models/role.rb
- spec/rails_helper.rb
- Gemfile
- app/assets/stylesheets/application.scss
🧰 Additional context used
🧬 Code graph analysis (4)
app/javascript/users.js (7)
app/javascript/payment_add.js (1)
element(9-9)app/javascript/activity.js (1)
element(18-18)app/javascript/activities.js (1)
element(8-8)app/javascript/credit_mutations.js (1)
element(11-11)app/javascript/user.js (1)
element(10-10)app/javascript/price_lists.js (1)
element(8-8)app/javascript/order_screen.js (1)
element(16-16)
spec/controllers/users_controller/update_spec.rb (1)
app/models/role.rb (1)
name(8-16)
app/controllers/users_controller.rb (2)
app/policies/user_policy.rb (1)
show?(14-16)app/models/user.rb (2)
calculate_credits(148-153)treasurer?(86-88)
app/models/user.rb (1)
app/mailers/user_mailer.rb (2)
account_creation_email(1-28)account_creation_email(2-9)
🪛 ast-grep (0.40.0)
app/models/user.rb
[warning] 20-21: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: where(deactivated: false).where('(provider IS NULL OR provider != ?) OR
(provider = ? AND id IN (?))', 'sofia_account', 'sofia_account', SofiaAccount.select('user_id'))
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
🪛 Gitleaks (8.29.1)
app/models/user.rb
[high] 36-36: 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). (1)
- GitHub Check: Build
🔇 Additional comments (16)
app/javascript/users.js (1)
22-35: Vue dataset wiring for new user cohorts looks consistentThe new JSON.parse calls and Vue
databindings forsofia_account_users,not_activated_users, anddeactivated_usersline up with the data attributes from the view and reuse existing error handling; no issues from my side.spec/controllers/users_controller/show_spec.rb (1)
6-12: SofiaAccount visibility specs align with policy expectationsThe added
sofiafixture and role-based expectations (treasurer ok, others forbidden) correctly mirror theUserPolicy#show?behavior for Sofia-account users and round out coverage for the new user type.Also applies to: 30-35, 53-58, 76-81, 99-104
spec/controllers/users_controller/update_spec.rb (1)
12-29: Clearer authorization matrix forPUT updateSwitching to
action_userand splitting contexts (self vs other vs roles) makes the intent explicit and verifies that only treasurers can successfully update via this path. Behavior and expectations look consistent.Also applies to: 31-48
spec/controllers/users_controller/index_spec.rb (1)
10-20: Index cohort expectations correctly track Sofia and activation statesThe new setup and role-based expectations for
manual_users,amber_users,sofia_account_users,not_activated_users, anddeactivated_usersmatch the seeding logic and policy behavior, giving good coverage for the expanded index categorization.Also applies to: 22-65
app/controllers/users_controller.rb (3)
6-13: Index grouping and JSON credit annotations look coherentThe new
@sofia_account_users,@not_activated_users, and@deactivated_usersscopes, plus their *_json variants with attached credits, mirror the existing manual/amber pattern and line up with what the frontend expects. No functional issues spotted here.Also applies to: 16-29
43-62: SofiaAccount loading and QR generation inshowFinding the SofiaAccount by
user_idand generating an SVG QR withprovisioning_urifor the authenticator app is a reasonable approach, and falling back toSofiaAccount.newensures the modal has an object to bind to.Just ensure that the SofiaAccount model is the one including the OTP mixin so that
provisioning_uriremains available there, not via the controller include above.
184-186: Allowingprovideron create looks intentional but stays restricted by policyAdding
providertopermitted_attributesforcreatemakes sense for distinguishing manual vs Sofia-account-backed users, and thecreateaction still runs throughauthorize @user, so only roles allowed byUserPolicy#create?can set it. I don’t see an immediate risk here.app/views/users/index.html.erb (1)
5-8: Users index wiring for new cohorts matches the JS expectationsThe
users-indexcontainer now exposesmanual_users,sofia_account_users,amber_users,not_activated_users, anddeactivated_usersvia data attributes, and the extra tables/sections (Sofia accounts, not-activated users, deactivated users) line up with the backend assignments and the Vue code. Layout and conditional rendering (@not_activated_users.any?,@deactivated_users.any?) look consistent.Also applies to: 13-15, 25-65
config/routes.rb (1)
22-32: Routing for SofiaAccount flows and user settings looks well-structured
update_with_sofia_accountas a memberPATCHon users, combined with thesofia_accountscollection/member routes for login, activation, password reset, and OTP toggling, forms a coherent, REST-ish surface with appropriate HTTP verbs. Nothing problematic stands out here.Also applies to: 53-68
app/models/user.rb (5)
14-15: Email validation logic is appropriate for Sofia accounts.The validation correctly requires email presence for active Sofia account users while allowing blank emails for other user types (manual, Amber). The format validation with
allow_blank: trueensures invalid formats are caught when an email is provided.
28-29: Association and nested attributes configured correctly.The
dependent: :destroyensures Sofia accounts are cleaned up when users are deleted, andaccepts_nested_attributes_forenables the nested form creation flow described in the PR objectives.
33-38: Activation token generation is secure and correctly scoped.The callback appropriately generates a secure token for new Sofia account users with a 5-day validity period as specified in the PR objectives.
99-100: Guard correctly restricts role updates to Amber users.The early return ensures
update_roleonly processes Amber OAuth users, preventing inappropriate role synchronization for Sofia and manual accounts.
110-110: I'll help you verify whether the 'sofia_account' whitelist entry has the intended effect. Let me investigate the codebase to understand the User model structure and how this association is configured.
<function_calls>
#!/bin/bashFind migration files related to users and sofia_account
echo "=== Looking for migration files ==="
fd -e rb "migrate" db/migrate 2>/dev/null | head -20
echo ""
echo "=== Searching for sofia_account in migrations ==="
rg -n "sofia_account" db/migrate/ 2>/dev/null || echo "No results in migrations"
echo ""
echo "=== Checking db/schema.rb for users table ==="
rg -A 50 "create_table "users"" db/schema.rb 2>/dev/null | head -60
</function_calls>db/schema.rb (2)
183-184: Activation token columns correctly added to support Sofia account flows.The nullable columns appropriately support the activation and password reset flows described in the PR objectives without impacting existing Amber and manual users.
141-141: The nullablegroup_uidchange is intentional and safe—no null-safety issues exist.The migration
20240413094147_create_sofia_accounts.rbexplicitly madegroup_uidnullable by callingchange_column_null :roles, :group_uid, true, 999_999, which also removed the unique composite index on[role_type group_uid]. This design supports Sofia-specific roles (withgroup_uid = 999_999orNULL) alongside Amber group-linked roles.The
Role.where(group_uid: groups)query inUser#update_role(line 101) works correctly because SQL'sWHEREclause naturally excludesNULLvalues—roles without an Amber group won't be matched, which is the intended behavior. No null checks are needed since the query logic is sound.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
db/migrate/20251113094147_create_sofia_accounts.rb(1 hunks)db/schema.rb(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- db/migrate/20251113094147_create_sofia_accounts.rb
⏰ 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). (1)
- GitHub Check: Build
🔇 Additional comments (2)
db/schema.rb (2)
13-13: LGTM: Schema version updated.The schema version bump reflects the new migrations for Sofia local accounts.
183-184: No index needed onactivation_token.The
activation_tokencolumn is not used for database lookups. In the activation and password reset flows, users are retrieved byid(from URL parameters), and the token is validated in-memory by comparing the user's stored token against the one from the request. Since no query ever searches byactivation_token, an index would not improve performance and is unnecessary for this implementation.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
still need to review manually and introduce coderabbit nitpick comments (28) |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
db/migrate/20251113094147_create_sofia_accounts.rb (1)
6-6: Missing unique index on user_id for 1:1 relationship.The model has
validates :user, uniqueness: true, but there's no database-level unique index onuser_id. Application-level validations can have race conditions. Add a unique index for data integrity:t.index :username, unique: true + t.index :user_id, unique: true end
🧹 Nitpick comments (9)
app/views/partials/_flash.html.erb (1)
6-15: Good fix for the XSS vulnerability.This properly addresses the previous concern by escaping regular flash values and sanitizing
_html-suffixed keys before rendering. The opt-in pattern is a safe approach.Minor note:
sanitize()already returns anActiveSupport::SafeBuffer, so the.html_safecall on line 12 is redundant.- <% if html_key %> - <%= trusted_html.html_safe %> + <% if html_key %> + <%= trusted_html %>spec/controllers/users_controller/update_with_sofia_account_spec.rb (1)
8-21: Test setup may not be testing the intended behavior.The
requestlet (lines 8-11) capturessofia_account.attributeswithusername: 'Old username'. Then in thebeforeblock (line 17), you modifyuser.sofia_account.username = 'New username', but this doesn't affect the already-definedrequestparams.The test expects
'New username'(line 27), but the request sends'Old username'in the params. Either the test is passing for the wrong reason, or there's a different mechanism at play.Consider restructuring to explicitly send the new username:
let(:request) do patch :update_with_sofia_account, - params: { id: user.id, user: user.attributes.merge({ sofia_account_attributes: sofia_account.attributes }) } + params: { id: user.id, user: { sofia_account_attributes: { username: 'New username' } } } end before do user sofia_account sign_in action_user - user.sofia_account.username = 'New username' - user.name = 'New name' request user.reload endapp/views/users/index.html.erb (1)
60-67: Minor ERB formatting inconsistency.Lines 60 and 67 use
<%end %>instead of the conventional<% end %>with a space.- <%end %> + <% end %>app/controllers/callbacks_controller.rb (1)
34-39: HTML in JSON error message - verify frontend escaping.The error message contains raw HTML (
<br/>,<i>tags). Ensure the frontend properly escapes or safely renders this content to prevent XSS if the message is ever user-influenced in the future.Also, the multi-line string may introduce unwanted whitespace. Consider using a single line or heredoc:
- render(json: { state: 'password_prompt', error_message: 'Inloggen mislukt door een error. Herlaad de pagina en probeer het nog - een keer. <br/><i>Werkt het na een paar keer proberen nog steeds niet? - Neem dan contact op met de ICT-commissie.</i>' }) + render(json: { state: 'password_prompt', error_message: 'Inloggen mislukt door een error. Herlaad de pagina en probeer het nog een keer. <br/><i>Werkt het na een paar keer proberen nog steeds niet? Neem dan contact op met de ICT-commissie.</i>' })app/views/users/show.html.erb (1)
126-136: Commented-out code should be removed or documented.The
=begin/=endblock disables payment instruction text and iDEAL functionality. If this is intentional, consider removing the code entirely or adding a comment explaining why it's disabled. Commented-out code in views creates maintenance burden.- <% -=begin%> - <%= 'onder vermelding van je naam en \'Inleg Zatladder\'.' %> - - <% if Rails.application.config.x.mollie_api_key.present? %> - <%= link_to add_payments_path do %> - <%= 'Klik hier om je saldo over te maken via iDEAL ' %> - <% end %> - <% end %> -<% -=end%>Do you want me to open an issue to track this cleanup or investigate why this section was disabled?
app/views/users/_edit_sofia_account_modal.html.erb (1)
90-114: Addrel="noopener noreferrer"to external links for security.Links with
target='_blank'should includerel="noopener noreferrer"to prevent the opened page from accessingwindow.opener, which could be exploited for phishing or tabnabbing attacks.<a href='https://play.google.com/store/apps/details?id=com.google.android.apps.authenticator2' target='_blank' + rel='noopener noreferrer' > Google Authenticator </a>Apply the same pattern to the Apple and Microsoft authenticator links on lines 100 and 109.
app/controllers/users_controller.rb (1)
58-60: Consider setting user association on new SofiaAccount.When no SofiaAccount exists, a new empty one is created without the user association. If the policy or view attempts to access
@sofia_account.user, it may return nil. Consider:else - @sofia_account = SofiaAccount.new + @sofia_account = @user.build_sofia_account endThis ensures the association is set even for unsaved records, which may help policy checks and view logic that depend on the relationship.
spec/controllers/callbacks_controller/sofia_account_spec.rb (1)
76-93: Consider clarifying OTP setup order in tests.The
otp_enabledflag is set in thebeforeblock aftersofia_accountis created withpasswordbut theverification_codeinrequest_paramsreferencessofia_account.otp_codefrom the baseletblock. This works becauseotp_codeis generated fromotp_secret, but the test setup order could be confusing to future readers.For clarity, consider moving
otp_enabled: trueinto the factory or using a separateletfor OTP-enabled accounts:context 'with sofia_account with otp' do let(:sofia_account) do create(:sofia_account, password: 'password1234', password_confirmation: 'password1234', otp_enabled: true) end # ... endapp/models/user.rb (1)
99-100: Consider optimizing the guard with an attribute check.The
User.in_amber.exists?(id)guard triggers a separate database query. Since you already have access to theproviderattribute on the current instance, you can optimize this by checking the attribute directly.Apply this diff:
def update_role(groups) - return unless User.in_amber.exists?(id) + return unless provider == 'amber_oauth2'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
app/controllers/callbacks_controller.rb(1 hunks)app/controllers/sofia_accounts_controller.rb(1 hunks)app/controllers/users_controller.rb(3 hunks)app/mailers/user_mailer.rb(1 hunks)app/models/user.rb(5 hunks)app/policies/user_policy.rb(1 hunks)app/views/partials/_flash.html.erb(1 hunks)app/views/sofia_accounts/login.html.erb(1 hunks)app/views/user_mailer/new_activation_link_email.html.erb(1 hunks)app/views/users/_edit_sofia_account_modal.html.erb(1 hunks)app/views/users/index.html.erb(2 hunks)app/views/users/show.html.erb(3 hunks)config/initializers/devise.rb(2 hunks)db/migrate/20251113094147_create_sofia_accounts.rb(1 hunks)spec/controllers/callbacks_controller/sofia_account_spec.rb(1 hunks)spec/controllers/sofia_accounts_controller/create_spec.rb(1 hunks)spec/controllers/sofia_accounts_controller/disable_otp_spec.rb(1 hunks)spec/controllers/sofia_accounts_controller/enable_otp_spec.rb(1 hunks)spec/controllers/sofia_accounts_controller/forgot_password_spec.rb(1 hunks)spec/controllers/sofia_accounts_controller/new_activation_link_spec.rb(1 hunks)spec/controllers/sofia_accounts_controller/update_password_spec.rb(1 hunks)spec/controllers/users_controller/update_with_sofia_account_spec.rb(1 hunks)spec/mailers/previews/user_mailer_preview.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- spec/controllers/sofia_accounts_controller/enable_otp_spec.rb
- spec/controllers/sofia_accounts_controller/disable_otp_spec.rb
- spec/controllers/sofia_accounts_controller/new_activation_link_spec.rb
- app/policies/user_policy.rb
- app/controllers/sofia_accounts_controller.rb
- config/initializers/devise.rb
- app/mailers/user_mailer.rb
- app/views/sofia_accounts/login.html.erb
- app/views/user_mailer/new_activation_link_email.html.erb
🧰 Additional context used
🧬 Code graph analysis (7)
app/controllers/callbacks_controller.rb (1)
app/models/user.rb (1)
from_omniauth_inspect(133-138)
app/models/user.rb (1)
app/mailers/user_mailer.rb (2)
account_creation_email(1-29)account_creation_email(2-4)
app/controllers/users_controller.rb (2)
app/policies/user_policy.rb (1)
show?(14-16)app/policies/application_policy.rb (1)
show?(13-15)
spec/controllers/users_controller/update_with_sofia_account_spec.rb (1)
app/controllers/sofia_accounts_controller.rb (1)
create(12-30)
spec/controllers/sofia_accounts_controller/create_spec.rb (1)
app/controllers/sofia_accounts_controller.rb (1)
create(12-30)
spec/controllers/callbacks_controller/sofia_account_spec.rb (2)
app/controllers/sofia_accounts_controller.rb (1)
create(12-30)spec/support/devise_helper.rb (2)
signed_in_user(6-12)signed_in?(2-4)
spec/mailers/previews/user_mailer_preview.rb (2)
app/javascript/user.js (1)
user(12-12)app/mailers/user_mailer.rb (4)
account_creation_email(1-29)account_creation_email(2-4)forgot_password_email(10-17)new_activation_link_email(6-8)
🪛 ast-grep (0.40.0)
app/models/user.rb
[warning] 20-21: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: where(deactivated: false).where('(provider IS NULL OR provider != ?) OR
(provider = ? AND id IN (?))', 'sofia_account', 'sofia_account', SofiaAccount.select('user_id'))
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
🪛 Gitleaks (8.29.1)
app/models/user.rb
[high] 36-36: 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). (1)
- GitHub Check: Build
🔇 Additional comments (20)
app/views/partials/_flash.html.erb (1)
2-2: LGTM on the styling adjustment.The added
mt-3provides consistent spacing with the updated layouts.spec/controllers/users_controller/update_with_sofia_account_spec.rb (3)
31-54: Authorization tests look comprehensive.Good coverage of unauthorized access scenarios for regular users, main bartenders, renting managers, and treasurers. This aligns with the policy that only the account owner can update their Sofia account settings.
56-75: Good edge case coverage for users without Sofia accounts.This test correctly verifies that attempting to update a non-existent Sofia account results in a redirect with an appropriate alert and doesn't inadvertently create one.
6-7: No issues identified. The:sofia_accounttrait only sets the provider attribute without creating a SofiaAccount object. Line 7 correctly creates a single SofiaAccount for the user, so the uniqueness validation constraint is satisfied.app/views/users/index.html.erb (2)
42-53: Authorization check is now in place.The
policy(User).create?check (line 44) properly guards the "Nieuwe gebruiker" button for Streepsysteem users, addressing the previous review concern.
13-13: Data payload structure looks good.The extended data attributes properly segment users by type (manual, sofia_account, amber, not_activated, deactivated) for the Vue components.
app/controllers/callbacks_controller.rb (1)
42-66: OTP and failure handling look correct.The OTP flow properly handles the three states: prompting when code is missing, validating when present, and returning appropriate errors. The failure handler correctly differentiates between credential errors and unexpected failures.
app/views/users/show.html.erb (1)
72-88: Good fix: Settings button is now additive, not mutually exclusive.The Sofia account "Instellingen" button (lines 72-77) and treasurer controls (lines 79-88) now use separate
ifblocks, allowing both to appear when applicable. This addresses the previous review concern.spec/controllers/sofia_accounts_controller/forgot_password_spec.rb (2)
5-6: Same potential duplicate SofiaAccount issue as in other specs.Similar to
update_with_sofia_account_spec.rb, this creates a user with:sofia_accounttrait and then a separatesofia_account. If the trait creates an account, this will violate the uniqueness constraint.- let(:user) { create(:user, :sofia_account) } + let(:user) { create(:user) } let(:sofia_account) { create(:sofia_account, user:) }
17-86: Good security-conscious test coverage.The tests correctly verify that:
- Non-existent usernames receive the same generic success message as existing ones (line 83), preventing username enumeration attacks
- Missing username gets an error (appropriate since it's a required field)
- Users without email get a generic message without sending (graceful handling)
spec/controllers/sofia_accounts_controller/update_password_spec.rb (2)
5-18: Password handling correctly uses literal values.Good fix from the previous review: the test now uses the literal
'password1234'string (lines 6 and 13) instead of relying on the virtualpasswordattribute which doesn't persist.
23-189: Comprehensive test coverage for password update scenarios.The tests cover:
- Successful owner update with proper authentication
- Authorization (403) for non-owners including other users, bartenders, and treasurers
- Validation errors: missing/wrong old password, missing/invalid/mismatched new password
This provides good confidence in the
update_passwordaction's behavior.app/views/users/_edit_sofia_account_modal.html.erb (1)
74-74: Previous issue resolved:disable_otp_sofia_account_pathnow includes the required id parameter.The routing error issue flagged in a previous review has been addressed. The path helper now correctly receives
@sofia_account.id.spec/mailers/previews/user_mailer_preview.rb (1)
1-18: LGTM!The mailer preview correctly builds mock User and SofiaAccount objects for previewing activation and password-reset emails without requiring database records. The approach using
User.newwithbuild_sofia_accountis appropriate for preview purposes.app/controllers/users_controller.rb (1)
144-164: Previous issue resolved: Nil guard for SofiaAccount is now in place.The action now properly checks for a nil SofiaAccount and redirects with an appropriate flash message before attempting authorization. This addresses the previously flagged vulnerability where calling
authorize @sofia_accountwith a nil record would cause errors.spec/controllers/callbacks_controller/sofia_account_spec.rb (1)
6-21: Good test coverage for identity authentication flow.The test suite comprehensively covers the key login scenarios: non-existent accounts, valid/invalid passwords, and OTP-enabled accounts with correct/incorrect/missing codes. The use of helper methods like
signed_in?andsigned_in_usermakes the assertions readable.spec/controllers/sofia_accounts_controller/create_spec.rb (2)
302-318: Previous issue resolved: Non-existent user ID now uses reliable approach.The test now correctly uses
User.maximum(:id).to_i + 1to generate a guaranteed non-existent user ID, addressing the fragility concern from the previous review.
1-22: Well-structured test suite with comprehensive coverage.The test file provides thorough coverage of the SofiaAccount creation flow, including:
- Valid creation paths (with/without email)
- Token validation (missing, expired, wrong tokens)
- User state validation (deactivated, already activated, non-existent)
- Input validation (missing username, password, password confirmation)
The setup with
letblocks is clear and the test isolation is good.app/models/user.rb (2)
40-43: Previous issue resolved: callback now correctly detects deactivation transitions.The use of
saved_change_to_deactivated?(from: false, to: true)correctly identifies when a user transitions to deactivated status in theafter_savecallback. This resolves the earlier concern about the unreachablenew_record?condition.
133-138: Previous issue resolved: safe lookup with graceful nil handling.The use of
SofiaAccount.find_by(id: auth.uid)with explicit nil checking now provides graceful error handling when the account doesn't exist, rather than raisingActiveRecord::RecordNotFound. This resolves the earlier concern and aligns with the caller's expectations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/models/user.rb (1)
20-23: Consider refactoring the active scope for clarity.The SQL condition is complex and difficult to read. Consider breaking it into smaller scopes or using Ruby logic to combine simpler scopes for better maintainability.
For example:
scope :active, lambda { where(deactivated: false).where.not( provider: 'sofia_account', id: where(provider: 'sofia_account').where.not(id: SofiaAccount.select('user_id')) ) }Or define helper scopes:
scope :activated_sofia_accounts, -> { sofia_account.where(id: SofiaAccount.select('user_id')) } scope :active, -> { where(deactivated: false).where.not(id: not_activated.select(:id)) }app/controllers/callbacks_controller.rb (2)
34-39: Avoid embedding HTML in controller error messages.The multi-line HTML error message is difficult to maintain and poses XSS risks if variables are ever interpolated. Consider moving this to I18n locales or a view helper.
Apply this diff:
else # sofia_account does not exist, should not be possible - render(json: { state: 'password_prompt', error_message: 'Inloggen mislukt door een error. Herlaad de pagina en probeer het nog - een keer. <br/><i>Werkt het na een paar keer proberen nog steeds niet? - Neem dan contact op met de ICT-commissie.</i>' }) + render(json: { state: 'password_prompt', error_message: I18n.t('errors.sofia_account.missing_account_html') }) endThen add to
config/locales/nl.yml:nl: errors: sofia_account: missing_account_html: "Inloggen mislukt door een error. Herlaad de pagina en probeer het nog een keer. <br/><i>Werkt het na een paar keer proberen nog steeds niet? Neem dan contact op met de ICT-commissie.</i>"
56-66: Consider using string interpolation instead of mutation.The string mutation with
<<on line 59 works but is less clear than building the string immutably.Apply this diff:
def failure - error_message = 'Inloggen mislukt.' - if request.env['omniauth.error.strategy'].instance_of? OmniAuth::Strategies::Identity - error_message << if request.env['omniauth.error.type'].to_s == 'invalid_credentials' - ' De ingevulde gegevens zijn incorrect.' - else - ' Er is een onverwachte fout opgetreden.' - end - end + error_message = if request.env['omniauth.error.strategy'].instance_of?(OmniAuth::Strategies::Identity) + if request.env['omniauth.error.type'].to_s == 'invalid_credentials' + 'Inloggen mislukt. De ingevulde gegevens zijn incorrect.' + else + 'Inloggen mislukt. Er is een onverwachte fout opgetreden.' + end + else + 'Inloggen mislukt.' + end render(json: { state: 'password_prompt', error_message: }) end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/controllers/callbacks_controller.rb(1 hunks)app/controllers/sofia_accounts_controller.rb(1 hunks)app/javascript/users.js(2 hunks)app/mailers/user_mailer.rb(1 hunks)app/models/user.rb(4 hunks)app/views/partials/_flash.html.erb(1 hunks)app/views/users/index.html.erb(2 hunks)app/views/users/show.html.erb(3 hunks)spec/controllers/sofia_accounts_controller/disable_otp_spec.rb(1 hunks)spec/controllers/users_controller/update_with_sofia_account_spec.rb(1 hunks)spec/models/user_spec.rb(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- app/views/partials/_flash.html.erb
- app/javascript/users.js
- spec/controllers/users_controller/update_with_sofia_account_spec.rb
- app/mailers/user_mailer.rb
🧰 Additional context used
🧬 Code graph analysis (3)
app/controllers/callbacks_controller.rb (1)
app/models/user.rb (1)
from_omniauth_inspect(133-138)
app/models/user.rb (1)
app/mailers/user_mailer.rb (2)
account_creation_email(1-30)account_creation_email(2-4)
spec/models/user_spec.rb (2)
app/controllers/sofia_accounts_controller.rb (1)
create(12-30)app/models/user.rb (1)
archive!(108-115)
🪛 ast-grep (0.40.0)
app/models/user.rb
[warning] 20-21: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: where(deactivated: false).where('(provider IS NULL OR provider != ?) OR
(provider = ? AND id IN (?))', 'sofia_account', 'sofia_account', SofiaAccount.select('user_id'))
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
🪛 Gitleaks (8.29.1)
app/models/user.rb
[high] 36-36: 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). (1)
- GitHub Check: Build
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
spec/controllers/users_controller/update_with_sofia_account_spec.rb (2)
23-53: Clarify request naming and consider asserting redirect target in the 302 case.The role-based authorization expectations here look good, but two small readability points:
- Using
let(:request)shadows Rails’requesthelper and makes the examples read a bit oddly (request.status). Renaming to something likelet(:perform_request)and asserting onresponse.statusafter calling it in abeforewould be clearer.- For the “as user themselves” case, you currently only assert
302. Adding aredirect_toexpectation would document where a successful update is supposed to send the user.These are purely cosmetic and can be deferred, but they’ll make the spec easier to read and maintain.
56-75: Good coverage of “no Sofia account yet” behavior; could tighten the “no create” assertion.This context nicely checks both the redirect + alert and that
user.sofia_accountstaysnil. If you want a slightly stronger guarantee that nothing is created at all, you could also wrap the PATCH in an expectation like:expect do patch :update_with_sofia_account, params: { id: user.id, user: { sofia_account_attributes: { username: 'New username' } } } end.not_to change(SofiaAccount, :count)Not required, but it hardens the spec against accidental creation of detached SofiaAccount records.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/mailers/user_mailer.rb(1 hunks)spec/controllers/users_controller/update_with_sofia_account_spec.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/mailers/user_mailer.rb
🧰 Additional context used
🧬 Code graph analysis (1)
spec/controllers/users_controller/update_with_sofia_account_spec.rb (2)
app/controllers/sofia_accounts_controller.rb (1)
create(12-30)app/controllers/users_controller.rb (1)
create(74-85)
⏰ 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). (1)
- GitHub Check: Build
| Role.create(role_type: :treasurer, group_uid: 4) | ||
| Role.create(role_type: :renting_manager, group_uid: 5) | ||
| Role.create(role_type: :main_bartender, group_uid: 6) | ||
| Role.create(role_type: :treasurer) |
There was a problem hiding this comment.
Would I be a good idea to change the seeding so we assign the roles to certain users. This would also make it easier to switch between user during testing
2 participants
Fixes #923.
Streepsysteem account aanmaken
Bij gebruikers kan iemand met de rol treasurer streepsysteem gebruikers inzien en aanmaken:
Nieuwe gebruiker aanmaken scherm (precies hetzelfde scherm als voor aanmaken van handmatige gebruikers):
Wanneer de treasurer een streepsysteem gebruiker aanmaakt, wordt het ingevulde emailadres gemaild met de volgende mail:
Als de persoon dan op de activatielink klikt (
/sofia_accounts/activate_account?activation_token=activation_token&user_id=user_id), ziet hij dit (zonder de icoontjes van mijn KeypassXC):Als hij dat invult en drukt op activeren dan wordt een SofiaAccount object aangemaakt, waar de username, password_digest en aanmaak-datum worden opgeslagen. Hij wordt dan ook meteen aan de user gelinkt waar die bij hoort. Als de user wordt verwijderd, wordt ook de SofiaAccount verwijdert.
Na activatie ziet de persoon dit:
Instellingen van login
Met de instellingen knop kan de persoon zijn user- en inlog-instellingen wijzingen, zoals de gebruikersnaam, email, wachtwoord, en 2FA instellen (zelfde 2FA menu als op AMBER)
Login pagina
Op de login pagina kan je nu kiezen om in te loggen met een streepsysteem account:
Dan krijg je deze inlog pagina:
Als je 2FA hebt aangezet, krijg je daarna nog een pagina waar je de token moet invullen.
Wachtwoord vergeten
Als je drukt op "wachtwoord vergeten?" krijg je de volgende pagina:
Als je je gegevens invult krijg je een email op het emailadres gelinkt aan de user:
Als je op de link klikt kom je op het volgende scherm, waar je een nieuw wachtwoord instellen:
Als je dat niet binnen 1 dag doet, verloopt de activatiecode. Na het verlopen, moet ze zelf weer op "wachtwoord vergeten?" klikken op de inlog pagina
Resetten van activatie token
Voor het activeren van een nieuw account heeft een user 5 dagen, daarna is de link verlopen. Maar in de activatiemail staat een 2e link, en als je daar op klikt dan wordt er een nieuwe activatielink gemaakt die vanaf dan 1 dag geldig is, en die wordt naar hetzelfde emailadres gemailt:
Deactiveren user
Wanneer een treasurer een user deactiveerd kan die user niet meer inloggen met zijn gelinkte streepsysteem account. Het streepsysteem account blijft wel bestaan, want als de user weer wordt geactiveerd dan moet die wel weer kunnen inloggen.
Technische details
SofiaAccountwaar logingegevens in worden opgeslagen. Deze is altijd gelinkt aan eenUseren wordt verwijdert wanneer de user wordt verwijdert.callback_controllerstaat de logica van het inloggen met een streepsysteem account.omniauth-identitygebruikt om dit zo te maken.Summary by CodeRabbit
New Features
UI / Style
Localization & Emails
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.