Unable to revoke non-admin users

[emteknetnz]

Non-admin user isn't able to revoke their own sessions.

With a test user in the group "Content authors", when attempting to revoke a session I see a red toast "Could not log out of session. Try again later"

Originally noted on this pull-request https://github.com/silverstripe/silverstripe-session-manager/pull/62#issuecomment-823742134

ACs

• [x] Non-admin users are able to revoke their own sessions
• [x] Admin users are able to revoke their own sessions
• [x] Behat and cucumber studio test are updated to require CMS users with restricted access.
• [x] Logout end point is covered by functional test to make sure anyone can terminate sessions and the CSRF token is enforced

Pull request

• [ ] https://github.com/silverstripe/silverstripe-session-manager/pull/68
• [x] https://studio.cucumber.io/projects/161118/test-plan/folders/1850251/scenarios/6076158 (Updated scenario in cucumber)

[maxime-rainville]

I've updated the ACs

I spent some time trying to solve this through #62. There's a couple thoughts:

• LoginSessionController is not covered by any unit test which is probably why we miss this for so long
  • Should test that any user can logout
  • Should test that the CSRF token is enforce
  • Should test the response code
• DELETE admin/loginsession/remove/123 is a weird REST API call, it should be DELETE admin/loginsession/123
  • My beef with the HTTP response code remain https://github.com/silverstripe/silverstripe-session-manager/issues/45
• A quick fix could be to add private static $required_permission_codes = false; to LoginSessionController. This mimics what CMSProfileController does.
• It's not clear to me LoginSessionController needs to be a LeftAndMain.
• It's not clear to me LoginSessionController should even exists. Other form fields that need AJAX endpoint add them as URL handlers directly on the FormField.

In short, this is an example of how cutting corners and not following best practices does not actually save you time in the long run.

[emteknetnz]

My preference is do it all and refactor the whole thing to make it more proper. And of course there's no sane argument against "Do it right first time"

I'm just not sure the time investment is worth it at this stage in the project though? Seems like the refactor itself could easily add several days once peer-review and testing is taken into consideration?

Other than "code less bad" what's the long term benefit here? There's no plan at this stage to develop or extend this module any further beyond this project.

Seems like MVP here is:

• Add private static $required_permission_codes = false;
• Should test that any user can logout
• Should test that the CSRF token is enforce
• Behat and cucumber studio test are updated to require CMS users with restricted access.

?

[maxime-rainville]

Obviously, I don't know that if we don't refactor this bit of the system, some horrible calamity will befall us. It's totally possible - even probable - that we could get away with the suggested MVP and never have to look at this bit of code ever again.

But it's worth pointing out that this refactor is relatively cheap right now:

• It's a very small piece of code that can easily be copy pasted in another class
• Fortunately, there's no unit test for this class right (which on another level is absolutely horrible and disgusting)
• We don't have any API commitments right now.

If we ship this like this in version 1.0.0, we're stuck keeping it like this forever.

On a side note that's probably more relevant for a post-project retro, there seems to have been very little planning around the architecture of this module or review of the pre-existing implementation. I would argue that this led to a lot of those late critical issues:

• this issue
• the bad permission implementation https://github.com/silverstripe/silverstripe-session-manager/issues/57
• the impossibility to invalidate a session without invalidating all other sessions https://github.com/silverstripe/silverstripe-session-manager/issues/46

[emteknetnz]

Cucumber test looks good