BookStack icon indicating copy to clipboard operation
BookStack copied to clipboard

Implement a new `AUTH_PRE_REGISTER` logical theme event, intended to allow custom logic for registration events

Open AMDHome opened this issue 1 year ago • 5 comments

Describe the feature you'd like

Allow users to manually specify whether or not they want to auto register OIDC authorized users.

This is similar to #2174 but for OIDC instead of SAML

Describe the benefits this would bring to existing BookStack users

Currently if you set up OIDC, anyone who attempts to log in with your provider will have a profile created for them if it doesn't exist. This feature would disable auto user creation and allow you to specify users in the user table

  • OIDC_AUTO_REGISTER=true - would have the same behavior as it does now
  • OIDC_AUTO_REGISTER=false - would require a user to be in the user table before granting access

Can the goal of this request already be achieved via other means?

This can be manually modded into bookstacks by editing the source code as shown in my PR #4831.

Although having it bundled into the releases would be nice.

Have you searched for an existing open/closed issue?

  • [X] I have searched for existing issues and none cover my fundamental request

How long have you been using BookStack?

Under 3 months

Additional context

No response

AMDHome avatar Feb 06 '24 20:02 AMDHome

Thanks for the request @AMDHome.

From my usage of OIDC/SAML2 auth systems, they typically provide decent (and often role-based) per-app access control on the auth side of things. In my view it's always made sense that we retain delegation for access control management to the auth systems themselves.

Is there a particular reason why this can't be managed at the auth-system-level in your scenario?

ssddanbrown avatar Feb 08 '24 12:02 ssddanbrown

So the short version is I don't have access to those access controls so I have to do them on the client side.


The long version is: I work for a public university, and would like to hook into their SSO services for login. For various reasons, our SSO department has not released the access control panel for me (and other people) to manage user access. They give out various reasons, but I feel like it has more to do with workplace bureaucracy. Either way, there's nothing I can do to get access.

In theory I can ask someone from the SSO department to make the changes on my behalf, but they are typically reluctant and/or slow. On top of that my department employs a number of student assistants who would need access. Student assistants have a relatively high turnover rate, so I would need to bother them every few weeks or so.

In the end, its easier to just do it client side because I have the ability to do it myself.

AMDHome avatar Feb 08 '24 16:02 AMDHome

Thanks @AMDHome for the added context, that makes sense.

As reflected within your PR #4831, I'm hesitant to add options, especially where they're to suit less common business process like this. Especially as common functionality would then be expected across other main auth methods, and I'm worried we'd just get future requests for additional filter control to suit other logic that can't be controlled auth-side (I vaguely remember a request in the past for filtering on specific user data).

I'm thinking, as a way to allow all of this, across all auth methods, we could add a logical theme event of AUTH_PRE_REGISTER. We already emit an AUTH_REGISTER event, which is fired just after a new user is registered. Instead we can fire the AUTH_PRE_REGISTER just before the user is saved, but after all other existing logic. The event could pass in the intended user details, and allow a boolean return type to indicate if the registration should be allowed or not.

Using this, you could just return false from such an event for your intended functionality. Otherwise, it's open to be flexibly used. For example, you could lookup user details against a file/database/api to decide if registration is allowed.

ssddanbrown avatar Feb 08 '24 17:02 ssddanbrown

No worries, I completely understand your feelings about the feature request for businesses.

Realistically the PR is just for me to be lazy so I don't have to manually add it back in myself after every update. Also since I already figured it out, might as well share the solution to anyone else who might want it.

The AUTH_PRE_REGISTER event sounds like it would work.

AMDHome avatar Feb 08 '24 17:02 AMDHome

@AMDHome Thanks for the feedback.

In that case, I'll update this issue to focus on adding the new theme system event. Should be something easy to add for next feature release, so I'll add this to that milestone. I'll also close your existing PR, thanks again for offering that.

Coincidently, I had another unique request on Reddit yesterday which could be solved by such a mechanism being in place.

ssddanbrown avatar Feb 09 '24 17:02 ssddanbrown

The discussed theme event has now been added in 055bbf17de2a0a2f6fb28596937a8652013b792a, to be part of the next feature release.

An example of a logical theme system functions.php file, to prevent login, would look like this:

<?php

use BookStack\Theming\ThemeEvents;
use BookStack\Facades\Theme;

Theme::listen(ThemeEvents::AUTH_PRE_REGISTER, function (string $authSystem, array $userData) {
    return false;
});

ssddanbrown avatar Feb 21 '24 15:02 ssddanbrown