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

Invitation Pending changes #3229

Merged
merged 15 commits into from
Jan 8, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions client/app/assets/less/redash/redash-newstyle.less
Original file line number Diff line number Diff line change
Expand Up @@ -706,6 +706,11 @@ page-header, .page-header--new {
height: 20px;
}

.user_list__user--invitation-pending {
color: fade(@alert-danger-bg, 75%);
font-weight: 500;
}

.btn__new {
margin-left: 15px;
}
Expand Down
6 changes: 5 additions & 1 deletion client/app/pages/users/list.html
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,9 @@
<img ng-src="{{ user.profile_image_url }}" height="32px" class="profile__image--settings" />
<a href="users/{{ user.id }}" ng-class="{'text-muted': user.is_disabled}">{{ user.name }}</a>
<span class="text-muted"> ({{user.email}})</span>
<span class="user_list__user--invitation-pending" ng-if="user.is_invitation_pending">
(Invitation Pending)
</span>
</td>
<td>
<a ng-repeat="group in user.groups" class="label label-tag" href="groups/{{group.id}}">{{group.name}}</a>
Expand All @@ -72,7 +75,8 @@
<td>
<div ng-if="$ctrl.currentUser.hasPermission('admin') && (user.id != $ctrl.currentUser.id)">
<button type="button" class="btn btn-primary" ng-if="user.is_disabled" ng-click="$ctrl.enableUser(user)">Enable</button>
<button type="button" class="btn btn-default" ng-if="!user.is_disabled" ng-click="$ctrl.disableUser(user)">Disable</button>
<button type="button" class="btn btn-default" ng-if="!(user.is_invitation_pending || user.is_disabled)" ng-click="$ctrl.disableUser(user)">Disable</button>
<button type="button" class="btn btn-default" ng-if="user.is_invitation_pending" ng-click="$ctrl.deleteUser(user)">Delete</button>
</div>
</td>
</tr>
Expand Down
1 change: 1 addition & 0 deletions client/app/pages/users/list.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ class UsersListCtrl extends ListCtrl {
this.policy = Policy;
this.enableUser = user => User.enableUser(user).then(this.update);
this.disableUser = user => User.disableUser(user).then(this.update);
this.deleteUser = user => User.deleteUser(user).then(this.update);
}

getRequest(requestedPage, itemsPerPage, orderByField) {
Expand Down
3 changes: 2 additions & 1 deletion client/app/pages/users/show.html
Original file line number Diff line number Diff line change
Expand Up @@ -79,9 +79,10 @@ <h3 class="profile__h3">{{user.name}}</h3>
<div ng-if="currentUser.isAdmin">
<br>
<div class="form-group">
<button class="btn btn-default" ng-click="sendPasswordReset()" ng-disabled="disablePasswordResetButton">Send
<button class="btn btn-default" ng-if="!user.is_invitation_pending" ng-click="sendPasswordReset()" ng-disabled="disablePasswordResetButton">Send
Password Reset Email
</button>
<button class="btn btn-default" ng-if="user.is_invitation_pending" ng-click="resendInvitation()" ng-disabled="disablePasswordResetButton">Resend Invitation</button>
</div>

<div ng-if="passwordResetLink" class="alert alert-success">
Expand Down
10 changes: 10 additions & 0 deletions client/app/pages/users/show.js
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,16 @@ function UserCtrl(
});
};

$scope.resendInvitation = () => {
$http.post(`api/users/${$scope.user.id}/invite`).success((data) => {
const inviteLink = absoluteUrl(data.invite_link);
toastr.success(`You can use the following link to invite them yourself: <textarea class="form-control m-t-10" rows="3" readonly>${inviteLink}</textarea>`, 'Invitation sent.', {
allowHtml: true,
timeOut: 10000,
});
});
};

$scope.enableUser = (user) => {
User.enableUser(user);
};
Expand Down
19 changes: 19 additions & 0 deletions client/app/services/user.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,24 @@ function disableUser(user, toastr, $sanitize) {
});
}

function deleteUser(user, toastr, $sanitize) {
const userName = $sanitize(user.name);
return $http
.delete(`api/users/${user.id}`)
.then((data) => {
toastr.warning(`User <b>${userName}</b> has been deleted.`, { allowHtml: true });
return data;
})
.catch((response) => {
const message =
response.data && response.data.message
? response.data.message
: `Cannot delete user <b>${userName}</b><br>${response.statusText}`;

toastr.error(message, { allowHtml: true });
});
}

function User($resource, $sanitize, toastr) {
const actions = {
get: { method: 'GET' },
Expand All @@ -59,6 +77,7 @@ function User($resource, $sanitize, toastr) {

UserResource.enableUser = user => enableUser(user, toastr, $sanitize);
UserResource.disableUser = user => disableUser(user, toastr, $sanitize);
UserResource.deleteUser = user => deleteUser(user, toastr, $sanitize);

return UserResource;
}
Expand Down
9 changes: 6 additions & 3 deletions redash/authentication/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ def get_api_key_from_request(request):

def api_key_load_user_from_request(request):
api_key = get_api_key_from_request(request)
if request.view_args is not None:
if request.view_args is not None:
query_id = request.view_args.get('query_id', None)
user = get_user_from_api_key(api_key, query_id)
else:
Expand Down Expand Up @@ -259,14 +259,17 @@ def create_and_login_user(org, name, email, picture=None):
user_object = models.User.get_by_email_and_org(email, org)
if user_object.is_disabled:
return None
if user_object.is_invitation_pending:
user_object.is_invitation_pending = False
models.db.session.commit()
if user_object.name != name:
logger.debug("Updating user name (%r -> %r)", user_object.name, name)
user_object.name = name
models.db.session.commit()
except NoResultFound:
logger.debug("Creating user object (%r)", name)
user_object = models.User(org=org, name=name, email=email, _profile_image_url=picture,
group_ids=[org.default_group.id])
user_object = models.User(org=org, name=name, email=email, is_invitation_pending=False,
_profile_image_url=picture, group_ids=[org.default_group.id])
models.db.session.add(user_object)
models.db.session.commit()

Expand Down
8 changes: 7 additions & 1 deletion redash/handlers/authentication.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,12 @@ def render_token_login_page(template, org_slug, token):
logger.exception("Failed to verify invite token: %s, org=%s", token, org_slug)
return render_template("error.html",
error_message="Your invite link has expired. Please ask for a new one."), 400

if not user.is_invitation_pending:
return render_template("error.html",
error_message=("This invitation has already been accepted. "
"Please try resetting your password instead.")), 400

status_code = 200
if request.method == 'POST':
if 'password' not in request.form:
Expand All @@ -48,7 +54,7 @@ def render_token_login_page(template, org_slug, token):
flash('Password length is too short (<6).')
status_code = 400
else:
# TODO: set active flag
user.is_invitation_pending = False
user.hash_password(request.form['password'])
models.db.session.add(user)
login_user(user)
Expand Down
6 changes: 5 additions & 1 deletion redash/handlers/setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,11 @@ def create_org(org_name, user_name, email, password):
db.session.add_all([default_org, admin_group, default_group])
db.session.commit()

user = User(org=default_org, name=user_name, email=email, group_ids=[admin_group.id, default_group.id])
user = User(org=default_org,
name=user_name,
email=email,
is_invitation_pending=False,
group_ids=[admin_group.id, default_group.id])
user.hash_password(password)

db.session.add(user)
Expand Down
16 changes: 16 additions & 0 deletions redash/handlers/users.py
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,22 @@ def post(self, user_id):

return user.to_dict(with_api_key=is_admin_or_owner(user_id))

@require_admin
def delete(self, user_id):
user = models.User.get_by_id_and_org(user_id, self.current_org)
Copy link
Member

Choose a reason for hiding this comment

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

Worth adding a try/except clause here, for cases when this is called for a non new user (who has items they created). In case of such exception, we should return message saying they can't delete the user, but can disable it.

Copy link
Contributor Author

@rauchy rauchy Jan 1, 2019

Choose a reason for hiding this comment

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

Sounds like this could suffice (without a try/except), no?

if not user.is_invitation_pending:
    abort(403, message="You cannot delete activated users. "
                       "Please disable the user instead.")

Copy link
Member

Choose a reason for hiding this comment

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

Yes.

# admin cannot delete self; current user is an admin (`@require_admin`)
# so just check user id
if user.id == current_user.id:
abort(403, message="You cannot delete your own account. "
"Please ask another admin to do this for you.")
elif not user.is_invitation_pending:
abort(403, message="You cannot delete activated users. "
"Please disable the user instead.")
models.db.session.delete(user)
models.db.session.commit()

return user.to_dict(with_api_key=is_admin_or_owner(user_id))


class UserDisableResource(BaseResource):
@require_admin
Expand Down
4 changes: 3 additions & 1 deletion redash/models/users.py
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ class User(TimestampMixin, db.Model, BelongsToOrgMixin, UserMixin, PermissionsCh
server_default='{}', default={})
active_at = json_cast_property(db.DateTime(True), 'details', 'active_at',
default=None)
is_invitation_pending = json_cast_property(db.Boolean(True), 'details', 'is_invitation_pending', default=True)

__tablename__ = 'users'
__table_args__ = (
Expand Down Expand Up @@ -203,6 +204,7 @@ def to_dict(self, with_api_key=False):
'disabled_at': self.disabled_at,
'is_disabled': self.is_disabled,
'active_at': self.active_at,
'is_invitation_pending': self.is_invitation_pending,
}

if self.password_hash is None:
Expand Down Expand Up @@ -445,4 +447,4 @@ def permissions(self):
return ['view_query']

def has_access(self, obj, access_type):
return False
return False
11 changes: 9 additions & 2 deletions tests/handlers/test_authentication.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,10 @@ def test_valid_token(self):
self.assertEqual(response.status_code, 200)

def test_already_active_user(self):
pass
token = invite_token(self.factory.user)
self.post_request('/invite/{}'.format(token), data={'password': 'test1234'}, org=self.factory.org)
response = self.get_request('/invite/{}'.format(token), org=self.factory.org)
self.assertEqual(response.status_code, 400)


class TestInvitePost(BaseTestCase):
Expand All @@ -47,7 +50,10 @@ def test_bad_token(self):
self.assertEqual(response.status_code, 400)

def test_already_active_user(self):
pass
token = invite_token(self.factory.user)
self.post_request('/invite/{}'.format(token), data={'password': 'test1234'}, org=self.factory.org)
response = self.post_request('/invite/{}'.format(token), data={'password': 'test1234'}, org=self.factory.org)
self.assertEqual(response.status_code, 400)

def test_valid_password(self):
token = invite_token(self.factory.user)
Expand All @@ -56,6 +62,7 @@ def test_valid_password(self):
self.assertEqual(response.status_code, 302)
user = User.query.get(self.factory.user.id)
self.assertTrue(user.verify_password(password))
self.assertFalse(user.is_invitation_pending)


class TestLogin(BaseTestCase):
Expand Down
10 changes: 9 additions & 1 deletion tests/handlers/test_users.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
from tests import BaseTestCase
from mock import patch


class TestUserListResourcePost(BaseTestCase):
def test_returns_403_for_non_admin(self):
rv = self.make_request('post', "/api/users")
Expand Down Expand Up @@ -193,6 +192,15 @@ def test_changing_email_does_not_end_current_session(self):
# make sure the session's `user_id` has changed to reflect the new identity, thus not logging the user out
self.assertNotEquals(previous, current)

def test_admin_can_delete_user(self):
admin_user = self.factory.create_admin()
other_user = self.factory.create_user()

rv = self.make_request('delete', "/api/users/{}".format(other_user.id), user=admin_user)

self.assertEqual(rv.status_code, 200)
self.assertEqual(models.User.query.get(other_user.id), None)


class TestUserDisable(BaseTestCase):
def test_non_admin_cannot_disable_user(self):
Expand Down