Add scope to auth return

[prplmg]

Adds scope key to the ElixirAuthGithub.github_auth return object

[prplmg]

@nelsonic can I request a review? It's just a small addition of scope info to allow for conditional handling

[nelsonic]

Hi @prplmg, 👋
Thanks very much for opening this PR to add the scope to the access_token;
seems like a useful addition. 👍

Very happy to merge this as it adds a useful feature.
But for future reference: avoid changing code that doesn't strictly need to be changed. It makes it much easier for module authors/maintainers to review & merge your code. In the case of set_user_details/2 having the return value changed will make this update a "breaking change" for someone using that function in the existing way so is unlikely to be merged.

Thanks again for contributing and happy Elixir-ing 🎉

[nelsonic]

As expected the tests fail: https://github.com/dwyl/elixir-auth-github/actions/runs/4930569458/jobs/8842364320?pr=81

this is because of the breaking change to set_user_details/2 which cascades through ...

We need to revert that change and then this PR can be merged. 👌

[nelsonic]

@prplmg if you can revert the change made to set_user_details/2 we can get this merged and published to Hex.pm today. 👌

[SimonLab]

@prplmg thanks for opening this PR.
However I'm trying to understand the use case for it.
When you using this package you already define the scope "manually" as a parameter to the login_url\1 function:

elixir-auth-github/lib/elixir_auth_github.ex

Lines 46 to 54 in a851e1e

	@doc """
	Identical to `login_url/0` except with an additional state and scope properties
	appended to the URL.
	"""
	def login_url(%{scopes: scopes, state: state}) do
	login_url() <>
	"&scope=#{Enum.join(scopes, "%20")}" <>
	"&state=#{state}"
	end

Is there an edge case where the scope is unkown to the person using this package? I'm not sure I understand why we need to add the scope information to the get_user_details Map.

Edit:

After reading the Github documentation, https://docs.github.com/en/apps/oauth-apps/building-oauth-apps/authorizing-oauth-apps#2-users-are-redirected-back-to-your-site-by-github
We can see that the json response when exchanging the code for an access_token is the following:

So the reply contains access_token, scope and token_type values. Still not sure about the scope information use case.

[prplmg]

@prplmg if you can revert the change made to set_user_details/2 we can get this merged and published to Hex.pm today. 👌

Just pushed some changes that reverts the changes to [set_user_details/2] and updates the tests, thanks for the input!

[prplmg]

@prplmg thanks for opening this PR. However I'm trying to understand the use case for it. When you using this package you already define the scope "manually" as a parameter to the login_url\1 function:

elixir-auth-github/lib/elixir_auth_github.ex

Lines 46 to 54 in a851e1e

	@doc """
	Identical to `login_url/0` except with an additional state and scope properties
	appended to the URL.
	"""
	def login_url(%{scopes: scopes, state: state}) do
	login_url() <>
	"&scope=#{Enum.join(scopes, "%20")}" <>
	"&state=#{state}"
	end

Is there an edge case where the scope is unkown to the person using this package? I'm not sure I understand why we need to add the scope information to the get_user_details Map.

Great question! We're developing an app where we can login via github, but not necessarity grant repo access on first/simple login. SInce we still want to be able to add repo access later on, we would have two different urls to authenticate via github: one that grants user scope (on the login screen) and one that adds the repo scope (on the 'integrations' tab in our app). We need to check the returning scope map to know whether the user has granted us repo access (and thus store the new key) or if we're just on user access.

[codecov]

Codecov Report

Merging #81 (f809bbe) into main (a851e1e) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main       #81   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines           32        34    +2     
=========================================
+ Hits            32        34    +2     

Impacted Files	Coverage Δ
lib/elixir_auth_github.ex	100.00% <100.00%> (ø)
lib/httpoison_mock.ex	100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[prplmg]

@nelsonic reverted the breaking changes, updated the tests, and fixed the coverage issue, I think it's good to go!

[SimonLab]

Great question! We're developing an app where we can login via github, but not necessarity grant repo access on first/simple login. SInce we still want to be able to add repo access later on, we would have two different urls to authenticate via github: one that grants user scope (on the login screen) and one that adds the repo scope (on the 'integrations' tab in our app). We need to check the returning scope map to know whether the user has granted us repo access (and thus store the new key) or if we're just on user access.

Ok I think I can see the use case, you're adding more scopes at a later stage.
Thanks for the explanation 👍

[nelsonic]

@prplmg thanks for updating. 👌
@SimonLab is our resident GitHub Auth (and Elixir) wizard. 🧙‍♂️
As long as there is no downside to having the scope property I'm in favour. 👍
What would be even better would be if this was somewhere in the Docs/README ... 📝

[nelsonic]

I would actually make this a def instead of defp so that it could be tested independently
to make sure that it degrades gracefully. e.g. if scope is nil does it still work?

Looks like you're handling the case of _scope below but it's not immediately obvious. 💭

[prplmg]

it's not possible for just scope to be nil, as that would mean access_token is also nil and check_authenticated/1 would fail, with set_scope/2 forwarding the error.

[nelsonic]

Ok. Let's ship this "undocumented feature" so @prplmg can use it.
Ideally some docs would be added later. 🙏

[nelsonic]

Package published to https://hex.pm/packages/elixir_auth_github/1.6.4 📦 🚀

[prplmg]

Ok. Let's ship this "undocumented feature" so @prplmg can use it. Ideally some docs would be added later. 🙏

How should I add it to the doc? I added it to the return object example, but I understand if it's a bit vague to not explain where it comes from. However, I could not find a more suitable place to mention it.