search

dwyl / elixir-auth-github

:octocat: Minimalist GitHub OAuth Authentication for Elixir Apps. Tested, Documented & Maintained. Setup in 5 mins. 🚀
GNU General Public License v2.0
38 stars 4 forks source link

filter_valid_scopes/1 returns :ok when scopes are invalid! #37

Closed nelsonic closed 4 years ago

nelsonic commented 4 years ago

While reading through the current /test/elixir_auth_github_test.exs file, I noticed that there is a test that includes the (invalid) scope "other": https://github.com/dwyl/elixir-auth-github/blob/44021233add025348f261f5bf0b9b32fe6c5711a/test/elixir_auth_github_test.exs#L69-L73 This "other" gets filtered out by the filter_valid_scopes/1 and ignored ... I don't think this is desirable behaviour because it can filter out a typo scope and the person consuming this package will be none the wiser! If typo scopes that seem valid but aren't are filtered silently then our package could be "swallowing errors" which could make it painful to debug!

The (private) function filter_valid_scopes/1 returns :ok as long as there is one valid scope: https://github.com/dwyl/elixir-auth-github/blob/44021233add025348f261f5bf0b9b32fe6c5711a/lib/elixir_auth_github.ex#L119-L128

While I can see the intent, I don't feel that this function offers us much value given that if I spell one of the scopes in a List of scopes correctly but typo the rest of them (or include completely invalid scopes) it will still return :ok ...

I understand that filter_valid_scopes means "filter for valid scopes" it's actually "filter out invalid scopes" i.e. it ignores when a scope is invalid. this can be quite a headache to debug if someone typos the scope e.g: user:emal instead of user:email or an even more fiendish "read:orgs" (invalid but "LGTM!") instead of "read:org" (valid) This would be super lame to debug for a user of this package and we can avoid wasting anyone's time

I'm going to re-do this ...

nelsonic commented 4 years ago

I think the filter_valid_scopes/1 is more hassle than it's worth. If it can hide the invalid or typo scopes and exclude them from the requests to GitHub, then the onus of this rests on us the developers of this package. Whereas if we do not include any filtering for valid scopes in our package, then the invalid scopes will be included in the URL for the request sent to GitHub and thus the dev consuming this package will find out about their invalid scope immediately!

nelsonic commented 4 years ago

I'm making the "executive" decision to remove all the code related to "valid" scopes and leave it to the developer consuming this package to not make any typos!

nelsonic commented 4 years ago

Removed in https://github.com/dwyl/elixir-auth-github/pull/34 ✂️