Remove re-enter password step from rename organisation

Note that this is copied from the same change made to the rename service
page:
1190e4541b

The original idea behind was to always ask users to re-enter their
password any time:
- we want them to be sure that they want to do what they’re about to do
- we want to be sure it’s really the user trying to do the thing (and
  not someone malicious)

In reality we:
- removed this from the initial place it was added (a descendent of the
  ‘suspend service’ feature)
- only ever added it to the ‘rename service’ and ‘rename organisation’
  features

So in reality it’s not a pattern we have persisted with. Arguably there
are several things you can now do in the admin app without re-entering
your password which are much more high consequence than changing the
service name.

Also, with browser autofill there’s a lot less chance that forcing
someone to re-enter a password really gives much defence against an
unattended laptop, for example.

So this commit removes the need to re-enter your password when renaming
an organisation.
This commit is contained in:
Chris Hill-Scott
2022-01-11 14:24:25 +00:00
parent 45e178036a
commit c630faf3b4
5 changed files with 36 additions and 180 deletions

View File

@@ -2,15 +2,7 @@ from collections import OrderedDict
from datetime import datetime
from functools import partial
from flask import (
flash,
redirect,
render_template,
request,
send_file,
session,
url_for,
)
from flask import flash, redirect, render_template, request, send_file, url_for
from flask_login import current_user
from notifications_python_client.errors import HTTPError
from werkzeug.exceptions import abort
@@ -22,14 +14,12 @@ from app import (
letter_branding_client,
org_invite_api_client,
organisations_client,
user_api_client,
)
from app.main import main
from app.main.forms import (
AddGPOrganisationForm,
AddNHSLocalOrganisationForm,
BillingDetailsForm,
ConfirmPasswordForm,
EditNotesForm,
GoLiveNotesForm,
InviteOrgUserForm,
@@ -330,12 +320,16 @@ def edit_organisation_name(org_id):
if form.name.data == current_organisation.name:
return redirect(url_for('.organisation_settings', org_id=current_organisation.id))
unique_name = organisations_client.is_organisation_name_unique(org_id, form.name.data)
if not unique_name:
form.name.errors.append("This organisation name is already in use")
return render_template('views/organisations/organisation/settings/edit-name/index.html', form=form)
session['organisation_name_change'] = form.name.data
return redirect(url_for('.confirm_edit_organisation_name', org_id=org_id))
try:
current_organisation.update(name=form.name.data)
except HTTPError as http_error:
error_msg = 'Organisation name already exists'
if http_error.status_code == 400 and error_msg in http_error.message:
form.name.errors.append('This organisation name is already in use')
else:
raise http_error
else:
return redirect(url_for('.organisation_settings', org_id=org_id))
return render_template(
'views/organisations/organisation/settings/edit-name/index.html',
@@ -549,38 +543,6 @@ def edit_organisation_domains(org_id):
)
@main.route("/organisations/<uuid:org_id>/settings/edit-name/confirm", methods=['GET', 'POST'])
@user_has_permissions()
def confirm_edit_organisation_name(org_id):
# Validate password for form
def _check_password(pwd):
return user_api_client.verify_password(current_user.id, pwd)
form = ConfirmPasswordForm(_check_password)
if form.validate_on_submit():
try:
organisations_client.update_organisation_name(
current_organisation.id,
name=session['organisation_name_change'],
)
except HTTPError as e:
error_msg = "Organisation name already exists"
if e.status_code == 400 and error_msg in e.message:
# Redirect the user back to the change organisation name screen
flash('This organisation name is already in use', 'error')
return redirect(url_for('main.edit_organisation_name', org_id=org_id))
else:
raise e
else:
session.pop('organisation_name_change')
return redirect(url_for('.organisation_settings', org_id=org_id))
return render_template(
'views/organisations/organisation/settings/edit-name/confirm.html',
new_name=session['organisation_name_change'],
form=form)
@main.route("/organisations/<uuid:org_id>/settings/edit-go-live-notes", methods=['GET', 'POST'])
@user_is_platform_admin
def edit_organisation_go_live_notes(org_id):

View File

@@ -324,7 +324,6 @@ class OrgNavigation(Navigation):
'organisation_dashboard',
},
'settings': {
'confirm_edit_organisation_name',
'edit_organisation_agreement',
'edit_organisation_billing_details',
'edit_organisation_crown_status',

View File

@@ -1,30 +0,0 @@
{% extends "org_template.html" %}
{% from "components/page-header.html" import page_header %}
{% from "components/page-footer.html" import page_footer %}
{% from "components/form.html" import form_wrapper %}
{% from "components/back-link/macro.njk" import govukBackLink %}
{% block org_page_title %}
Confirm organisation name change
{% endblock %}
{% block backLink %}
{{ govukBackLink({ "href": url_for('.organisation_settings', org_id=current_org.id) }) }}
{% endblock %}
{% block maincolumn_content %}
{{ page_header('Confirm organisation name change') }}
<div class="govuk-grid-row">
<div class="govuk-grid-column-three-quarters">
{% call form_wrapper() %}
{{ form.password(param_extensions={"autocomplete": "current-password"}) }}
<p class="govuk-body"> Your organisation name will be changed from {{ current_org.name }} to {{ new_name }} </p>
{{ page_footer('Confirm') }}
{% endcall %}
</div>
</div>
{% endblock %}

View File

@@ -1202,22 +1202,26 @@ def test_update_organisation_domains_when_domain_already_exists(
def test_update_organisation_name(
client_request,
platform_admin_user,
organisation_one,
fake_uuid,
mock_get_organisation,
mock_organisation_name_is_unique
mock_update_organisation,
):
client_request.login(platform_admin_user)
client_request.post(
'.edit_organisation_name',
org_id=organisation_one['id'],
org_id=fake_uuid,
_data={'name': 'TestNewOrgName'},
_expected_redirect=url_for(
'.confirm_edit_organisation_name',
org_id=organisation_one['id'],
'.organisation_settings',
org_id=fake_uuid,
_external=True,
),
)
)
mock_update_organisation.assert_called_once_with(
fake_uuid,
name='TestNewOrgName',
cached_service_ids=None,
)
assert mock_organisation_name_is_unique.called
@pytest.mark.parametrize('name, error_message', [
@@ -1246,73 +1250,29 @@ def test_update_organisation_with_incorrect_input(
def test_update_organisation_with_non_unique_name(
client_request,
platform_admin_user,
organisation_one,
fake_uuid,
mock_get_organisation,
mock_organisation_name_is_not_unique
mocker,
):
mocker.patch(
'app.organisations_client.update_organisation',
side_effect=HTTPError(
response=mocker.Mock(
status_code=400,
json={'result': 'error', 'message': 'Organisation name already exists'}
),
message='Organisation name already exists',
)
)
client_request.login(platform_admin_user)
page = client_request.post(
'.edit_organisation_name',
org_id=organisation_one['id'],
org_id=fake_uuid,
_data={'name': 'TestNewOrgName'},
_expected_status=200,
)
assert 'This organisation name is already in use' in page.select_one('.govuk-error-message').text
assert mock_organisation_name_is_not_unique.called
def test_confirm_update_organisation(
client_request,
platform_admin_user,
organisation_one,
mock_get_organisation,
mock_verify_password,
mock_update_organisation,
mocker
):
with client_request.session_transaction() as session:
session['organisation_name_change'] = 'newName'
client_request.login(platform_admin_user)
client_request.post(
'.confirm_edit_organisation_name',
org_id=organisation_one['id'],
_data={'password': 'validPassword'},
_expected_redirect=url_for(
'.organisation_settings',
org_id=organisation_one['id'],
_external=True,
),
)
mock_update_organisation.assert_called_with(
organisation_one['id'],
name=session['organisation_name_change']
)
def test_confirm_update_organisation_with_incorrect_password(
client_request,
platform_admin_user,
organisation_one,
mock_get_organisation,
mocker
):
with client_request.session_transaction() as session:
session['organisation_name_change'] = 'newName'
mocker.patch('app.user_api_client.verify_password', return_value=False)
client_request.login(platform_admin_user)
page = client_request.post(
'.confirm_edit_organisation_name',
org_id=organisation_one['id'],
_expected_status=200,
)
assert normalize_spaces(
page.select_one('.govuk-error-message').text
) == 'Error: Invalid password'
def test_confirm_update_organisation_with_existing_name(
@@ -1334,40 +1294,6 @@ def test_confirm_update_organisation_with_existing_name(
)
def test_confirm_update_organisation_with_name_already_in_use(
client_request,
platform_admin_user,
organisation_one,
mock_get_organisation,
mock_verify_password,
mocker
):
with client_request.session_transaction() as session:
session['organisation_name_change'] = 'newName'
mocker.patch(
'app.organisations_client.update_organisation_name',
side_effect=HTTPError(
response=mocker.Mock(
status_code=400,
json={'result': 'error', 'message': 'Organisation name already exists'}
),
message="Organisation name already exists"
)
)
client_request.login(platform_admin_user)
client_request.post(
'.confirm_edit_organisation_name',
org_id=organisation_one['id'],
_expected_redirect=url_for(
'main.edit_organisation_name',
org_id=organisation_one['id'],
_external=True,
),
)
def test_get_edit_organisation_go_live_notes_page(
client_request,
platform_admin_user,

View File

@@ -63,7 +63,6 @@ EXCLUDED_ENDPOINTS = tuple(map(Navigation.get_endpoint_with_blueprint, {
'choose_template',
'choose_template_to_copy',
'clear_cache',
'confirm_edit_organisation_name',
'confirm_edit_user_email',
'confirm_edit_user_mobile_number',
'confirm_redact_template',