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

Check SecureMode in a Plack middleware #1431

Closed
bschmalhofer opened this issue Nov 20, 2021 · 4 comments
Closed

Check SecureMode in a Plack middleware #1431

bschmalhofer opened this issue Nov 20, 2021 · 4 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@bschmalhofer
Copy link
Contributor

Currently the authentication checks are done in the interface modules and possibly in Kernel::GenericInterface::Provider. IMHO, it would be nice to refactor that into nice Plack middlewares. However, this might be a bit tricky. Therefore I propose to do a simple case in an examplar fashion. install.pl and migration.pl only check the SecureMode. Thus this case is an easy case for refactoring.

In order to not make otobo.psgi unmanageable, the new middleware should be a module in Kernel/System/Web/Middlewares.

@bschmalhofer bschmalhofer added the enhancement New feature or request label Nov 20, 2021
@bschmalhofer bschmalhofer added this to the OTOBO 10.1 milestone Nov 20, 2021
@bschmalhofer bschmalhofer self-assigned this Nov 20, 2021
@bschmalhofer bschmalhofer changed the title Check SecureMode a middleware Check SecureMode in a Plack middleware Nov 20, 2021
bschmalhofer added a commit that referenced this issue Nov 21, 2021
Also extracting the script_file_name and localising $Kernel::OM
are now also done in middlewares.
@bschmalhofer
Copy link
Contributor Author

A first test looked fine. _installer.pl could be accessed for a new installation. After calling bin/docker/quick_setup.pl access is denied. In the latter case the HTTP status is now 403.
@svenoe : Could you take a look at the PR? It is a little bit more complicated than it could be expected. The reason is that currently no middlewares can be assigned to specific interfaces. At least I didn't see an obvious way.

bschmalhofer added a commit that referenced this issue Nov 22, 2021
So that they can be treated almost like PSGI applications.
Not completely, as the constructor still needs to be called.
bschmalhofer added a commit that referenced this issue Nov 22, 2021
And enhance code comments.
bschmalhofer added a commit that referenced this issue Nov 22, 2021
This makes it ease to assing middlewares to specific interfaces.
otobo.x.script_file_name is no longer needed, as dispatching is done with an URL map.
Set $Env->{SERVER_SOFTWARE} in $SetEnvMiddleware, just to avoid jet another middleware
bschmalhofer added a commit that referenced this issue Nov 22, 2021
This just feels wrong. Separation of concerns is a good thing.
bschmalhofer added a commit that referenced this issue Nov 22, 2021
before the method Content() is called.
Because Content() might want to set input params.
@bschmalhofer
Copy link
Contributor Author

Ah, Plack expects middlewares to reside in Plack::Middleware. So the SecureMode middleware will be called Plack::Middleware::OTOBO::SecureModeAccessFilter.

The interface modules will be outfitted with the method response, thus a interface specifc Plack App will look like:

   sub {
        my ($Env) = @_;

        return Kernel::System::Web::InterfacePublic->new(
            Debug   => 0,      # pass 1 for enabling debug messages
            PSGIEnv => $Env,
        )->Response;
    };

@bschmalhofer
Copy link
Contributor Author

A hopefully final beautification. The interface modules can inherit from Plack::Component, https://metacpan.org/pod/Plack::Component, and Response() can be renamed to call(). This would leave us with:

mount "/public.pl" => Kernel::System::Web::InterfacePublic->new()->to_app;
in otobo.psgi.

bschmalhofer added a commit that referenced this issue Nov 22, 2021
bschmalhofer added a commit that referenced this issue Nov 22, 2021
Reinstating the previous behavior.
Fixing the failure of scripts/test/PSGI/IndexHtml.t
bschmalhofer added a commit that referenced this issue Nov 22, 2021
@bschmalhofer
Copy link
Contributor Author

Looks good so far. Closing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant