Closed SimonFlapse closed 4 years ago
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.
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.
- Maybe we should only create the default account when there are zero users?
- Delete the default account if there are users that aren't the default?
- 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.
- 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.
- 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.
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);
https://github.com/Refactorio/FactorioWebInterface/pull/67/commits/4918e6224f462accd67dbce1472db4a53b98903f implements points: 1. 2. 3. (4.)
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.
I've added a couple of tests for the NoAccount, ValidateDefaultUserAsync and DeleteDefaultAdminFile
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:
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.
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