Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Testcases for the Ominiauth controller openid connect #869

Merged
merged 21 commits into from
Sep 11, 2024

Conversation

200455939-yashu
Copy link
Collaborator

Fixes # .

Changes proposed in this PR: Adding the Testcases for the Omniauth Controller for the openid_connect.

@aaronskiba aaronskiba self-requested a review August 29, 2024 17:21
Copy link
Collaborator

@aaronskiba aaronskiba left a comment

Choose a reason for hiding this comment

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

Tests are still breaking due to the following:

redirect_uri: Rails.application.secrets.omniauth_full_host + "/users/auth/openid_connect/callback"
                                                                 ^ (NoMethodError)

Does that change still have to be made? Or maybe this PR needs to be rebased?

Comment on lines 19 to 29
# Mock OmniAuth data for OpenID Connect with necessary info
OmniAuth.config.mock_auth[:openid_connect] = OmniAuth::AuthHash.new({
provider: 'openid_connect',
uid: '12345',
info: {
email: '[email protected]',
first_name: 'Test',
last_name: 'User',
name: 'Test User'
}
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't need to define this here since it is already on spec_helper.rb

roadmap/spec/spec_helper.rb

Lines 126 to 136 in 4da5fdd

OmniAuth.config.mock_auth[:openid_connect] = OmniAuth::AuthHash.new(
{
provider: 'openid_connect',
uid: '12345',
info: {
email: '[email protected]',
first_name: 'John',
last_name: 'Doe'
}
}
)

context 'when the email is missing and the user does not exist' do
before do
# Simulate missing email
OmniAuth.config.mock_auth[:openid_connect].info.email = nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we reset the email after this test in case we use it in another test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In this test case, we want to ensure it returns a 'nil' value. Replacing the values will not guarantee the nil results. 

Comment on lines 45 to 47
def User.from_omniauth(_auth)
nil
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

How does monkey patching this method work here? Will it be reset after the tests are done ? Or do we need to reset it for the rest of the tests ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we change the values we are using to get the nil value as a return instead of monkey patching this method ?

Comment on lines 63 to 69
def User.from_omniauth(_auth)
User.find_by(email: '[email protected]')
end

def IdentifierScheme.find_by_name(provider_name)
IdentifierScheme.find_by(name: provider_name)
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same question about monkey patching these methods. Do we need to reset them ?

Comment on lines 88 to 90
User.define_singleton_method(:from_omniauth) do |_auth|
nil
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same question about monkey patching

Copy link
Collaborator

@lagoan lagoan left a comment

Choose a reason for hiding this comment

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

This looks like a good start. I see we are getting the correct results from the tests.

Could we have the tests without monkey patching methods and changing the defined test values from the helper ?


it "links identifier to current user, sets flash notice, and redirects to root path" do
expect {
get :openid_connect
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm confused why get :openid_connect is working.
get :openid_connect seems to behave like post :openid_connect within these tests, but the same is not true outside of the tests.
Screenshot from 2024-09-04 16-13-46
Screenshot from 2024-09-04 16-13-11

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated this

RSpec.describe Users::OmniauthCallbacksController, type: :controller do
before do
# Enable test mode for OmniAuth
OmniAuth.config.test_mode = true
Copy link
Collaborator

Choose a reason for hiding this comment

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

spec/spec_helper.rb:124 of yashu-sso-link-accounts also has OmniAuth.config.test_mode = true. Do we need the same statement here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated this

get :openid_connect

expect(flash[:alert]).to eq(
"The current #{@identifier_scheme.description} iD has been already linked to a user with email #{different_user.email}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should probably be updated to match the changes you made in app/controllers/users/omniauth_callbacks_controller.rb:57

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

String value is going to be same as expected here. I have changed the way of passing the variable into this string.

Copy link
Collaborator

@lagoan lagoan left a comment

Choose a reason for hiding this comment

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

LGTM!

@aaronskiba aaronskiba merged commit a71c056 into yashu-sso-link-accounts Sep 11, 2024
11 checks passed
@aaronskiba aaronskiba deleted the yashu-controller-spec branch September 11, 2024 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants