[authentication-system] Missing test + Identity probably should be class

[wischi-chr]

I just mentored this exercise and found out that this exercise should probably be improved.

In Task 3 the user is asked to "Remove the set accessor or make it private" and because Identity is a struct and the properties are strings it can no longer be changed (because structs are implicitly copied) but Task 4 now asks the user to prevent tempering with the Admin property (which is not possible because we prevented that in Task 3, because string is immuatble and Identity is a struct) - maybe make Identity a class and link some resources in the instructions about the difference of structs and classes in C#.

In such simple exercises I would even go as far as not to use structs at all(!) unless the exercise is about structs. Don't get me wrong structs are important in C# but such a niche that I would consider them an "advanced concept" because there are so many things that might be confusing for example in structs with mutable reference fields, structs that are larger than 16 bytes, etc.

Task 5 asks the user to make a defensive copy of the developer dictionary before returning it but no test tests for that.
I sent the student the following test to make sure they implement a defensive copy:

[Fact]
public void DevelopersCantBeTamperedWith()
{
    var authenticator = new Authenticator(new Identity { EyeColor = "green", Email = "[email protected]" });

    var devs = authenticator.GetDevelopers() as IDictionary<string, Identity>;

    try
    {
        devs.Add("Evil Attacker", new Identity { Email = "[email protected]", EyeColor = "transparent" });
    }
    catch (NotSupportedException)
    {
        // Ignore exception from read-only dictionaries.
    }

    Assert.False(authenticator.GetDevelopers().ContainsKey("Evil Attacker"));
}

Something like that should probably be added to the exercise.

[ErikSchierboom]

We might want to redesign the exercise a bit in the future. As for the missing test for task 5, we want the student to use a readonly dictionary. We could thus have the test be:

[Fact]
public void DevelopersCantBeTamperedWith()
{
    var authenticator = new Authenticator(new Identity { EyeColor = "green", Email = "[email protected]" });
    Assert.IsType<ReadOnlyDictionary<string, Identity>>(authenticator.GetDevelopers());
}

What do you think? The main problem with this, is that it sort of gives away the solution, but then again the introduction also explicitly mentions this.