Skip to content

Commit 5fc51ab

Browse files
aleDszaleDsz
authored
Only browsers sends a request to Teams (#3122)
1 parent cd05516 commit 5fc51ab

File tree

3 files changed

+102
-49
lines changed

3 files changed

+102
-49
lines changed

‎lib/livebook/zta/livebook_teams.ex‎

Lines changed: 49 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,26 @@ defmodule Livebook.ZTA.LivebookTeams do
7474
|> halt(), nil}
7575
end
7676

77+
defp handle_request(conn, team, %{"teams_redirect" => _, "redirect_to" => redirect_to}) do
78+
case Teams.Requests.create_auth_request(team) do
79+
{:ok, %{"authorize_uri" => authorize_uri}} ->
80+
uri =
81+
authorize_uri
82+
|> URI.new!()
83+
|> URI.append_query("redirect_to=#{redirect_to}")
84+
85+
{conn
86+
|> redirect(external: URI.to_string(uri))
87+
|> halt(), nil}
88+
89+
{_error_or_transport_error, _reason} ->
90+
{conn
91+
|> put_session(:teams_error, true)
92+
|> redirect(to: conn.request_path)
93+
|> halt(), nil}
94+
end
95+
end
96+
7797
defp handle_request(conn, team, _params) do
7898
case get_session(conn) do
7999
%{"livebook_teams_access_token" => access_token} ->
@@ -100,14 +120,14 @@ defmodule Livebook.ZTA.LivebookTeams do
100120
|> halt(), nil}
101121

102122
_ ->
103-
request_user_authentication(conn, team)
123+
request_user_authentication(conn)
104124
end
105125
end
106126

107127
defp validate_access_token(conn, team, access_token) do
108128
case get_user_info(team, access_token) do
109129
{:ok, metadata} -> {conn, metadata}
110-
_ -> request_user_authentication(conn, team)
130+
_ -> request_user_authentication(conn)
111131
end
112132
end
113133

@@ -118,41 +138,33 @@ defmodule Livebook.ZTA.LivebookTeams do
118138
end
119139
end
120140

121-
defp request_user_authentication(conn, team) do
122-
case Teams.Requests.create_auth_request(team) do
123-
{:ok, %{"authorize_uri" => authorize_uri}} ->
124-
# We have the browser do the redirect because the browser
125-
# knows the current page location. Unfortunately, it is quite
126-
# complex to know the actual host on the server, because the
127-
# user may be running inside a proxy. So in order to make the
128-
# feature more accessible, we do the redirecting on the client.
129-
conn =
130-
html(conn, """
131-
<!DOCTYPE html>
132-
<html lang="en">
133-
<head>
134-
<meta charset="UTF-8">
135-
<title>Redirecting...</title>
136-
<script>
137-
const redirectTo = new URL(window.location.href);
138-
redirectTo.searchParams.append("teams_identity", "");
139-
140-
const url = new URL("#{authorize_uri}");
141-
url.searchParams.set("redirect_to", redirectTo.toString());
142-
window.location.href = url.toString();
143-
</script>
144-
</head>
145-
</html>
146-
""")
147-
148-
{halt(conn), nil}
149-
150-
_ ->
151-
{conn
152-
|> put_session(:teams_error, true)
153-
|> redirect(to: conn.request_path)
154-
|> halt(), nil}
155-
end
141+
defp request_user_authentication(conn) do
142+
# We have the browser do the redirect because the browser
143+
# knows the current page location. Unfortunately, it is quite
144+
# complex to know the actual host on the server, because the
145+
# user may be running inside a proxy. So in order to make the
146+
# feature more accessible, we do the redirecting on the client.
147+
html_document = """
148+
<!DOCTYPE html>
149+
<html lang="en">
150+
<head>
151+
<meta charset="UTF-8">
152+
<title>Redirecting...</title>
153+
<script>
154+
const redirectTo = new URL(window.location.href);
155+
redirectTo.searchParams.append("teams_identity", "");
156+
157+
const url = new URL(window.location.href);
158+
url.searchParams.set("redirect_to", redirectTo.toString());
159+
url.searchParams.append("teams_redirect", "");
160+
161+
window.location.href = url.toString();
162+
</script>
163+
</head>
164+
</html>
165+
"""
166+
167+
{conn |> html(html_document) |> halt(), nil}
156168
end
157169

158170
defp get_user_info(team, access_token) do

‎test/livebook_teams/zta/livebook_teams_test.exs‎

Lines changed: 37 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -25,18 +25,36 @@ defmodule Livebook.ZTA.LivebookTeamsTest do
2525
end
2626

2727
test "gets the user information from Livebook Teams", %{conn: conn, node: node, test: test} do
28-
# Step 1: Get redirected to Livebook Teams
28+
# Step 1: Would get redirected to Livebook to check if it's a bot
2929
conn = init_test_session(conn, %{})
3030
{conn, nil} = LivebookTeams.authenticate(test, conn, [])
31+
assert html_response(conn, 200) =~ "teams_redirect"
3132

32-
[_, location] = Regex.run(~r/URL\("(.*?)"\)/, html_response(conn, 200))
33+
redirect_to =
34+
LivebookWeb.Endpoint.url()
35+
|> URI.new!()
36+
|> URI.append_query("teams_identity")
37+
38+
# Step 2: Checks if the given request belongs to a browser
39+
conn =
40+
build_conn(:get, "/", %{teams_redirect: "", redirect_to: URI.to_string(redirect_to)})
41+
|> init_test_session(%{})
42+
|> put_req_header(
43+
"user-agent",
44+
"Mozilla/5.0 (X11; Linux x86_64; rv:121.0) Gecko/20100101 Firefox/121.0"
45+
)
46+
47+
{conn, nil} = LivebookTeams.authenticate(test, conn, [])
48+
49+
# Step 3: Get redirected to Livebook Teams
50+
location = Phoenix.ConnTest.redirected_to(conn)
3351
uri = URI.parse(location)
3452
assert uri.path == "/identity/authorize"
3553
assert %{"token" => token} = URI.decode_query(uri.query)
3654

3755
%{code: code} = TeamsRPC.allow_auth_request(node, token)
3856

39-
# Step 2: Emulate the redirect back with the code for validation
57+
# Step 4: Emulate the redirect back with the code for validation
4058
conn =
4159
build_conn(:get, "/", %{teams_identity: "", code: code})
4260
|> init_test_session(Plug.Conn.get_session(conn))
@@ -46,14 +64,29 @@ defmodule Livebook.ZTA.LivebookTeamsTest do
4664

4765
assert redirected_to(conn, 302) == "/"
4866

49-
# Step 3: Confirm the token is valid for future requests
67+
# Step 5: Confirm the token is valid for future requests
5068
conn =
5169
build_conn(:get, "/")
5270
|> init_test_session(Plug.Conn.get_session(conn))
5371

5472
assert {%{halted: false}, ^metadata} = LivebookTeams.authenticate(test, conn, [])
5573
end
5674

75+
test "blocks non-browsers to reach Livebook Teams", %{test: test} do
76+
redirect_to =
77+
LivebookWeb.Endpoint.url()
78+
|> URI.new!()
79+
|> URI.append_query("teams_identity")
80+
81+
conn =
82+
build_conn(:get, "/", %{teams_redirect: "", redirect_to: URI.to_string(redirect_to)})
83+
|> init_test_session(%{})
84+
|> put_req_header("user-agent", "Mozilla/5.0 (compatible; Thinkbot/0.5.8; +blablabla.)")
85+
86+
{conn, nil} = LivebookTeams.authenticate(test, conn, [])
87+
assert response(conn, 200) == ""
88+
end
89+
5790
test "shows an error when the user does not belong to the org", %{conn: conn, test: test} do
5891
# Step 1: Emulate a request coming from Teams saying the user does belong to the org
5992
conn = init_test_session(conn, %{})

‎test/support/integration/teams_tests.ex‎

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -179,18 +179,26 @@ defmodule Livebook.TeamsIntegrationHelper do
179179
end
180180

181181
def authenticate_user_on_teams(name, node, team) do
182-
conn = Phoenix.ConnTest.build_conn()
182+
conn =
183+
Phoenix.ConnTest.build_conn()
184+
|> Plug.Conn.put_req_header(
185+
"user-agent",
186+
"Mozilla/5.0 (X11; Linux x86_64; rv:121.0) Gecko/20100101 Firefox/121.0"
187+
)
188+
189+
redirect_to =
190+
LivebookWeb.Endpoint.url()
191+
|> URI.new!()
192+
|> URI.append_query("teams_identity")
183193

184-
response =
194+
uri =
185195
conn
186196
|> LivebookWeb.ConnCase.with_authorization(team.id, name)
187-
|> get("/")
188-
|> html_response(200)
197+
|> get("/", %{teams_redirect: "", redirect_to: URI.to_string(redirect_to)})
198+
|> Phoenix.ConnTest.redirected_to()
199+
|> URI.parse()
189200

190-
[_, location] = Regex.run(~r/URL\("(.*?)"\)/, response)
191-
uri = URI.parse(location)
192201
%{"token" => token} = URI.decode_query(uri.query)
193-
194202
%{code: code} = Livebook.TeamsRPC.allow_auth_request(node, token)
195203

196204
session =
@@ -199,7 +207,7 @@ defmodule Livebook.TeamsIntegrationHelper do
199207
|> get("/", %{teams_identity: "", code: code})
200208
|> Plug.Conn.get_session()
201209

202-
authenticated_conn = %Plug.Conn{} = Plug.Test.init_test_session(conn, session)
210+
authenticated_conn = Plug.Test.init_test_session(conn, session)
203211
final_conn = get(authenticated_conn, "/")
204212
assigns = Map.take(final_conn.assigns, [:current_user])
205213

0 commit comments

Comments
 (0)