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

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

Closed
1 task done
AMDHome opened this issue Feb 6, 2024 · 6 comments

Comments

@AMDHome
Copy link

AMDHome commented Feb 6, 2024

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?

  • 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

@ssddanbrown
Copy link
Member

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?

@AMDHome
Copy link
Author

AMDHome commented Feb 8, 2024

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.

@ssddanbrown
Copy link
Member

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.

@AMDHome
Copy link
Author

AMDHome commented Feb 8, 2024

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.

@ssddanbrown
Copy link
Member

@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 ssddanbrown changed the title OIDC_AUTO_REGISTER variables to be added Implement a new AUTH_PRE_REGISTER logical theme event, intended to allow custom logic for registration events Feb 9, 2024
@ssddanbrown ssddanbrown added this to the Next Feature Release milestone Feb 9, 2024
ssddanbrown added a commit that referenced this issue Feb 21, 2024
Included tests to cover.
Manually tested on standard and social (GitHub) auth.
For #4833
@ssddanbrown
Copy link
Member

The discussed theme event has now been added in 055bbf1, 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;
});

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

No branches or pull requests

2 participants