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

Prevent admin from invalidating their own password #708

Merged
merged 1 commit into from
Jan 24, 2023

Conversation

matthew-white
Copy link
Member

@matthew-white matthew-white commented Jan 22, 2023

See the commit message for notes.


Note for the QA team: You can verify this by looking for "Reset password" in the actions dropdown on the Users page. It should be disabled for the current user.

I don't think an admin would ever want to reset and invalidate their own
password rather than changing their password. If they did invalidate
their password, their sessions would be deleted, including their current
session. That would lead to the issue described in #705.
Comment on lines +49 to 52
<li :class="{ disabled }">
<a class="reset-password" href="#"
v-tooltip.aria-describedby="disabled ? $t('cannotResetPassword') : null"
@click.prevent="$emit('reset-password', user)">
Copy link
Member Author

Choose a reason for hiding this comment

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

This is very similar to how we disable the retire button directly below.

Comment on lines +24 to +30
it('is disabled for the current user', async () => {
const component = await load('/users', { root: false });
const a = component.get('.user-row .reset-password');
a.element.parentNode.classList.contains('disabled').should.be.true();
a.should.have.ariaDescription(/^You may not reset your own password/);
await a.should.have.tooltip();
});
Copy link
Member Author

Choose a reason for hiding this comment

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

This is very similar to the test in test/components/user/retire.spec.js that the retire button is disabled for the current user.

@matthew-white matthew-white self-assigned this Jan 22, 2023
@matthew-white
Copy link
Member Author

@alxndrsn, I've tagged you for review here with the thought that the changes here are very similar to how things are done elsewhere in the codebase. I figure you could probably verify that even without having written a lot of component code in Frontend, but let me know if it'd be better for me to tag someone else.

Copy link
Contributor

@alxndrsn alxndrsn left a comment

Choose a reason for hiding this comment

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

LGTM. Hopefully there is corresponding code in odk-backend to prevent this on the API side as well.

@matthew-white
Copy link
Member Author

Hopefully there is corresponding code in odk-backend to prevent this on the API side as well.

I figure that if an admin wants to use the API to invalidate their own password, YOLO. Their session token won't work for future requests, but that's an outcome they should be prepared for after invalidating their password. The API will invalidate a password during a password reset only if ?invalidate=true is specified, so I think it would be pretty unusual for an admin to do that for their own account. If they really want to, I don't think we necessarily need to stop them. But I wanted to stop them in Frontend because invalidating their own password would lead to the issues described in #705, and that issue is something that we still need to address on our end. I could be convinced to do this in Backend as well, but my instinct is that it's probably not needed.

@matthew-white matthew-white merged commit bc17ac3 into master Jan 24, 2023
@matthew-white matthew-white deleted the disable-reset-password branch January 24, 2023 22:34
@srujner
Copy link

srujner commented Jan 25, 2023

@matthew-white
What about resetting the admin password from the login screen? Should it be possible to reset it from the email token?
Screenshot(56)

@matthew-white
Copy link
Member Author

Good question. It should be possible to reset anyone's password using that process. The difference between requesting a password reset from the page in the screenshot above and requesting it from the Users page is that when a reset is initiated from the Users page, the user's password is immediately invalidated. In contrast, the page above is only available to someone who isn't logged in already. If you aren't logged in, you can request a password reset for anyone, but that won't invalidate anyone's existing password. (Otherwise you could invalidate someone's password just by knowing their email address.)

An admin shouldn't be able to request a reset of their own password from the Users page because that would immediately invalidate their password, which would cause other issues. However, if the admin isn't logged in, they should be able to request a password reset from the page above; it just won't invalidate their password. Does that make sense?

@srujner
Copy link

srujner commented Jan 26, 2023

Yes, It's pretty clear now :)

@dbemke
Copy link

dbemke commented Jan 26, 2023

Tested with success!

@srujner
Copy link

srujner commented Jan 26, 2023

Tested with success!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ done
Development

Successfully merging this pull request may close these issues.

4 participants