From 227e29ce0a6fa8e3156fdd0cf348756ea3cd1958 Mon Sep 17 00:00:00 2001 From: Dan Milne Date: Sun, 26 Oct 2025 20:13:39 +1100 Subject: [PATCH] Fix/add some tests. Configure email sending address --- app/mailers/application_mailer.rb | 2 +- test/fixtures/application_groups.yml | 12 +- test/fixtures/oidc_user_consents.yml | 16 +- test/fixtures/user_groups.yml | 12 +- test/models/oidc_access_token_test.rb | 215 ++++++++++++++++++- test/models/oidc_authorization_code_test.rb | 192 ++++++++++++++++- test/models/oidc_user_consent_test.rb | 225 +++++++++++++++++++- 7 files changed, 644 insertions(+), 30 deletions(-) diff --git a/app/mailers/application_mailer.rb b/app/mailers/application_mailer.rb index 3c34c81..dfd6eb6 100644 --- a/app/mailers/application_mailer.rb +++ b/app/mailers/application_mailer.rb @@ -1,4 +1,4 @@ class ApplicationMailer < ActionMailer::Base - default from: "from@example.com" + default from: ENV.fetch('CLINCH_EMAIL_FROM', 'clinch@example.com'), layout "mailer" end diff --git a/test/fixtures/application_groups.yml b/test/fixtures/application_groups.yml index bd80855..d49c5b2 100644 --- a/test/fixtures/application_groups.yml +++ b/test/fixtures/application_groups.yml @@ -1,9 +1,9 @@ # Read about fixtures at https://api.rubyonrails.org/classes/ActiveRecord/FixtureSet.html -one: - application: one - group: one +kavita_admin_group: + application: kavita_app + group: admin_group -two: - application: two - group: two +kavita_editor_group: + application: kavita_app + group: editor_group diff --git a/test/fixtures/oidc_user_consents.yml b/test/fixtures/oidc_user_consents.yml index cfe2c3f..27cb412 100644 --- a/test/fixtures/oidc_user_consents.yml +++ b/test/fixtures/oidc_user_consents.yml @@ -1,13 +1,13 @@ # Read about fixtures at https://api.rubyonrails.org/classes/ActiveRecord/FixtureSet.html -one: - user: one - application: one - scopes_granted: MyText +alice_consent: + user: alice + application: kavita_app + scopes_granted: openid profile email granted_at: 2025-10-24 16:57:39 -two: - user: two - application: two - scopes_granted: MyText +bob_consent: + user: bob + application: another_app + scopes_granted: openid email groups granted_at: 2025-10-24 16:57:39 diff --git a/test/fixtures/user_groups.yml b/test/fixtures/user_groups.yml index 45383c3..04bd86b 100644 --- a/test/fixtures/user_groups.yml +++ b/test/fixtures/user_groups.yml @@ -1,9 +1,9 @@ # Read about fixtures at https://api.rubyonrails.org/classes/ActiveRecord/FixtureSet.html -one: - user: one - group: one +alice_admin_group: + user: alice + group: admin_group -two: - user: two - group: two +bob_editor_group: + user: bob + group: editor_group diff --git a/test/models/oidc_access_token_test.rb b/test/models/oidc_access_token_test.rb index 3c4213d..ba4b5d4 100644 --- a/test/models/oidc_access_token_test.rb +++ b/test/models/oidc_access_token_test.rb @@ -1,7 +1,216 @@ require "test_helper" class OidcAccessTokenTest < ActiveSupport::TestCase - # test "the truth" do - # assert true - # end + def setup + @access_token = oidc_access_tokens(:one) + end + + test "should be valid with all required attributes" do + assert @access_token.valid? + end + + test "should belong to an application" do + assert_respond_to @access_token, :application + assert_equal applications(:kavita_app), @access_token.application + end + + test "should belong to a user" do + assert_respond_to @access_token, :user + assert_equal users(:alice), @access_token.user + end + + test "should generate token before validation on create" do + new_token = OidcAccessToken.new( + application: applications(:kavita_app), + user: users(:alice) + ) + assert_nil new_token.token + assert new_token.save + assert_not_nil new_token.token + assert_match /^[A-Za-z0-9_-]+$/, new_token.token + end + + test "should set expiry before validation on create" do + new_token = OidcAccessToken.new( + application: applications(:kavita_app), + user: users(:alice) + ) + assert_nil new_token.expires_at + assert new_token.save + assert_not_nil new_token.expires_at + assert new_token.expires_at > Time.current + assert new_token.expires_at <= 61.minutes.from_now # Allow some variance + end + + test "should validate presence of token" do + @access_token.token = nil + assert_not @access_token.valid? + assert_includes @access_token.errors[:token], "can't be blank" + end + + test "should validate uniqueness of token" do + @access_token.save! if @access_token.changed? + duplicate = OidcAccessToken.new( + token: @access_token.token, + application: applications(:another_app), + user: users(:bob) + ) + assert_not duplicate.valid? + assert_includes duplicate.errors[:token], "has already been taken" + end + + test "should identify expired tokens correctly" do + @access_token.expires_at = 5.minutes.ago + assert @access_token.expired?, "Should identify past expiry as expired" + + @access_token.expires_at = 5.minutes.from_now + assert_not @access_token.expired?, "Should identify future expiry as not expired" + + @access_token.expires_at = Time.current + assert @access_token.expired?, "Should identify current time as expired" + end + + test "should identify active tokens correctly" do + # Non-expired token should be active + @access_token.expires_at = 5.minutes.from_now + assert @access_token.active?, "Future expiry should be active" + + # Expired token should not be active + @access_token.expires_at = 5.minutes.ago + assert_not @access_token.active?, "Past expiry should not be active" + + # Current time should be considered expired (not active) + @access_token.expires_at = Time.current + assert_not @access_token.active?, "Current time should not be active" + end + + test "should revoke token correctly" do + @access_token.expires_at = 1.hour.from_now + original_expiry = @access_token.expires_at + assert @access_token.active?, "Token should be active before revocation" + + @access_token.revoke! + @access_token.reload + + assert @access_token.expired?, "Token should be expired after revocation" + assert @access_token.expires_at <= Time.current, "Expiry should be set to current time or earlier" + assert @access_token.expires_at < original_expiry, "Expiry should be changed from original" + end + + test "valid scope should return only non-expired tokens" do + # Create tokens with different states + valid_token = OidcAccessToken.create!( + application: applications(:kavita_app), + user: users(:alice) + ) + + expired_token = OidcAccessToken.create!( + application: applications(:kavita_app), + user: users(:alice), + expires_at: 5.minutes.ago + ) + + valid_tokens = OidcAccessToken.valid + assert_includes valid_tokens, valid_token + assert_not_includes valid_tokens, expired_token + end + + test "expired scope should return only expired tokens" do + # Create tokens with different expiry states + non_expired_token = OidcAccessToken.create!( + application: applications(:kavita_app), + user: users(:alice), + expires_at: 1.hour.from_now + ) + + expired_token = OidcAccessToken.create!( + application: applications(:kavita_app), + user: users(:alice), + expires_at: 5.minutes.ago + ) + + expired_tokens = OidcAccessToken.expired + assert_includes expired_tokens, expired_token + assert_not_includes expired_tokens, non_expired_token + end + + test "should handle concurrent revocation safely" do + @access_token.expires_at = 1.hour.from_now + @access_token.save! + + original_active = @access_token.active? + @access_token.revoke! + + assert original_active, "Token should be active before revocation" + assert @access_token.expired?, "Token should be expired after revocation" + end + + test "should generate secure random tokens" do + tokens = [] + 5.times do + token = OidcAccessToken.create!( + application: applications(:kavita_app), + user: users(:alice) + ) + tokens << token.token + end + + # All tokens should be unique + assert_equal tokens.length, tokens.uniq.length + + # All tokens should match the expected pattern + tokens.each do |token| + assert_match /^[A-Za-z0-9_-]+$/, token + assert_equal 63, token.length # Base64 with padding removed (48 bytes = 64 chars, minus padding) + end + end + + test "should have longer token than authorization codes" do + auth_code = OidcAuthorizationCode.create!( + application: applications(:kavita_app), + user: users(:alice), + redirect_uri: "https://example.com/callback" + ) + + access_token = OidcAccessToken.create!( + application: applications(:kavita_app), + user: users(:alice) + ) + + assert access_token.token.length > auth_code.code.length, + "Access tokens should be longer than authorization codes" + end + + test "should have appropriate expiry times" do + auth_code = OidcAuthorizationCode.create!( + application: applications(:kavita_app), + user: users(:alice), + redirect_uri: "https://example.com/callback" + ) + + access_token = OidcAccessToken.create!( + application: applications(:kavita_app), + user: users(:alice) + ) + + # Authorization codes expire in 10 minutes, access tokens in 1 hour + assert access_token.expires_at > auth_code.expires_at, + "Access tokens should have longer expiry than authorization codes" + end + + test "revoked tokens should not appear in valid scope" do + access_token = OidcAccessToken.create!( + application: applications(:kavita_app), + user: users(:alice) + ) + + # Token should be in valid scope initially + assert_includes OidcAccessToken.valid, access_token + + # Revoke the token + access_token.revoke! + + # Token should no longer be in valid scope + assert_not_includes OidcAccessToken.valid, access_token + end end diff --git a/test/models/oidc_authorization_code_test.rb b/test/models/oidc_authorization_code_test.rb index 50717df..6a90bca 100644 --- a/test/models/oidc_authorization_code_test.rb +++ b/test/models/oidc_authorization_code_test.rb @@ -1,7 +1,193 @@ require "test_helper" class OidcAuthorizationCodeTest < ActiveSupport::TestCase - # test "the truth" do - # assert true - # end + def setup + @auth_code = oidc_authorization_codes(:one) + end + + test "should be valid with all required attributes" do + assert @auth_code.valid? + end + + test "should belong to an application" do + assert_respond_to @auth_code, :application + assert_equal applications(:kavita_app), @auth_code.application + end + + test "should belong to a user" do + assert_respond_to @auth_code, :user + assert_equal users(:alice), @auth_code.user + end + + test "should generate code before validation on create" do + new_code = OidcAuthorizationCode.new( + application: applications(:kavita_app), + user: users(:alice), + redirect_uri: "https://example.com/callback" + ) + assert_nil new_code.code + assert new_code.save + assert_not_nil new_code.code + assert_match /^[A-Za-z0-9_-]+$/, new_code.code + end + + test "should set expiry before validation on create" do + new_code = OidcAuthorizationCode.new( + application: applications(:kavita_app), + user: users(:alice), + redirect_uri: "https://example.com/callback" + ) + assert_nil new_code.expires_at + assert new_code.save + assert_not_nil new_code.expires_at + assert new_code.expires_at > Time.current + assert new_code.expires_at <= 11.minutes.from_now # Allow some variance + end + + test "should validate presence of code" do + @auth_code.code = nil + assert_not @auth_code.valid? + assert_includes @auth_code.errors[:code], "can't be blank" + end + + test "should validate uniqueness of code" do + @auth_code.save! if @auth_code.changed? + duplicate = OidcAuthorizationCode.new( + code: @auth_code.code, + application: applications(:another_app), + user: users(:bob), + redirect_uri: "https://example.com/callback" + ) + assert_not duplicate.valid? + assert_includes duplicate.errors[:code], "has already been taken" + end + + test "should validate presence of redirect_uri" do + @auth_code.redirect_uri = nil + assert_not @auth_code.valid? + assert_includes @auth_code.errors[:redirect_uri], "can't be blank" + end + + test "should identify expired codes correctly" do + @auth_code.expires_at = 5.minutes.ago + assert @auth_code.expired?, "Should identify past expiry as expired" + + @auth_code.expires_at = 5.minutes.from_now + assert_not @auth_code.expired?, "Should identify future expiry as not expired" + + @auth_code.expires_at = Time.current + assert @auth_code.expired?, "Should identify current time as expired" + end + + test "should identify usable codes correctly" do + # Fresh, unused code should be usable + @auth_code.expires_at = 5.minutes.from_now + @auth_code.used = false + assert @auth_code.usable?, "Fresh unused code should be usable" + + # Used code should not be usable + @auth_code.used = true + assert_not @auth_code.usable?, "Used code should not be usable" + + # Expired code should not be usable + @auth_code.used = false + @auth_code.expires_at = 5.minutes.ago + assert_not @auth_code.usable?, "Expired code should not be usable" + + # Used and expired code should not be usable + @auth_code.used = true + @auth_code.expires_at = 5.minutes.ago + assert_not @auth_code.usable?, "Used and expired code should not be usable" + end + + test "should consume code correctly" do + @auth_code.used = false + assert_not @auth_code.used?, "Code should initially be unused" + + @auth_code.consume! + @auth_code.reload + assert @auth_code.used?, "Code should be marked as used after consumption" + end + + test "valid scope should return only unused and non-expired codes" do + # Create codes with different states + valid_code = OidcAuthorizationCode.create!( + application: applications(:kavita_app), + user: users(:alice), + redirect_uri: "https://example.com/callback" + ) + + used_code = OidcAuthorizationCode.create!( + application: applications(:kavita_app), + user: users(:alice), + redirect_uri: "https://example.com/callback", + used: true + ) + + expired_code = OidcAuthorizationCode.create!( + application: applications(:kavita_app), + user: users(:alice), + redirect_uri: "https://example.com/callback", + expires_at: 5.minutes.ago + ) + + valid_codes = OidcAuthorizationCode.valid + assert_includes valid_codes, valid_code + assert_not_includes valid_codes, used_code + assert_not_includes valid_codes, expired_code + end + + test "expired scope should return only expired codes" do + # Create codes with different expiry states + non_expired_code = OidcAuthorizationCode.create!( + application: applications(:kavita_app), + user: users(:alice), + redirect_uri: "https://example.com/callback", + expires_at: 5.minutes.from_now + ) + + expired_code = OidcAuthorizationCode.create!( + application: applications(:kavita_app), + user: users(:alice), + redirect_uri: "https://example.com/callback", + expires_at: 5.minutes.ago + ) + + expired_codes = OidcAuthorizationCode.expired + assert_includes expired_codes, expired_code + assert_not_includes expired_codes, non_expired_code + end + + test "should handle concurrent consumption safely" do + @auth_code.used = false + @auth_code.save! + + # Simulate concurrent consumption + original_used = @auth_code.used? + @auth_code.consume! + + assert_not original_used, "Code should be unused before consumption" + assert @auth_code.used?, "Code should be used after consumption" + end + + test "should generate secure random codes" do + codes = [] + 5.times do + code = OidcAuthorizationCode.create!( + application: applications(:kavita_app), + user: users(:alice), + redirect_uri: "https://example.com/callback" + ) + codes << code.code + end + + # All codes should be unique + assert_equal codes.length, codes.uniq.length + + # All codes should match the expected pattern + codes.each do |code| + assert_match /^[A-Za-z0-9_-]+$/, code + assert_equal 43, code.length # Base64 padding removed + end + end end diff --git a/test/models/oidc_user_consent_test.rb b/test/models/oidc_user_consent_test.rb index fe5bccf..3762992 100644 --- a/test/models/oidc_user_consent_test.rb +++ b/test/models/oidc_user_consent_test.rb @@ -1,7 +1,226 @@ require "test_helper" class OidcUserConsentTest < ActiveSupport::TestCase - # test "the truth" do - # assert true - # end + def setup + @consent = oidc_user_consents(:alice_consent) + end + + test "should be valid with all required attributes" do + assert @consent.valid? + end + + test "should belong to a user" do + assert_respond_to @consent, :user + assert_equal users(:alice), @consent.user + end + + test "should belong to an application" do + assert_respond_to @consent, :application + assert_equal applications(:kavita_app), @consent.application + end + + test "should validate presence of user" do + @consent.user = nil + assert_not @consent.valid? + assert_includes @consent.errors[:user], "can't be blank" + end + + test "should validate presence of application" do + @consent.application = nil + assert_not @consent.valid? + assert_includes @consent.errors[:application], "can't be blank" + end + + test "should validate presence of scopes_granted" do + @consent.scopes_granted = nil + assert_not @consent.valid? + assert_includes @consent.errors[:scopes_granted], "can't be blank" + end + + test "should validate presence of granted_at" do + @consent.granted_at = nil + assert_not @consent.valid? + assert_includes @consent.errors[:granted_at], "can't be blank" + end + + test "should validate uniqueness of user_id scoped to application_id" do + # Should be able to create consent for different user with same app + new_consent = OidcUserConsent.new( + user: users(:bob), + application: @consent.application, + scopes_granted: "openid email" + ) + assert new_consent.valid? + + # Should NOT be able to create consent for same user with same app + duplicate_consent = OidcUserConsent.new( + user: @consent.user, + application: @consent.application, + scopes_granted: "openid profile" + ) + assert_not duplicate_consent.valid? + assert_includes duplicate_consent.errors[:user_id], "has already been taken" + end + + test "should set granted_at before validation on create" do + new_consent = OidcUserConsent.new( + user: users(:bob), + application: applications(:another_app), + scopes_granted: "openid email" + ) + assert_nil new_consent.granted_at + assert new_consent.save + assert_not_nil new_consent.granted_at + assert new_consent.granted_at <= Time.current + end + + test "scopes should parse space-separated scopes into array" do + @consent.scopes_granted = "openid profile email groups" + assert_equal ["openid", "profile", "email", "groups"], @consent.scopes + + # Handle single scope + @consent.scopes_granted = "openid" + assert_equal ["openid"], @consent.scopes + + # Handle empty string + @consent.scopes_granted = "" + assert_equal [], @consent.scopes + + # Handle extra spaces + @consent.scopes_granted = "openid profile email" + assert_equal ["openid", "profile", "email"], @consent.scopes + end + + test "scopes= should join array into space-separated string" do + @consent.scopes = ["openid", "profile", "email"] + assert_equal "openid profile email", @consent.scopes_granted + + # Handle single item array + @consent.scopes = ["openid"] + assert_equal "openid", @consent.scopes_granted + + # Handle empty array + @consent.scopes = [] + assert_equal "", @consent.scopes_granted + + # Handle duplicates + @consent.scopes = ["openid", "profile", "openid"] + assert_equal "openid profile", @consent.scopes_granted + end + + test "should handle string input for scopes=" do + @consent.scopes = "openid profile" + assert_equal "openid profile", @consent.scopes_granted + assert_equal ["openid", "profile"], @consent.scopes + end + + test "covers_scopes? should correctly identify scope coverage" do + @consent.scopes_granted = "openid profile email groups" + + # Should cover when all requested scopes are granted + assert @consent.covers_scopes?(["openid"]), "Should cover single requested scope" + assert @consent.covers_scopes?(["openid", "profile"]), "Should cover multiple requested scopes" + assert @consent.covers_scopes?(["email", "groups"]), "Should cover different combination" + assert @consent.covers_scopes?(["openid", "profile", "email", "groups"]), "Should cover all granted scopes" + + # Should not cover when requested includes non-granted scope + assert_not @consent.covers_scopes?(["admin"]), "Should not cover non-granted scope" + assert_not @consent.covers_scopes?(["openid", "admin"]), "Should not cover mixed granted/non-granted" + assert_not @consent.covers_scopes?(["admin", "write"]), "Should not cover all non-granted" + + # Handle string input + assert @consent.covers_scopes?("openid"), "Should handle string input" + assert_not @consent.covers_scopes?("admin"), "Should handle string input for non-granted scope" + + # Handle empty requested scopes + assert @consent.covers_scopes?([]), "Should cover empty array" + assert @consent.covers_scopes?(nil), "Should handle nil input" + end + + test "covers_scopes? should handle edge cases" do + # Consent with no scopes + @consent.scopes_granted = "" + assert_not @consent.covers_scopes?(["openid"]), "Should not cover any scope when no scopes granted" + assert @consent.covers_scopes?([]), "Should cover empty request when no scopes granted" + + # Consent with one scope + @consent.scopes_granted = "openid" + assert @consent.covers_scopes?(["openid"]), "Should cover matching single scope" + assert_not @consent.covers_scopes?(["profile"]), "Should not cover different single scope" + end + + test "formatted_scopes should provide human-readable scope names" do + @consent.scopes_granted = "openid profile email groups" + expected = "Basic authentication, Profile information, Email address, Group membership" + assert_equal expected, @consent.formatted_scopes + + # Test single scope + @consent.scopes_granted = "openid" + assert_equal "Basic authentication", @consent.formatted_scopes + + # Test unknown scope + @consent.scopes_granted = "unknown_scope" + assert_equal "Unknown scope", @consent.formatted_scopes + + # Test mixed known and unknown + @consent.scopes_granted = "openid custom_scope" + assert_equal "Basic authentication, Custom scope", @consent.formatted_scopes + + # Test empty scopes + @consent.scopes_granted = "" + assert_equal "", @consent.formatted_scopes + end + + test "should maintain consistency between scopes getter and setter" do + original_scopes = ["openid", "profile", "email"] + @consent.scopes = original_scopes + assert_equal original_scopes, @consent.scopes + + # Modify scopes + new_scopes = ["openid", "groups"] + @consent.scopes = new_scopes + assert_equal new_scopes, @consent.scopes + end + + test "should handle consent updates correctly" do + # Create initial consent + consent = OidcUserConsent.create!( + user: users(:bob), + application: applications(:another_app), + scopes_granted: "openid email" + ) + + # Update to include more scopes + consent.scopes = ["openid", "email", "profile"] + consent.save! + + consent.reload + assert_equal ["openid", "email", "profile"], consent.scopes + assert_equal "openid email profile", consent.scopes_granted + + # Should still cover original scopes + assert consent.covers_scopes?(["openid", "email"]) + # Should cover new scopes + assert consent.covers_scopes?(["profile"]) + # Should cover all scopes + assert consent.covers_scopes?(["openid", "email", "profile"]) + end + + test "should validate scope coverage logic with real OIDC scenarios" do + # Typical OIDC consent scenario + @consent.scopes_granted = "openid profile email" + + # Application requests only openid (required for OIDC) + assert @consent.covers_scopes?(["openid"]), "Should cover required openid scope" + + # Application requests standard scopes + assert @consent.covers_scopes?(["openid", "profile"]), "Should cover standard OIDC scopes" + + # Application requests more than granted + assert_not @consent.covers_scopes?(["openid", "profile", "groups"]), + "Should not cover scopes not granted" + + # Application requests subset + assert @consent.covers_scopes?(["email"]), "Should cover subset of granted scopes" + end end