Why can I set the properties of a privately set property?

I have a class LoginManager which has a private field currentUser and a public property CurrentUser to not allow myself to accidentally change the value of CurrentUser from outside the LoginManager class.

CurrentUser is { get; } only but I can still change properties in the underlying currentUser which is private.

eg.

Console.WriteLine(loginManager.CurrentUser.ClockedIn.ToString()); // true
loginManager.CurrentUser.ClockedIn = false;
Console.WriteLine(loginManager.CurrentUser.ClockedIn.ToString()); // false
loginManager.CurrentUser.ClockedIn = true;
Console.WriteLine(loginManager.CurrentUser.ClockedIn.ToString()); // true

LoginManager.cs

public class LoginManager
    {
        private User? currentUser { get; set; }
        private readonly ApplicationDbContext dbContext;

        public event EventHandler CurrentUserChanged;

        public User? CurrentUser
        {
            get { return currentUser; }
        }

        //...
    }

User.cs

public class User
    {
        public Guid Id { get; set; }
        public string Name { get; set; }
        public string Username { get; set; }
        public string Password { get; set; }
        public bool ClockedIn { get; set; }

    }

I want it so that I can't change CurrentUser from outside the LoginManager class. Could anybody please point me in the right direction?

Solution 1:

The first thing to understand is that the lacking set accessor only prevents you from changing the value of the CurrentUser property itself. That is, it prevents you from changing it to another User object (or null). However, these rules only apply to the property itself, and not anything inside of it. The read-only nature of properties is not recursive.

If you want to prevent changing the properties inside the User object contained in CurrentUser, you'll need to have an immutable User object of some sort, or you must prevent direct access to the object and provide helper methods that allow retrieving certain data from the user without being able to change it. The two options are thus to hide direct access or to make the User object somehow immutable itself.

The first option is hiding access to the User object:

public User? CurrentUser { get; }

public string GetCurrentUserName() {
    return CurrentUser?.Name;
}

public string GetCurrentUserUsername() {
    return CurrentUser?.UserName;
}

The above follows the so-called Law of Demeter, but I don't see it used too often as it is not ergonomic and bloats up classes. Only go this route if many of the details of the current user aren't important to code using the LoginManager and if such code doesn't need to be able to pass around the User object.

The second option has some suboptions. You can create an IReadOnlyUser interface that only exposes read-only properties:

interface IReadOnlyUser {
    public string Name { get; }
    public string Username { get; }
}

public class User : IReadOnlyUser {
    // although these have setters, they cannot be access through variables of type IReadOnlyUser (see below)
    public string Name { get; set; }
    public string Username { get; set; }
}

public class LoginManager {
    // you can use a field here instead of a property since it is private
    private User currentUser;

    // returns an read-only view of the user, so write accessors are not available
    public IReadOnlyUser CurrentUser {
        get { return currentUser; }
    }
}

// usage
var lm = new LoginManager();
Console.WriteLine(lm.CurrentUser.Name);  // okay because IReadOnlyUser has a read accessor for Name
lm.CurrentUser.Name = "foo";   // not allowed because IImutableUser does not expose a write accessor
}

Additionally, you can do as in the other answer and make the fields in the User class read-only, either with readonly fields or with properties that only have get accessors. It depends on whether you feel there are cases where you should be able to modify properties of User objects, just not in the context of the LoginManager.

Finally, if you do not control the User class, you can create a facade class that wraps a User object but only exposes getters:

public class ReadOnlyUser {
    private User user;
    public ReadOnlyUser(User user) { this.user = user; }
    public string Name { get { return user.Name; } }
}

// in LoginManager
private ReadOnlyUser currentUser;
public ReadOnlyUser CurrentUser { 
    get { return currentUser; } 
    set {
        currentUser = new ImmutableUser(value);
    }
}

// usage
var lm = new LoginManager();
lm.CurrentUser = userFromSomewhereElse;
Console.Log(lm.CurrentUser.Name);  // okay because ReadOnlyUser exposes this property from the underlying User class
lm.CurrentUser.Name = "foo";  // not allowed because ReadOnlyUser doesn't have a setter for Name

There is, in my mind, no particular single right answer. It depends on what you want to do.

Solution 2:

I think I see the problem, the private CurrentUser instances get and set properties are exposed outside the class since those member variables are public in scope.

Why not make the User properties private?

public class User
{
    private Guid Id { get; set; }
    private string Name { get; set; }
    private string Username { get; set; }
    private string Password { get; set; }
    private bool ClockedIn { get; set; }

}

Or if you need to get the properties mark private set:

public class User
{
    public Guid Id { get; private set; }
    public string Name { get; private set; }
    public string Username { get; private set; }
    public string Password { get; private set; }
    public bool ClockedIn { get; private set; }
}

Or explicitly make them readonly so they can only be initialized in the Constructor:

public class User
{
    public readonly Guid Id { get; private set; }
    public readonly string Name { get; private set; }
    public readonly string Username { get; private set; }
    public readonly string Password { get; private set; }
    public readonly bool ClockedIn { get; private set; }

    public User(Guid Id, string Name, string Username, string Password, bool ClockedIn) 
    {
        this.Id = Id;
        this.Name = NameName;
        this.Username = Username;
        this.Password = Password;
        this.ClockedIn = ClockedIn;
    }
}