Show correct email for authorization confirmation #11667

vrajmohan · 2024-12-18T15:46:13Z

🎫 Ticket

Link to the relevant ticket:
Authorization confirmation doesn't show selected email for partner

🛠 Summary of changes

Refactored code - added a ServiceProviderIdentity#email_address_for_sharing that does the right thing
Added a test to verify correct behavior in SAML
Eliminated EmailContext class that seemed to be of limited value.

📜 Testing Plan

Use the "Steps to reproduce" from the Jira ticket.

kevinsmaster5 · 2024-12-19T13:28:10Z

app/models/user.rb

+  def last_sign_in_email_address
+    confirmed_email_addresses.first
+  end
+


I like how you are using this to simplify things like EmailContext.new(current_user)...etc

kevinsmaster5

Looks good and works as expected in bug ticket.

aduth

Nice refactors 👍

aduth · 2024-12-19T13:59:39Z

app/decorators/email_context.rb

-  def email_address_count
-    user.email_addresses.count
-  end


I don't think these stubs were valid anyways, but might have been trying to refer to this method now removed and we could remove the stub as well?

identity-idp/spec/controllers/accounts/connected_accounts/selected_email_controller_spec.rb

Line 79 in b5c849e

allow(user).to receive(:email_address_count).and_return(2)

identity-idp/spec/controllers/sign_up/select_email_controller_spec.rb

Line 76 in b5c849e

allow(user).to receive(:email_address_count).and_return(2)

app/models/service_provider_identity.rb

aduth · 2024-12-19T14:02:59Z

app/models/user.rb

+  def last_sign_in_email_address
+    confirmed_email_addresses.first
+  end


Similar comment about test coverage in spec/models/user_spec.rb. I'd be specifically concerned to have regression coverage in how we rely on confirmed_email_addresses to be ordered here, which I wouldn't think is a given.

IMO, it would be better to use scopes as shown at the end of https://api.rubyonrails.org/v8.0.1/classes/ActiveRecord/Scoping/Named/ClassMethods.html#method-i-scope. Should I go ahead?

What would scopes do for us here, or at least specifically in regard to my comment?

We would be explicit when we say user.email_addresses.confirmed.last_signed_in.

vrajmohan · 2024-12-20T15:27:44Z

Just realizing that I have misunderstood active service provider identities. Fixing...

aduth · 2024-12-20T16:16:55Z

Can you resolve the merge conflict?

aduth · 2024-12-20T16:18:39Z

spec/models/service_provider_identity_spec.rb

+      context 'when an email address for sharing has been set' do
+        it 'returns the shared email' do
+          identity.email_address = shared_email_address


Minor: I might expect the assignment here to happen in a before block so that if we added additional test cases to this context, each would have the same common behavior of "sharing has been set" without having to duplicate the code. Similar to what we're doing with feature flag enabling.

Suggested change

context 'when an email address for sharing has been set' do

it 'returns the shared email' do

identity.email_address = shared_email_address

context 'when an email address for sharing has been set' do

before do

identity.email_address = shared_email_address

end

it 'returns the shared email' do

Same comment applies for context block below this.

aduth · 2024-12-20T16:20:58Z

app/models/user.rb

-    email_addresses.where.not(confirmed_at: nil).order('last_sign_in_at DESC NULLS LAST')
+    email_addresses.confirmed


How are we ensuring last_sign_in_at ordering with this change?

This line by itself does not ensure that. The change to User#last_sign_in_email_address on line 524 does it.

After further discussion with @aduth, we decided to leave the sort order in, even though the tests pass. We plan to clean this up in a separate PR.

Avoid duplicating logic in OIDC and SAML changelog: Upcoming Features, Partner account, Select email to share with partner

aduth · 2024-12-20T19:02:03Z

spec/controllers/sign_up/select_email_controller_spec.rb

I think a rebase might have gone awry, reintroducing code removed in #11656 (causing build failure).

Yep, saw that! Fixing.

Clean this up in a separate refactoring

vrajmohan requested a review from a team December 18, 2024 15:46

vrajmohan force-pushed the vm-correct-email-for-authorization-confirmation branch from 614c31a to b5c849e Compare December 19, 2024 04:29

vrajmohan marked this pull request as ready for review December 19, 2024 04:30

kevinsmaster5 reviewed Dec 19, 2024

View reviewed changes

kevinsmaster5 approved these changes Dec 19, 2024

View reviewed changes

aduth reviewed Dec 19, 2024

View reviewed changes

vrajmohan force-pushed the vm-correct-email-for-authorization-confirmation branch from e17819d to be8d297 Compare December 20, 2024 01:52

vrajmohan force-pushed the vm-correct-email-for-authorization-confirmation branch from be8d297 to 4aa4f42 Compare December 20, 2024 15:34

vrajmohan requested a review from aduth December 20, 2024 16:09

aduth reviewed Dec 20, 2024

View reviewed changes

vrajmohan added 6 commits December 20, 2024 10:03

Extract common method email_address_for_sharing

134a6ff

Avoid duplicating logic in OIDC and SAML changelog: Upcoming Features, Partner account, Select email to share with partner

Get authorization using email selected for sharing if available

8fef59c

Eliminate any lingering smells

4fc7e89

Fix typo

8573f35

Add test for email shown in SAML authorization confirmation

7027063

Get rid of EmailContext

c5ebe86

vrajmohan force-pushed the vm-correct-email-for-authorization-confirmation branch from 4aa4f42 to 7d8fc14 Compare December 20, 2024 18:06

Add test for email shown in OIDC authorization confirmation

bf42fe1

aduth reviewed Dec 20, 2024

View reviewed changes

vrajmohan added 6 commits December 20, 2024 11:02

Clean up test fixtures

29dd15c

Add tests for ServiceProviderIdentity#email_address_for_sharing

1acd864

Use scope and class method to clean up implementation

de148cb

Restore sorting

57b077c

Clean this up in a separate refactoring

Use before block for fixture

076f430

Share email only when the identity is "active"

b83e589

vrajmohan force-pushed the vm-correct-email-for-authorization-confirmation branch from 7d8fc14 to b83e589 Compare December 20, 2024 19:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show correct email for authorization confirmation #11667

Show correct email for authorization confirmation #11667

vrajmohan commented Dec 18, 2024

kevinsmaster5 Dec 19, 2024

kevinsmaster5 left a comment

aduth left a comment

aduth Dec 19, 2024

aduth Dec 19, 2024

vrajmohan Dec 19, 2024

aduth Dec 19, 2024

vrajmohan Dec 19, 2024

vrajmohan commented Dec 20, 2024

aduth commented Dec 20, 2024

aduth Dec 20, 2024

aduth Dec 20, 2024

vrajmohan Dec 20, 2024

vrajmohan Dec 20, 2024

aduth Dec 20, 2024

vrajmohan Dec 20, 2024

		email_addresses.where.not(confirmed_at: nil).order('last_sign_in_at DESC NULLS LAST')
		email_addresses.confirmed

Show correct email for authorization confirmation #11667

Are you sure you want to change the base?

Show correct email for authorization confirmation #11667

Conversation

vrajmohan commented Dec 18, 2024

🎫 Ticket

🛠 Summary of changes

📜 Testing Plan

Choose a reason for hiding this comment

kevinsmaster5 left a comment

Choose a reason for hiding this comment

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrajmohan commented Dec 20, 2024

aduth commented Dec 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment