-
Notifications
You must be signed in to change notification settings - Fork 212
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(fxa-admin-panel) allow admin to enable login for account from ad… #12709
Conversation
7ec60b8
to
0984e89
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. Add some simple tests for enableAccount, and it's good to go.
@@ -0,0 +1,5 @@ | |||
-- -- Delete new row from table | |||
-- DELETE FROM securityEventNames WHERE name='account.enable` OR name='account.disable; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing ' after account.disable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WHERE name='account.enable`
looks like it might have the wrong closing quote too (backtick vs single quote).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catches, thank you!
@Mutation((returns) => Boolean) | ||
public async enableAccount( | ||
@Args('uid') uid: string, | ||
@CurrentUser() user: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
user doesn't seem to referenced. Maybe remove this arg?
@@ -154,17 +155,31 @@ export class AccountResolver { | |||
return !!result; | |||
} | |||
|
|||
@Features(AdminPanelFeature.EnableAccount) | |||
@Mutation((returns) => Boolean) | |||
public async enableAccount( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get this under test? I noticed that disableAccount also isn't under test. If possible adding cursory tests for both would good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely, great idea
0984e89
to
6c53639
Compare
…min panel fix(fxa-admin-panel): Add new feature to guards feat(fxa-admin-panel) add new SecurityEventNames to db to log enable/disable times feat(fxa-admin-panel): Add new gql mutation and securityeventnames type fix(fxa-admin-panel): Fix securityEventName order and react error fix(fxa-admin-server) fix typos/unused args chore(fxa-admin-server) add tests fix(db-migrations) bump patch number to avoid merge conflict
6c53639
to
52d49c0
Compare
LGTM! When testing manually, I noticed one weird UI defect where after clicking disable, none of the buttons work. But I have a feeling this has been there all along, and might be true for any action on the page. I will file a bug ticket for this separately. |
…min panel
fix(fxa-admin-panel): Add new feature to guards
feat(fxa-admin-panel) add new SecurityEventNames to db to log enable/disable times
feat(fxa-admin-panel): Add new gql mutation and securityeventnames type
Because
Disable
login for an account, but cannot reverse this action and re-enable the account. The ability to re-enable the account was requested in this ticket.This pull request
disabledAt
column. When a user clicks theDisable
button, the user sees a confirmation window warning them that the action is irreversible. If the user confirms that they want to disable the account,disabledAt
is set to the current time.Enable
button, which when clicked, will prompt the user with a confirmation window. If the user confirms that they want to enable the account, the accountdisabledAt
value is set back tonull
.Enable
button is only visible when an account is disabled.Disable
action is now reversible, I changed the messaging in theDisable
confirmation window to reflect this.disabledAt
tonull
erases some of the history of the account, I opted to storeaccount.disable
actions andaccount.enable
actions in thesecurityEvents
table. To do so, I first created a db patch adding those two new names to thesecurityEventNames
table, updated the necessary graphql types, updated therecordAdminSecurityEvent
mutation to accept different event type names, and then calledrecordAdminSecurityEvent
with both the disable handler and the enable handler.Issue that this pull request solves
Closes: #12568
Checklist
Put an
x
in the boxes that applyScreenshots (Optional)
Previously, once an account was disabled, you would see the time at which it was disabled, but you could not re-enable it.
Now, when an account is disabled, you can see the time at which it was disabled both under the "Disable Login" section of the "Danger Zone," and also in the "Account History". Also, you can see a button for re-enabling the account.
Other information (Optional)
To test this PR:
yarn email-bounce
from the root of thefxa/packages/fxa-admin-server
directory. This will create a random email account with a bounce. One way to get the email address for this account (which you will need) is to open a separate tab in your terminal at the root of thefxa
directory and runyarn mysql
. Enteruse fxa;
to select your database. Then, enterselect email from accounts;
. You should see the email address of the account that you are logged in as, and the randomized email address created by the above command. Copy that email address.localhost:8091
Disable
. Click it.OK
when shown the confirmation window.OK
.Disable Login
. You should also see a new section of the "Danger Zone" labelledEnable Login
, with a new button markedEnable
. Finally, if you look above in the "Account History" section, you should see an event "account.disable" with the same timestamp as is displayed in the "Danger Zone" under "Disable Login"Enable
button. When prompted, clickOK
, both for the confirmation window and the alert window that states that you have successfully re-enabled login.