achedeuzot / ueberauth_auth0

Auth0 OAuth2 strategy for Überauth.
https://hexdocs.pm/ueberauth_auth0
MIT License
71 stars 46 forks source link

Adds computed configurations #161

Open silvagustin opened 3 years ago

silvagustin commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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.

silvagustin commented 1 year ago

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.