New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Remove Security::{get/set}CurrentUser #8524

Open
Firesphere opened this Issue Oct 27, 2018 · 8 comments

Comments

Projects
None yet
5 participants
@Firesphere
Copy link
Contributor

Firesphere commented Oct 27, 2018

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
@robbieaverill

This comment has been minimized.

Copy link
Member

robbieaverill commented Oct 27, 2018

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, but getCurrentUser would always return a Member object readily useable by the controller if needed.

If we're going to allow arbitrary input then it might be nice to add a IdentityInterface or something which can be type hinted in setCurrentUser

[Con:] Instead of a static call, it would become a dynamic call

I'm not sure this matters - we can use a singleton service and/or dependency injection to get the IdentityProvider. The only downside of this is a downside of this entire RFC in that there will be tonnes of supported and community modules (as well as user code) that currently relies on Security::getCurrentUser()

The purpose of this change, is to remove the coupling between the Member and Controller state

+1

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

We could probably achieve this easily by making Security::setCurrentUser() and Security::getCurrentUser() proxies for IdentityStore instead of a static property. If you did this you could make the change in the next 4.x minor release and deprecate these methods for 5.x removal.

IdentityStore becomes what it should be, and Security no longer is a partial IdentityStore

Can you clarify this wording a little? "what is should be" could be explicit. I feel like the objective for the Security controller is that is no longer has any responsibility for storing or returning the current user, right?

@Firesphere

This comment has been minimized.

Copy link
Contributor

Firesphere commented Oct 27, 2018

If we're going to allow arbitrary input then it might be nice to add a IdentityInterface or something which can be type hinted in setCurrentUser

IdentityStore is the interface currently, Session or Cookie store are the implementations

The only downside of this is a downside of this entire RFC in that there will be tonnes of supported and community modules (as well as user code) that currently relies on Security::getCurrentUser()

I did not realise that, stupidly enough of me, you are correct. Your suggestion of deprecating and proxying it to the IdentityStore (Like we did with the DefaultAdministrator) sounds like a less volatile approach 👍

Can you clarify this wording a little? "what is should be" could be explicit. I feel like the objective for the Security controller is that is no longer has any responsibility for storing or returning the current user, right?

Wording updated a bit (in a few places) to adjust to your comments and clarify a few things more

@Firesphere

This comment has been minimized.

Copy link
Contributor

Firesphere commented Nov 18, 2018

I'd love to have some more feedback on this thought, before I'm going to dive in to it and actually make these changes as to not waste my time. @robbieaverill can you ping the core team for me? I can't ;)

@ScopeyNZ

This comment has been minimized.

Copy link
Contributor

ScopeyNZ commented Nov 18, 2018

@silverstripe/core-team

I don't think we can remove these static methods. It's already relied on by heaps of code: https://github.com/search?l=PHP&q=%22Security%3A%3AgetCurrentUser%22&type=Code .

I do think it's a good idea to refactor to be a proxy of IdentityStore. I'm always going to be a proponent of SRP. This could probably be a minor change without looking at the code - worth a quick PR? 😉

@maxime-rainville

This comment has been minimized.

Copy link
Contributor

maxime-rainville commented Nov 18, 2018

I broadly agree with the stated goal.

I like the "Refactor Security in to it's own module" alternative. Basically, security is not really about security ... it's about authentication and user management. You could in theory build an application that doesn't need users at all. At which points, all the Security bits is just dead weight.

I'm kind of ambivalent on removing the static method from the current controller. If we don't remove it, we're just pushing down the pain further down the line. It's an easy rule to add to the upgrader to tell people to switch to the identity store.

I guess to me it kind of depends on how big of a deal SS5 turns out to be. If it's just a maintenance release where we don't introduce too many new APIs and just deprecate old ones, I'd say we could delay it to SS6. If SS5 turns out to be a big departure from SS4, than we might as well remove the static methods from Security.

@robbieaverill

This comment has been minimized.

Copy link
Member

robbieaverill commented Nov 19, 2018

I guess to me it kind of depends on how big of a deal SS5 turns out to be. If it's just a maintenance release where we don't introduce too many new APIs and just deprecate old ones, I'd say we could delay it to SS6. If SS5 turns out to be a big departure from SS4, than we might as well remove the static methods from Security.

I guess it's things like this that will decide that =)

For me, the biggest benefit of this PR is getting the user out of a controller and into a service pattern

@Firesphere

This comment has been minimized.

Copy link
Contributor

Firesphere commented Nov 24, 2018

I do think it's a good idea to refactor to be a proxy of IdentityStore. I'm always going to be a proponent of SRP. This could probably be a minor change without looking at the code - worth a quick PR?

What does SRP mean?

Further, I do believe making it a proxy method for the duration of SS4 at least, it would otherwise be a breaking change, which is not very high on the list of SemVer good practices ;)

@wilr

This comment has been minimized.

Copy link
Member

wilr commented Nov 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment