Skip to content
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

DOC Add information regarding Security::setCurrentUser() #10061

Conversation

emteknetnz
Copy link
Member

Comment on lines 71 to 81
#### Important information regarding Security::setCurrentUser($member)

When the session manager module is installed, any custom code calling `Security::setCurrentUser($member)` before creating a mock HTTP request, notably `FunctionalTest::get()` and `FunctionalTest::post()` will need to be updated. Functional tests should be updated to `$this->logInAs($member)` and `$this->logOut()`, while regular application code should be updated to `Injector::inst()->get(IdentityStore::class)->logIn($member);` and `Injector::inst()->get(IdentityStore::class)->logOut();`. Code that is not creating a mock HTTP request should be able to continue using `Security::setCurrentUser($member)`.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to give people a bit of an overview about where the problem is coming from before digging into what the solution is. We should also be explicit that what they were doing before was wrong.

I don't think we need to go into the details of how you would log a user into the regular logic. If you call setCurrentUser in your actual code thinking that this would survive into the next request, your site is already busted with or without session manager.

Suggested change
#### Important information regarding Security::setCurrentUser($member)
When the session manager module is installed, any custom code calling `Security::setCurrentUser($member)` before creating a mock HTTP request, notably `FunctionalTest::get()` and `FunctionalTest::post()` will need to be updated. Functional tests should be updated to `$this->logInAs($member)` and `$this->logOut()`, while regular application code should be updated to `Injector::inst()->get(IdentityStore::class)->logIn($member);` and `Injector::inst()->get(IdentityStore::class)->logOut();`. Code that is not creating a mock HTTP request should be able to continue using `Security::setCurrentUser($member)`.
#### Test should not use `Security::setCurrentUser($member)` when mocking HTTP request
When writing automated test, you can use `FunctionalTest::get()` and `FunctionalTest::post()` to mock HTTP requests. Until now, developers could abuse the `Security::setCurrentUser($member)` method to define which member those mocked requests would run against.
`Security::setCurrentUser()` is *stateless* ... its effect only last for the current request. When mocking an HTTP request, session-manager logs out the mocked user if it was defined with `Security::setCurrentUser()`.
Your tests should use `$this->logInAs($member)` and `$this->logOut()` when mocking HTTP requests. It is still appropriate to use `Security::setCurrentUser()` when testing stateless logic. e.g.: Testing that a `DataObject`'s `canView` method returns the correct value for the current user.
Review the [Functional Testing developer documentation](/developer_guides/testing/functional_testing/#loginas) for more details on `logInAs()` and `logOut()`.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've taken you suggestions and made some slight tweaks to them. It still has all the same points.

src/Security/Security.php Show resolved Hide resolved
@emteknetnz emteknetnz force-pushed the pulls/4/doc-set-current-member branch 2 times, most recently from 84b71ef to 04d9ee7 Compare August 22, 2021 22:31
@emteknetnz emteknetnz changed the base branch from 4 to 4.9 August 29, 2021 22:46
@emteknetnz emteknetnz merged commit d614cc6 into silverstripe:4.9 Sep 6, 2021
@emteknetnz emteknetnz deleted the pulls/4/doc-set-current-member branch September 6, 2021 02:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants