Basic Setup scripts #60
Conversation
|
cool - looks like i have some credo to fix :) |
| @@ -6,7 +6,7 @@ defmodule ExOauth2Provider.Authorization.CodeTest do | |||
| alias Dummy.{OauthAccessGrants.OauthAccessGrant, Repo} | |||
|
|
|||
| @client_id "Jf5rM8hQBc" | |||
| @valid_request %{"client_id" => @client_id, "response_type" => "code", "scope" => "app:read app:write"} | |||
| @valid_request %{"client_id" => @client_id, "response_type" => "code"} | |||
There was a problem hiding this comment.
Can we keep this as it is, and instead just do Map.delete(@valid_request, "scope") in the "with no scope" test? The default setup for most OAuth2 apps is to have scopes, so that's why it's a default in these tests.
There was a problem hiding this comment.
@danschultzer : since scopes are an optional part of OAUTH i wanted to have some tests that did sanity check and make sure it would function.
I see no problem with keeping it. I could rename it to @valid_request_with_scope and make one for no scope.
| @@ -83,6 +83,12 @@ defmodule ExOauth2Provider.Authorization.CodeTest do | |||
| %{resource_owner: resource_owner, application: application} | |||
| end | |||
|
|
|||
| test "with no scope", %{resource_owner: resource_owner, application: application} do | |||
There was a problem hiding this comment.
This is tested with an application that doesn't have any scope, but relies on the server scope. Is this as intended? It may make more sense to put it outside this describe block.
There was a problem hiding this comment.
Yes hmm - I think that is fair. I really appreciate your thoughts
I think in general that part of the trouble OAUTH2 creates for itself is not having strong and clear best options to follow. IE (PKCE + authorization code will cover 95% of use cases , but instead it has password grant (only secure for specific cases) , credential grant, implicit grant (depricated) )
Maybe the right question we need to be asking is : Is a token with no scope legitimate if the application specifies a scope?
| @@ -134,12 +140,20 @@ defmodule ExOauth2Provider.Authorization.CodeTest do | |||
| %{resource_owner: resource_owner, application: application} | |||
| end | |||
|
|
|||
| test "generates grant with no scope passed", %{resource_owner: resource_owner} do | |||
There was a problem hiding this comment.
Same as above, this is a specific test environment, and it may make more sense to put it outside this describe block unless you need to test this in a no scope application environment (remember, the server scopes are still there).
| test "error when invalid server scope", %{resource_owner: resource_owner} do | ||
| request = Map.merge(@valid_request, %{"scope" => "public profile"}) | ||
| assert Authorization.authorize(resource_owner, request, otp_app: :ex_oauth2_provider) == {:error, @invalid_scope, :unprocessable_entity} | ||
| end | ||
|
|
||
| test "generates grant", %{resource_owner: resource_owner} do | ||
| test "generates grant with public scope", %{resource_owner: resource_owner} do |
There was a problem hiding this comment.
I don't call this public scope, since the scope's actual name has no meaning in these tests. I just ensures that the server scope works.
There was a problem hiding this comment.
Ah - I though the intent was that it was a non-sensical scope.
| @@ -160,7 +174,7 @@ defmodule ExOauth2Provider.Authorization.CodeTest do | |||
|
|
|||
| assert access_grant.resource_owner_id == resource_owner.id | |||
| assert access_grant.expires_in == Config.authorization_code_expires_in(otp_app: :ex_oauth2_provider) | |||
| assert access_grant.scopes == @valid_request["scope"] | |||
| assert access_grant.scopes == "" | |||
There was a problem hiding this comment.
This should be tested with scope.
Hi Dan - I'm just getting my environment setup.
I wrote a simple test to confirm that scopes are optional.