firesphere

https://github.com/silverstripe/silverstripe-framework/issues/8524

Show 1 attachment(s)
Firesphere

Affected Version

5.0

Introduction

Currently, the Security class has a getter and setter method for the current user, for the current request. This is a coupling between controller and IdentityStore that should not exist in the first place, but was decided to use for developer practicalities.

Removing this feature and moving it to the IdentityStore would make more sense in the way things are coupled between Controller and Store operations

Purpose and Outcome

The purpose of this change, is to remove the coupling between the Member and Controller state that is currently existing in SilverStripe 4.

The outcome would be, that all checks for the Member state, would flow through the IdentityStore used instead.

Motivation

Because coupling between objects as well as separation of concerns is an important part of Object Oriented approaches, a controller should not contain or know of any state of the member, unless it needs to. Security containing the current member state is violating that principal concept.

Proposal

Move the get/set current user method to the IdentityStore, as a method (not static) and make it publicly available to use. The interface would require this method to be implemented and it's up to the end developer on how to implement it. This could be either way they want instead of the current expectation of it being a Member object. Expected from the IdentityStore however, is that there is always a member object returned as per SilverStripe standards.

This would mean that the setCurrentUser method would be able to accept any given set of data to store in a way that's up to the developer (Although it is obviously a lot easier to store a Member object right away), but getCurrentUser would always return a Member object readily useable by the controller if needed. This means that the getter should map whatever is set in the setter to a SilverStripe Member object for ease of use.

This would include the Security class to be renamed to SecurityController, to clarify it's a controller and not a Security authority.

Pros

• The Controller doesn't need to know the Member state anymore • The IdentityStore completes it's job from start to finish • The concern of requesting a member from a controller is gone • Only complete Member objects are returned, no matter what has been set in the setter • Coupling between request and state reduced • IdentityStore becomes the true storage of Member states, as opposed to the Controller • Instead of a static call, it would become a dynamic call • Security class is no longer easily mistaken for being an authority

Cons

• The IdentityStore needs to make sure that the getter always returns a valid Member object or null • Unlike the current situation, where the setter only accepts a Member • The code needed for getting the Member during any stage of the request will become longer and possibly less readable • The Security class has always been treated as the God of Security State. This would further disappear in favour of the IdentityStore (this is also a Pro, I believe) • Instead of a static call, it would become a dynamic call • All modules currently relying on the Security methods will get deprecation warnings • Or break catastrophically

Mitigations

• It is possible to only give a deprecation notice for SilverStripe 5 instead of fully removing the get/set methods and make the methods proxy to the new methods • Load the renamed SecurityController class as Security, to prevent possible breaks due to incompatible reverse code

Alternatives

• Leave the code as is • This means towards SilverStripe 6, the same consideration needs to be made • Refactor Security in to it's own module • This would imply Security is no longer a core concern. Although I would like this to happen, it does imply Security is a second thought instead of a primary thought

Impact

• The community will hate us for it, because it's once again more words to type • The authentication flow from a MiddleWare perspective is going to need a rewrite on all levels and implementations • Security becomes a plain Controller instead of an Authority • IdentityStore becomes the go-to for Member state • All Security implementations will break due to the change in API • Given this is a 5.0 RFC, I don't see any problems with it

Expected outcome

• IdentityStore becomes what it should be, and Security no longer is a partial IdentityStore • IdentityStore handles the setting for the member for the current request, as opposed to the current situation where Security does this • IdentityStore handles the getting for the member for the current request, as opposed to the current situation where Security does this • IdentityStore is the sole endpoint for anything user storage, user login and logout related • Security Is just a controller, which it is intended to be • Requiring Security static methods for anything Member related will be deprecated • A more unified approach to anything related to the current User

Optional outcomes

• Rename Security to SecurityController, to make it more clear it's "just" a controller

Hide attachment content