wintercms / wintercms/winter

Revamp authentication events

Open
#296 14 comments 4 reactions 1 assignee View on GitHub

@LukeTowers is already working on this.

Since Sep 1, 2021.

help wanted
Dominant language
PHP
Stars
1.5k
Forks
246
Avg merge
19h 2m
Merged PRs (30d)
7

Description

This is a proposal to revamp the authentication events system within Winter CMS. Feedback and suggestions very much welcome. Related: https://github.com/wintercms/wn-user-plugin/pull/11 & https://github.com/wintercms/storm/pull/48

The current events are as follows:

  • backend.user.login -> Global event for backend users logging in
  • model.auth.afterImpersonate -> Local event triggered after impersonation is stopped
  • model.auth.beforeImpersonate -> Local event triggered before impersonation is started
  • canBeImpersonated() -> Model method used if impersonation events don't return anything
  • winter.user.activate -> Global event triggered after Winter.User user's activate for the first time
  • winter.user.reactivate -> Global event triggered after Winter.User user's activate after being deactivated
  • winter.user.login -> Global event triggered after Winter.User user's login
  • winter.user.deactivate -> Global event triggered after Winter.User user's deactivate
  • winter.user.activate -> Global event triggered after Winter.User user's activate
  • winter.user.getNotificationVars -> Not technically part of the auth system, used for extending the variables made available to the Winter.Notify notifications
  • winter.user.logout -> Global event triggered after Winter.User user's logout
  • winter.user.beforeAuthenticate -> Global event triggered immediately before Winter.User user's credentials are checked
  • winter.user.beforeRegister -> Global event triggered before Winter.User user's registration is processed
  • winter.user.register -> Global event triggered after Winter.User user's registration is processed

The revised system would remove any direct event logic from the Auth system and instead move it to model event methods in the form of the following:

Model events:

  • beforeRegister()
  • afterRegister()
  • beforeLogin() (already exists)
  • afterLogin() (already exists)
  • beforeLogout()
  • afterLogout()
  • beforeActivate($activationCode)
  • afterActivate($activationCode)
  • beforeResetPassword($resetCode, $newPassword)
  • afterResetPassword()
  • beforeImpersonate($impersonator)
  • afterImpersonate($impersonator)
  • beforeEndImpersonation($impersonator)
  • afterEndImpersonation($impersonator)
  • beforeSuspend()
  • afterSuspend()
  • beforeUnsuspend()
  • afterUnsuspend()
  • beforeBan()
  • afterBan()
  • beforeUnban()
  • afterUnban()

The Backend module & Winter.User plugin would then use those methods to fire the externally accessible events:

  • *.user.beforeRegister
  • *.user.register
  • *.user.beforeLogin
  • *.user.login
  • *.user.beforeLogout
  • *.user.logout
  • *.user.beforeImpersonate
  • *.user.impersonate
  • *.user.beforeEndImpersonate
  • *.user.endImpersonate
  • *.user.beforeResetPassword
  • *.user.resetPassword
  • *.user.beforeActivate
  • *.user.activate
  • *.user.beforeDeactivate
  • *.user.deactivate
  • *.user.beforeReactivate
  • *.user.reactivate
  • *.user.beforeSuspend
  • *.user.suspend
  • *.user.beforeUnsuspend
  • *.user.unsuspend
  • *.user.beforeBan
  • *.user.ban
  • *.user.beforeUnban
  • *.user.unban

Some questions to consider:

  • What needs to be done with the existing events to retain BC as much as possible? The impersonation events are unlikely to be in wide spread use at the moment, but everything else existing is fair game.
  • How many of these newly proposed events are redundant in that developers can relatively easily use already existing more generic model local events in order to accomplish the same end results? Should we avoid adding new events for those already addressable use-cases and instead publish a guide for using those existing events to achieve the same results?
  • Which of these events should be halting vs non-halting? What return values should be supported if any? What arguments will be provided if any? Should each event be global or local to the user instance?
  • Are any of these events likely to be utilized within high traffic workflows? I.e. what are the performance implications of adding these additional events; are there any concerns about that?
  • What extra use cases (beyond just easier targeting for logging entries into an audit log) will the addition of each new event support?
  • Should we also add an authentication log that tracks these events for every user and display it in the backend as a system level view and a contextual view? Should we be logging extra data for these events if so or leave it to a plugin to worry about that? (extra data like user agent and IP address).
  • The previous developer guidelines recommended never using "after" terminology for events, however this was inconsistently applied even within the core (model events for example for the most part have after versions, although some don't instead using the unmodified verb as the "after" event name); should we retain that recommendation or revert it to being it more in line with the model events? How does that effect our existing collection of events?

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.