nbgrp / onelogin-saml-bundle

OneLogin SAML Symfony Bundle
BSD 3-Clause "New" or "Revised" License
43 stars 13 forks source link

allow runtime resolution of app url #7

Closed cdaguerre closed 2 years ago

cdaguerre commented 2 years ago

This allows inferring the service-provider urls during runtime, rather than "hard-coding" them. One could argue that runtime configuration is also possible through the use of env vars, eg.:

sp:
    entityId: '%env(APP_URL)%/saml/metadata' 
    assertionConsumerService:
        url: '%env(APP_URL)%/saml/acs'
    singleLogoutService:
        url: '%env(APP_URL)%/saml/acs'

But: 1/ this requires the environment variables to be set ;) 2/ I can't think of a case where they wouldn't match the current request's host so I'm not sure this needs to be configurable at all (but maybe I'm missing something?)

I think it makes sense to have sensible defaults in the bundle's configuration, still allowing to change things. This is also much more convenient during development because it just works, regardless of the scheme (TLS or not), port and host of your dev server (even localhost) without having to configure anything at all.

As a side note, we've been using the hslavich bundle for years maintaining our own fork to achieve this through service decoration and to address other issues than this bundle has addressed (like the hardcoded firewall name in the login controller for example). This is the last pain point for us ;)

a-menshchikov commented 2 years ago

@cdaguerre hello! Could you please explain your use case where you need it?

codecov-commenter commented 2 years ago

Codecov Report

Merging #7 (47108f6) into 1.0 (c7044e6) will decrease coverage by 3.96%. The diff coverage is 19.35%.

Impacted Files Coverage Δ
src/Onelogin/Auth.php 0.00% <0.00%> (ø)
src/Onelogin/AuthArgumentResolver.php 93.33% <75.00%> (-6.67%) :arrow_down:
...ncyInjection/Compiler/AuthRegistryCompilerPass.php 100.00% <100.00%> (ø)
src/DependencyInjection/Configuration.php 100.00% <100.00%> (ø)
src/Controller/Metadata.php 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c7044e6...47108f6. Read the comment docs.

cdaguerre commented 2 years ago

@cdaguerre hello! Could you please explain your use case where you need it?

Hi @a-menshchikov sorry for opening the PR without a description, I updated the description.

a-menshchikov commented 2 years ago

@cdaguerre thank you for detailed description!

I have made another PR (#8) where using service factory for Auth instantiating instead of extending of original Auth class. This approach looks more conventional for me and want to know your opinion.

Also I've renamed <request_host> to <request_scheme_and_host> and used Symfony\Component\HttpFoundation\Request::getSchemeAndHttpHost instead of composing of this value by myself.

Let me know if I can close this PR in favor of #8.

cdaguerre commented 2 years ago

@a-menshchikov #8 is indeed a far cleaner approach, thank you very much!!