Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(idp-migration): redirection for account confirmation link #29003

Merged
merged 44 commits into from
Oct 6, 2021
Merged
Show file tree
Hide file tree
Changes from 42 commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
e7221f1
feat(auth): Reuse existing user profile for unknown identity
RyanSkonnord Sep 14, 2021
8ff1e04
feat(idp-migrations): one-time account validation keys
maxiuyuan Sep 20, 2021
bb2c073
added test for creating verification key
maxiuyuan Sep 21, 2021
47e3ab0
uncomment raise NotImplementedError and set feature flag to false
maxiuyuan Sep 21, 2021
a604829
typing to send_confirm_email
maxiuyuan Sep 21, 2021
906d610
removed some comments and white space
maxiuyuan Sep 21, 2021
403d2e9
changed verification from UUID4 to default_validation_hash and remove…
maxiuyuan Sep 21, 2021
eecb007
feat(idp-migration): Verify user and store key in django session
maxiuyuan Sep 22, 2021
d17f024
changes based on pr reviews
maxiuyuan Sep 22, 2021
6edfc34
Merge branch 'idp_verification' into idp_endpoint
maxiuyuan Sep 22, 2021
b463c96
tests for the routes
maxiuyuan Sep 22, 2021
344f700
moved ttl to be a const
maxiuyuan Sep 22, 2021
f0f31fb
Merge branch 'idp_verification' into idp_endpoint
maxiuyuan Sep 22, 2021
5e84ba5
enabled clickable email link to verify
maxiuyuan Sep 22, 2021
4de1d50
removed submitting key on form option, huge refactor to the routes fu…
maxiuyuan Sep 23, 2021
1d25e23
Merge branch 'master' into idp_verification
maxiuyuan Sep 23, 2021
ef147ab
Merge branch 'idp_verification' into idp_endpoint
maxiuyuan Sep 23, 2021
84493af
removed comment
maxiuyuan Sep 23, 2021
5bd84ae
made changes based on reviews
maxiuyuan Sep 24, 2021
a034b68
changed create_verification_key -> send_one_time_account_confirm_link…
maxiuyuan Sep 27, 2021
25a8f33
renamed verify_new_identity -> verify_account
maxiuyuan Sep 27, 2021
bb6bff2
Merge branch 'idp_verification' into idp_endpoint
maxiuyuan Sep 27, 2021
e0c7fc5
fixed tests
maxiuyuan Sep 27, 2021
1b9dd72
Merge branch 'idp_verification' into idp_endpoint
maxiuyuan Sep 27, 2021
17d9104
fixed tests
maxiuyuan Sep 27, 2021
816a98c
Merge branch 'master' into idp_endpoint
maxiuyuan Sep 28, 2021
3cb038f
changes based on pr reviews
maxiuyuan Sep 28, 2021
8d3ec8a
mocking wrong key and changed session key name
maxiuyuan Sep 28, 2021
d87b53a
reverted changing email text
maxiuyuan Sep 28, 2021
81c40f5
removed IDPEmailVerificationView
maxiuyuan Sep 28, 2021
0b8caca
added redirect for email verification
maxiuyuan Sep 29, 2021
56c724d
changed test cases
maxiuyuan Sep 30, 2021
2060f5c
Merge branch 'master' into idp_redirect
maxiuyuan Sep 30, 2021
665bdb7
changed wording on redirection page
maxiuyuan Oct 1, 2021
e1d84c6
made changes based on pr reviews
maxiuyuan Oct 1, 2021
ddbc385
fixed test
maxiuyuan Oct 1, 2021
faa87c3
Merge branch 'master' into idp_redirect
maxiuyuan Oct 1, 2021
fe142eb
style(lint): Auto commit lint changes
getsantry[bot] Oct 1, 2021
3e12cde
code refactor
maxiuyuan Oct 1, 2021
c13530b
code refactor
maxiuyuan Oct 1, 2021
c486177
fixed csrf error and removed redirect in form
maxiuyuan Oct 6, 2021
a16b396
Merge branch 'master' into idp_redirect
maxiuyuan Oct 6, 2021
1b4d2b9
changed name from IDPView -> AccountConfirmationView
maxiuyuan Oct 6, 2021
d87ace9
added comment for auth
maxiuyuan Oct 6, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/sentry/auth/idpmigration.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

_REDIS_KEY = "verificationKeyStorage"
_TTL = timedelta(minutes=10)
SSO_VERIFICATION_KEY = "confirm_account_verification_key"


def send_one_time_account_confirm_link(
Expand Down
12 changes: 12 additions & 0 deletions src/sentry/templates/sentry/idp_account_not_verified.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{% extends "sentry/emails/base.html" %}

{% load i18n %}

{% block main %}
<h3>Sorry,</h3>
<p>
Something went wrong and your email could not be confirmed. Click the button below to go back to the login page.
</p>

<a href="{{ url }}" class="btn">Go back</a>
{% endblock %}
15 changes: 15 additions & 0 deletions src/sentry/templates/sentry/idp_account_verified.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
{% extends "sentry/emails/base.html" %}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we have extra time at the end of this project, we should see if it would be an easy swap to react views for these

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should i make tickets for these and work on it if i have time at the end/during spare cycles?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good.


{% load i18n %}

{% block main %}
<h3>Thank you!</h3>
<p>
Your email has been confirmed. Click the button below to log in to your Sentry account.
</p>

<form class="form-stacked" action="/auth/login/{{org}}/" method="post">
{% csrf_token %}
<button class="btn btn-primary" type="submit">{% trans "Continue" %}</button>
</form>
{% endblock %}
33 changes: 19 additions & 14 deletions src/sentry/web/frontend/idp_email_verification.py
Original file line number Diff line number Diff line change
@@ -1,18 +1,23 @@
from django.http.response import HttpResponseNotFound, HttpResponseRedirect
from django.http.response import HttpResponse
from rest_framework.request import Request
from rest_framework.response import Response

from sentry.auth.idpmigration import get_redis_key, verify_account
from sentry.utils.auth import get_login_url
from sentry.auth.idpmigration import SSO_VERIFICATION_KEY, verify_account
from sentry.web.frontend.base import BaseView
from sentry.web.helpers import render_to_response


def idp_confirm_email(request: Request, key: str) -> Response:
verification_key = get_redis_key(key)
if verify_account(key):
request.session["confirm_account_verification_key"] = verification_key
# TODO Change so it redirects to a confirmation page that needs to be made: Simple page with a confirmation msg and redirect to login
login = get_login_url()
redirect = HttpResponseRedirect(login)
return redirect
# TODO add view that shows key not found
return HttpResponseNotFound()
class IDPView(BaseView):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
class IDPView(BaseView):
class AccountConfirmationView(BaseView):

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would prefer to have things named account_verification around this feature, as email_verification is a seperate feature we don't want to conflate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good, i've changed the name of this class

auth_required = False
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would add a quick comment explaining that the user using this endpoint is currently locked out of their account, so auth isn't required on this endpoint.


def handle(self, request: Request, key: str) -> HttpResponse:
# TODO get org from idpmigration later
context = {"org": "sentry"}

if verify_account(key):
request.session[SSO_VERIFICATION_KEY] = key
return render_to_response(
"sentry/idp_account_verified.html", context=context, request=request
)
return render_to_response(
"sentry/idp_account_not_verified.html", context=context, request=request
)
4 changes: 2 additions & 2 deletions src/sentry/web/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
from sentry.web.frontend.group_plugin_action import GroupPluginActionView
from sentry.web.frontend.group_tag_export import GroupTagExportView
from sentry.web.frontend.home import HomeView
from sentry.web.frontend.idp_email_verification import idp_confirm_email
from sentry.web.frontend.idp_email_verification import IDPView
from sentry.web.frontend.js_sdk_loader import JavaScriptSdkLoader
from sentry.web.frontend.mailgun_inbound_webhook import MailgunInboundWebhookView
from sentry.web.frontend.oauth_authorize import OAuthAuthorizeView
Expand Down Expand Up @@ -214,7 +214,7 @@
),
url(
r"^user-confirm/(?P<key>[^\/]+)/$",
idp_confirm_email,
IDPView.as_view(),
name="sentry-idp-email-verification",
),
url(r"^recover/$", accounts.recover, name="sentry-account-recover"),
Expand Down
13 changes: 6 additions & 7 deletions tests/sentry/auth/test_idpmigration.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,10 @@ def test_verify_account(self):
args=[link.verification_code],
)
response = self.client.get(path)
assert (
self.client.session["confirm_account_verification_key"]
== f"auth:one-time-key:{link.verification_code}"
)
assert response.status_code == 302
assert response.url == "/auth/login/"

assert self.client.session[idpmigration.SSO_VERIFICATION_KEY] == link.verification_code
assert response.status_code == 200
assert response.templates[0].name == "sentry/idp_account_verified.html"

def test_verify_account_wrong_key(self):
idpmigration.send_one_time_account_confirm_link(
Expand All @@ -48,4 +46,5 @@ def test_verify_account_wrong_key(self):
args=["d14Ja9N2eQfPfVzcydS6vzcxWecZJG2z2"],
)
response = self.client.get(path)
assert response.status_code == 404
assert response.status_code == 200
assert response.templates[0].name == "sentry/idp_account_not_verified.html"