Commit f44794d2 authored by lain's avatar lain

Merge branch '1427-oauth-graceful-admin-scope' into 'develop'

[#1427] Graceful clearance of OAuth admin scopes for non-admin users

Closes #1427

See merge request !2061
parents 79532a7f 81b05340
Pipeline #21060 passed with stages
in 46 minutes and 6 seconds
......@@ -79,7 +79,9 @@ defp authorize_admin_scopes(scopes, app_scopes, %User{} = user) do
if user.is_admin || !contains_admin_scopes?(scopes) || !contains_admin_scopes?(app_scopes) do
{:ok, scopes}
else
{:error, :unsupported_scopes}
# Gracefully dropping admin scopes from requested scopes if user isn't an admin (not raising)
scopes = scopes -- OAuthScopesPlug.filter_descendants(scopes, ["admin"])
validate(scopes, app_scopes, user)
end
end
......
......@@ -567,33 +567,41 @@ test "with existing authentication and OOB `redirect_uri`, redirects to app with
end
describe "POST /oauth/authorize" do
test "redirects with oauth authorization" do
user = insert(:user)
app = insert(:oauth_app, scopes: ["read", "write", "follow"])
test "redirects with oauth authorization, " <>
"keeping only non-admin scopes for non-admin user" do
app = insert(:oauth_app, scopes: ["read", "write", "admin"])
redirect_uri = OAuthController.default_redirect_uri(app)
conn =
build_conn()
|> post("/oauth/authorize", %{
"authorization" => %{
"name" => user.nickname,
"password" => "test",
"client_id" => app.client_id,
"redirect_uri" => redirect_uri,
"scope" => "read:subscope write",
"state" => "statepassed"
}
})
non_admin = insert(:user, is_admin: false)
admin = insert(:user, is_admin: true)
target = redirected_to(conn)
assert target =~ redirect_uri
for {user, expected_scopes} <- %{
non_admin => ["read:subscope", "write"],
admin => ["read:subscope", "write", "admin"]
} do
conn =
build_conn()
|> post("/oauth/authorize", %{
"authorization" => %{
"name" => user.nickname,
"password" => "test",
"client_id" => app.client_id,
"redirect_uri" => redirect_uri,
"scope" => "read:subscope write admin",
"state" => "statepassed"
}
})
query = URI.parse(target).query |> URI.query_decoder() |> Map.new()
target = redirected_to(conn)
assert target =~ redirect_uri
assert %{"state" => "statepassed", "code" => code} = query
auth = Repo.get_by(Authorization, token: code)
assert auth
assert auth.scopes == ["read:subscope", "write"]
query = URI.parse(target).query |> URI.query_decoder() |> Map.new()
assert %{"state" => "statepassed", "code" => code} = query
auth = Repo.get_by(Authorization, token: code)
assert auth
assert auth.scopes == expected_scopes
end
end
test "returns 401 for wrong credentials", %{conn: conn} do
......@@ -623,31 +631,34 @@ test "returns 401 for wrong credentials", %{conn: conn} do
assert result =~ "Invalid Username/Password"
end
test "returns 401 for missing scopes", %{conn: conn} do
user = insert(:user)
app = insert(:oauth_app)
test "returns 401 for missing scopes " <>
"(including all admin-only scopes for non-admin user)" do
user = insert(:user, is_admin: false)
app = insert(:oauth_app, scopes: ["read", "write", "admin"])
redirect_uri = OAuthController.default_redirect_uri(app)
result =
conn
|> post("/oauth/authorize", %{
"authorization" => %{
"name" => user.nickname,
"password" => "test",
"client_id" => app.client_id,
"redirect_uri" => redirect_uri,
"state" => "statepassed",
"scope" => ""
}
})
|> html_response(:unauthorized)
for scope_param <- ["", "admin:read admin:write"] do
result =
build_conn()
|> post("/oauth/authorize", %{
"authorization" => %{
"name" => user.nickname,
"password" => "test",
"client_id" => app.client_id,
"redirect_uri" => redirect_uri,
"state" => "statepassed",
"scope" => scope_param
}
})
|> html_response(:unauthorized)
# Keep the details
assert result =~ app.client_id
assert result =~ redirect_uri
# Keep the details
assert result =~ app.client_id
assert result =~ redirect_uri
# Error message
assert result =~ "This action is outside the authorized scopes"
# Error message
assert result =~ "This action is outside the authorized scopes"
end
end
test "returns 401 for scopes beyond app scopes hierarchy", %{conn: conn} do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment