Skip to content

Commit

Permalink
[auth] Require tos acceptance on UI login (#14798)
Browse files Browse the repository at this point in the history
## Change Description

Changes login flow in the UI to go via the user page, which gives us a
chance to show (and require acceptance of) the terms of service:

<img width="940" alt="image"
src="https://github.com/user-attachments/assets/b656a577-4432-47fe-acd5-971a2373207f"
/>
Notes to reviewers:
- It should not be possible to log in via the UI except via the `/user`
page (ignoring direct url-surfing to the `/login` url)
- The next_url mechanism should continue to work as before (ie navigate
to `/batches` > prompted to login > login success > should end up on the
batches page)

## Security Assessment

Delete all except the correct answer:
- This change has a medium security impact

### Impact Description

Moves the login button and updates some redirects, but does not
fundamentally alter anything about the login flow.

(Reviewers: please confirm the security impact before approving)
  • Loading branch information
cjllanwarne authored Jan 29, 2025
1 parent b8bea26 commit 5539028
Show file tree
Hide file tree
Showing 5 changed files with 46 additions and 24 deletions.
8 changes: 5 additions & 3 deletions auth/auth/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -454,9 +454,11 @@ async def create_user(request: web.Request, _) -> web.Response:

@routes.get('/user')
@web_security_headers
@auth.authenticated_users_only()
async def user_page(request: web.Request, userdata: UserData) -> web.Response:
return await render_template('auth', request, userdata, 'user.html', {'cloud': CLOUD})
@auth.maybe_authenticated_user
async def user_page(request: web.Request, userdata: Optional[UserData]) -> web.Response:
context_dict = {'cloud': CLOUD, **({'next_page': request.query['next']} if 'next' in request.query else {})}

return await render_template('auth', request, userdata, 'user.html', context_dict)


async def create_copy_paste_token(db, session_id, max_age_secs=300):
Expand Down
36 changes: 35 additions & 1 deletion auth/auth/templates/user.html
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
{% block title %}User{% endblock %}
{% block content %}
<div id="profile" class="vcentered space-y-2">
{% if userdata %}
<h1 class='text-4xl mb-4'>{{ userdata['username'] }}</h1>
<form action="{{ auth_base_url }}/logout" method="POST">
<input type="hidden" name="_csrf" value="{{ csrf_token }}" />
Expand All @@ -20,11 +21,44 @@ <h1 class='text-4xl mb-4'>{{ userdata['username'] }}</h1>
<input type="hidden" name="_csrf" value="{{ csrf_token }}" />
{{ submit_button('Get a copy-paste login token') }}
</form>
{% else %}
<h1 class='text-4xl mb-4'>Log in to continue</h1>
{% if next_page %}
<p>You must sign up or log in to continue to {{ next_page }}</p>
{% else %}
<p>You must sign up or log in to continue</p>
{% endif %}
<table class="table-auto w-64">
<tr>
<td>
<form action="{{ auth_base_url }}/signup" method="GET">
<input type="hidden" name="_csrf" value="{{ csrf_token }}" />
{% if next_page %}
<input type="hidden" name="next" value="{{ next_page }}" />
{% endif %}
{{ submit_button('Sign up') }}
</form>
</td>
<td>
<form action="{{ auth_base_url }}/login" method="GET">
<input type="hidden" name="_csrf" value="{{ csrf_token }}" />
{% if next_page %}
<input type="hidden" name="next" value="{{ next_page }}" />
{% endif %}
{{ submit_button('Log in') }}
</form>
</td>
</tr>
</table>
{% endif %}
<p>
<b>Notice:</b> The Hail system records your email address and IP address. Your email address
The Hail system records your email address and IP address. Your email address
is recorded so that we can authenticate you. Your IP address is tracked as part of our
surveillance of all traffic to and from the Hail system. This broad surveillance enables the
protection of the Hail system from malicious actors.
</p>
<p>
<b>Notice:</b> By signing up or logging in and continuing to use the system, you agree to these terms of service.
</p>
</div>
{% endblock %}
2 changes: 1 addition & 1 deletion gear/gear/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ async def get_session_id(request: web.Request) -> Optional[str]:


def login_redirect(request) -> web.HTTPFound:
login_url = deploy_config.external_url('auth', '/login')
login_url = deploy_config.external_url('auth', '/user')

# request.url is a yarl.URL
request_url = request.url
Expand Down
14 changes: 2 additions & 12 deletions web_common/web_common/templates/header.html
Original file line number Diff line number Diff line change
Expand Up @@ -74,19 +74,9 @@
<a href="{{ auth_base_url }}/user" class="header-link">
<b>{{ userdata["username"] }}</b>
</a>
|
<form action="{{ auth_base_url }}/logout" method="POST">
<input type="hidden" name="_csrf" value="{{ csrf_token }}"/>
<button class="button-header-link" type="submit">
Log out
</button>
</form>
{% else %}
<a class="header-link" href="{{ auth_base_url }}/signup">
Sign Up
</a>
<a class="header-link" href="{{ auth_base_url }}/login">
Login
<a class="header-link" href="{{ auth_base_url }}/user">
Log in or sign up
</a>
{% endif %}
</div>
10 changes: 3 additions & 7 deletions web_common/web_common/templates/new_header.html
Original file line number Diff line number Diff line change
Expand Up @@ -62,13 +62,9 @@
<b class='font-semibold'>{{ userdata["username"] }}</b>
</a>
{% else %}
<a href="{{ auth_base_url }}/signup">
Sign Up
</a>
<span>|</span>
<a href="{{ auth_base_url }}/login">
Login
</a>
<a class="header-link" href="{{ auth_base_url }}/user">
Log in or sign up
</a>
{% endif %}
</div>
</div>

0 comments on commit 5539028

Please sign in to comment.