[SECURITY] If unique_identfier_field is on a Member subclass, you can't authenticate

[elliot-sawyer]

Affected Version

4.0.1

Description

I have implemented a custom subclass of Member with its own unique_identifier_field called "Username". I can implement my own Authenticator to handle the login, but I've come across an issue where Member::onBeforeWrite() assumes that the identifier is always located on the Member table, due to hard-coded references to Member. (Link) This is not always the case - if a subclass implements the unique_identifier_field, this will throw a validation error. More discussion is in the SS4 user chat

I've come up with three possible workarounds for this: 1) If $tableName = static::class, check if $tableName.unique_identifier_field exists in the database. If not, fall back to Member.unique_identifier_field 2) Refactor these blocks of code into separate methods, so a subclass has the option of overriding them. 3) Have the subclass call DataObject::onBeforeWrite() (credit to @kinglozzer) instead of parent::onBeforeWrite, and implement the changes in the subclass.

Steps to Reproduce

1. Create a subclass MyMember extends Member, with a DB column "Username"
2. Use Config to change MyMember.unique_identifier_field to Username
3. Replace Member with MyMember class using the Injector
4. Create a new subclass MyMemberAuthenticator extends MemberAuthenticator. Re-implement the authenticateMember method, but change any references from Member to MyMember
5. Create a new Member with a username/password
6. Attempt to login with this new Member
7. You should get a ValidationResult exception at this point on Member::onBeforeWrite, because of a missing database column Member.Username

[robbieaverill]

We could just switch this line to static::config()->get('unique_identifier_field'), which would use your custom Member subclass config var if it's there or fall back to Member if not.

It should really be doing this, or $this->config()->get('...') anyway, as opposed to referencing itself literally. If the intention is that the property should be uninherited then it should be using ->uninherited() to get the config value.

A workaround would be to set Member.unique_identifier_field in YAML config, but I agree that your custom subclass should propagate down to this anyway.

[robbieaverill]

Oh, we might need to replace the references to the Member database table with a DataObjectSchema lookup for the current class (->tableName($this)) and references to Member::class to static::class as well, otherwise the config change would probably just break.

[elliot-sawyer]

Sorry, the actual DB error is caused a few lines down here, due to the hardcoded Member table.

[elliot-sawyer]

I tried the static::class trick above, and while it works for me, I think it would cause the reverse problem for everybody else. If you're authenticating on the subclass, but the unique_identifier_field is on Member, the call would also fail.

Checking for the existence of the database column before adding the string to the filters array would be a safe option?

[robbieaverill]

True - it'd probably be better to remove the SQL filter in favour of a standard ORM filter instead, then it wouldn't matter which model defines the column

[dhensby]

1. Subclassing Member in this way is not considered "best practice" and leads to lots of edge-cases (as you're experiencing). Any of the "core" model classes like this should use DataExtensions or replaced with Injector instead of relying on a subclass. You claim to be doing the latter, but if that's so then you'd expect the DB table to be Member and for all the DB fields to be on that table - there should be no multi-table inheritance in play.
2. It would probably be better to remove the table qualifier from the filter regardless as the ORM should be figuring this out itself

[sminnee]

If you're using a subclass field for unique_identifier what does it mean for the identification of members that aren't records of that subclass.

My recommendation here would be to throw a hard error if unique_identifier_field pointed to a field that's not directly on Member.

[robbieaverill]

I'm not sure if I agree with @sminnee or @dhensby =( I think it'd be better for us to fix the edge cases you'd encounter by extending Member in this way rather than advise against doing it at all.