DefaultAdminAccountOption added

[SimonFlapse]

Adds a user account to the web interface when the option is enabled with the unsafe password: administrator (This is non-configurable to prevent using this account for production)

I've made this PR to become more familiar with the WebInterface.

Effort towards #47

[grilledham]

Very nice!

Something like this is needed, and I've been thinking about the best way to do this too. I'm not sure what is the best way to do this, so I'm going to write my thoughts and we can discuss it.

1. Maybe we should only create the default account when there are zero users?
2. Delete the default account if there are users that aren't the default?
3. With the above we could have the option be enabled by default (still with the ability to turn it off). I like the idea that less configuration would have to be done to get a first run of the web interface working.
4. If we are concern about the unsafe password we could have the code generate a random one and output to a file or logs. This is probably overkill though.
5. What we need is a way to create/manage accounts independently of Discord. But that is a larger feature that can be left for later.

[SimonFlapse]

Very nice!

Something like this is needed, and I've been thinking about the best way to do this too. I'm not sure what is the best way to do this, so I'm going to write my thoughts and we can discuss it.

1. Maybe we should only create the default account when there are zero users?
2. Delete the default account if there are users that aren't the default?
3. With the above we could have the option be enabled by default (still with the ability to turn it off). I like the idea that less configuration would have to be done to get a first run of the web interface working.
4. If we are concern about the unsafe password we could have the code generate a random one and output to a file or logs. This is probably overkill though.
5. What we need is a way to create/manage accounts independently of Discord. But that is a larger feature that can be left for later.

1. I had the same thought, but wanted to start off with something simple.
2. We could do that but I think we need 5. first, to prevent locking someone out if they don't have discord integration
3. I think this is needed, otherwise you have no way of accessing the web panel without going through discord setup.
4. I just wanted to make it clear that the password should be changed, if the account is enabled.
   I just realised that it would reset the password everytime the webinterface restarts, so changing it wouldn't matter. I might do the random one outputting to log.
5. I think I can extend on the account management page to implement this feature. Question just is, who should be able to create new accounts. Extended account management #68

[grilledham]

2: Fair point. I guess my thinking is on first login they would create another account to user. But that needs 5. Currently you can change your password so they could do that when they log in. We just have to not delete the account each time.

4: We could just tell them to change the password after first log in. Maybe we redirect them to the account page instead of the server page on first log in. Though we would need to keep track of if it's first log in. If we want to go down that route a property could be added to the ApplicationUser class. Then do the entity framework magic to update the database. Run migration + update https://docs.microsoft.com/en-us/ef/core/managing-schemas/migrations/?tabs=dotnet-core-cli.

5: I did put some thought to this when first setting this up. There are two roles: Root and Admin. Only Root users would be able to manage accounts. I guess this means the default admin should be added to the root role. Something like this
result = await userManager.AddToRoleAsync(user, Constants.RootRole);

[SimonFlapse]

4918e62 implements points: 1. 2. 3. (4.)

[grilledham]

This is brilliant work!

I would however like to turn the DefaultAdminAccount class into a service.

If we did that we could then add tests. I would like all new code to have tests when it is reasonable and meaningful to do so. I have test for most of the database stuff so it would seem reasonable to add test for this too. I appreciate that might be difficult to do as I don't have examples of test that use the UserManager. So I suggest that you make the changes to turn this into a service, then I will make the setup code and the first test. You can then use that test as a template for the other tests in this PR.

[SimonFlapse]

This is brilliant work!

I would however like to turn the DefaultAdminAccount class into a service.

If we did that we could then add tests. I would like all new code to have tests when it is reasonable and meaningful to do so. I have test for most of the database stuff so it would seem reasonable to add test for this too. I appreciate that might be difficult to do as I don't have examples of test that use the UserManager. So I suggest that you make the changes to turn this into a service, then I will make the setup code and the first test. You can then use that test as a template for the other tests in this PR.

The DefaultAdminAccount is now a service. Unless something else comes up, I'll wait until you've had the time to setup tests for this PR.

[SimonFlapse]

I've added a couple of tests for the NoAccount, ValidateDefaultUserAsync and DeleteDefaultAdminFile

[grilledham]

I've had some time to review this and I think we should make some changes.

I would like to simplify the DefaultAdminAccountService, I think all we need to do is:

1. If there are 0 users, created a default account with admin+root roles. (I don’t think this needs to be any more complicated than just 0 users, no roles or name checks).
2. Don’t delete the default account. (We will let the users decide if they want to do this when we have the improved account management).
3. In the password file, recommend that people change the default account password when they log in. (Later with the account management we could recommend creating a new account and deleting the default one).
4. Maybe an option in the config to disable automatic creation of default accounts, but this isn’t important.

Thoughts on this?

Also I think the tests aren't quite right. In the DefaultAdminAccountService class don’t make the private methods public so that you can test them. The purpose of the tests should be to show that the code works the way it is used in production.
So all the tests should setup what they need before calling SetupDefaultUserAsync(), then assert that what was expected happened.
For example, I would have a test called NotCreated_WhenAccountsInDatabase. In the Arrange section add an account, in Act, just call SetupDefaultUserAsync() and in Assert, test that there is only the account made during Arrange.

If this isn't clear or you would like any help, feel free to ask.

[grilledham]

These need to be awaited to make sure the roles are present before calling SetupDefaultUserAsync.

The SeedData function can be moved into the the WhenAll call on line 48.