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

Adds computed configurations #161

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

silvagustin
Copy link

Hi!

First of all I want to thank you for creating ueberauth-auth0 dependency.

I've made some small changes to the dependency to allow computed configurations. This was based on the need of multi tenancy support.

This was made possible thanks to bracket7/ueberauth_auth0 fork. I took his fork and adapted it to the current version of ueberauth-auth0 dep and updated the tests.

I followed the rules mentioned on the CONTRIBUTING.md file.

Any feedback would be appreacited.

Cheers.

@achedeuzot
Copy link
Owner

Hi @silvagustin !

Thanks for your contribution 🎉 Seems like a neat additional feature 💪

Would you be so kind as to add a few tests that covers the logic in compute_configs ? They will also serve as a good example of how to use it with or without computed configs and ensure everything keeps running smoothly.

I'll also give it a try in the upcoming weeks if I can before merging. I'm extra careful with this as auth systems can be critical components in some companies ;)

Thanks again !

P.S. I think I'll need to update the CI configuration to use new github actions as the current one is outdated and might fail.

@silvagustin
Copy link
Author

Hello again!

Of course, I'll work on adding some tests for the compute_configs. It's my first time contributing to the Elixir community so any feedback will be well received 😁.

Cheers.

@achedeuzot
Copy link
Owner

Cool :) You'll see tests that require an external call use exvcr to return sample payloads.

For the CI, you'll need to rebase your changes on master, I had to update the CI image as the previous one was stale dans wouldn't build anymore.

Cheers !

@silvagustin
Copy link
Author

@achedeuzot

The first thing I did before continued working on this was rebase this branch with master.

I'm not sure if what I did is what you expected. The only way I found to test it was to:

Tests are working fine by running mix test.all on Terminal:

Screenshot

If this is fine then the next step would be to update the github workflows file.

Let me know your thoughts.

Cheers.

Copy link
Owner

@achedeuzot achedeuzot left a comment

Choose a reason for hiding this comment

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

Thanks for the cool contribution, I left a few comments here and there if it's OK for you :)

fixture/vcr_cassettes/auth0-invalid-code.json Outdated Show resolved Hide resolved
test/strategy/auth0/oauth_test.exs Outdated Show resolved Hide resolved
@@ -21,7 +22,8 @@ defmodule Ueberauth.Strategy.Auth0.OAuthTest do

test "raises when there is no configuration" do
assert_raise(RuntimeError, ~r/^Expected to find settings under.*/, fn ->
client(otp_app: :unknown_auth0_otp_app)
conn = %Plug.Conn{}
client(conn, otp_app: :unknown_auth0_otp_app)
Copy link
Owner

@achedeuzot achedeuzot May 4, 2021

Choose a reason for hiding this comment

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

I'd add a few tests to check that the settings are fetched from the ConfigFrom module as expected.

Copy link
Author

Choose a reason for hiding this comment

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

I'm trying to get rid of the mix test.all alias to just use mix test.

I've added some tests to oauth_test.exs to check that the ConfigFrom module was set as expected. Unfortunately, they're not working and I believe it's because the configurations are being wrongly loaded on runtime (see "How to dynamically load configurations for tests at runtime within Elixir libraries").

Do you think it's necessary for this case to create multiple environments to test the possible configurations?

Another references:

Copy link
Author

Choose a reason for hiding this comment

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

I forgot to mention that Mix.Config is deprecated: https://hexdocs.pm/mix/Mix.Config.html#read!/2

Copy link
Author

Choose a reason for hiding this comment

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

@achedeuzot I'm not sure if you saw this comment before commented "LGTM 🎉 " at the bottom 😬

Copy link
Owner

@achedeuzot achedeuzot May 8, 2021

Choose a reason for hiding this comment

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

Ah no sorry, I commented LGTM before running the CI tests. I see the errors.

If it works with the mix test.all, I'm okay with it until we may find a better way :)
It's true the Mix.Config.read! approach seemed better / cleaner.

Copy link
Author

Choose a reason for hiding this comment

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

I've changed the deprecated Mix.Config with Config. Also, I continued working on the OAuthTest module but I couldn't make the tests related to config_from pass. I don't know why the config loading from the test setup is not working. I've also tried creating multiple mix aliases on another branch, hoping that if the App is being recompiled each time with a different environment but it also failed.

Can you take a look to the code? Maybe you can find something that my eyes are not seeing.

Cheers.

Copy link
Owner

Choose a reason for hiding this comment

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

Hey @silvagustin,

Thanks so much for your efforts to make the tests work, really appreciate it ! Just a quick message to let you know that I'll be having a lot on my plate in the upcoming days and won't be available before at least next Tuesday (18th of may) to look into it. I'll look at it then.

Otherwise, I'm okay if you want to revert to one of your other solutions that seemed to work (with the 2 environments in mix.exs) if you want to move forward with this PR.

Cheers,

lib/ueberauth/strategy/auth0/oauth.ex Show resolved Hide resolved
mix.exs Outdated Show resolved Hide resolved
achedeuzot
achedeuzot previously approved these changes May 7, 2021
Copy link
Owner

@achedeuzot achedeuzot left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@silvagustin
Copy link
Author

Hello again @achedeuzot.

It's been a while since I created this PR and days ago I've decided to update it by rebasing this branch with master and fix the tests that weren't working.

Can you review it again?

Cheers.

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.

None yet

2 participants