Automatically Disable Accounts #1532
Conversation
…ypes for user updates
|
This pull request introduces 1 alert when merging 5a999ce into 48dc0dc - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging fe62052 into 48dc0dc - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging 4457b31 into 48dc0dc - view on LGTM.com new alerts:
|
| .send({ firstName, lastName, disabled: false }) | ||
| .expect(403); | ||
| expect(response.body).toEqual({}); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
We should also add tests in, for example, domains.test.ts, that ensure that if a user with disabled: true accesses the domains list endpoint, it should return a 403.
There was a problem hiding this comment.
@epicfaace IIRC, this test in users.test.ts validates that users with disabled: true cannot authenticate, and 3a8b223 validates that users that have already authenticated but have since been disabled can not get authorization to hit any endpoint.
There was a problem hiding this comment.
This test in users.test.ts doesn't validate that users with disabled: true cannot authenticate. It just validates that users cannot set the disabled property.
There was a problem hiding this comment.
Happy to chat offline if this is unclear!
There was a problem hiding this comment.
Oops, I meant the test 'verify disabled accounts cannot get tokens' in auth.test.ts validates that users with disabled: true cannot authenticate, not this one in users.test.ts.
There was a problem hiding this comment.
Yes, that's correct, though in this thread I was originally asking to see if we could add a test in domains.test.ts, that ensure that if a user with disabled: true accesses the domains list endpoint, it should return a 403. Just an end-to-end test making sure this is true -- could you please add it?
As you mentioned, the test 'verify disabled accounts cannot get tokens' in auth.test.ts validates that users with disabled: true cannot call the callback endpoint, but 3a8b223 only checks API keys. So I think adding the above test would be helpful to ensure we cover everything.
There was a problem hiding this comment.
Once the above is done this is ready to go
|
This pull request introduces 1 alert when merging 2b87d71 into 48dc0dc - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging fd25107 into 48dc0dc - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging 3a8b223 into 48dc0dc - view on LGTM.com new alerts:
|
|
@bgtripp Can you also re-merge the master branch with this branch so we can run CI again? Thanks! |
| expect(user.disabled).toEqual(true); | ||
| }); | ||
| it('update by globalAdmin that enables user should work', async () => { | ||
| const response = await request(app) |
There was a problem hiding this comment.
I think you need to add, at the beginning of this test,
const response = await request(app)
.put(`/users/${user.id}`)
.set(
'Authorization',
createUserToken({
userType: UserType.GLOBAL_ADMIN
})
)
.send({ firstName, lastName, disabled: true })
.expect(200);
so that the user is disabled in the first place.
| .send({ firstName, lastName, disabled: false }) | ||
| .expect(403); | ||
| expect(response.body).toEqual({}); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
This test in users.test.ts doesn't validate that users with disabled: true cannot authenticate. It just validates that users cannot set the disabled property.
|
@mbachtell-nais if you could also take a look at this PR, that would be great! This PR is by @bgtripp and involves automatically disabling accounts after a certain period of inactivity. |
|
OBE |
🗣 Description
Adds the ability to disable/re-enable user accounts for global admins, and a lambda function that automatically disables accounts that have been inactive for over 60 days. Disabled accounts are able to initially authenticate via Login.gov, but will not receive an authorization token from the callback endpoint.
💭 Motivation and context
See issue #958