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

Ability to define the SSO route's domain? #41

Closed
nathan-io opened this issue Feb 5, 2022 · 6 comments · Fixed by #44
Closed

Ability to define the SSO route's domain? #41

nathan-io opened this issue Feb 5, 2022 · 6 comments · Fixed by #44

Comments

@nathan-io
Copy link

Hi,

Not a bug, just a question.

We're using subdomain routing in our Laravel application. Our authentication routes are bound to the "account." subdomain.

Accordingly, I'd like to have the sso.login route bound to this subdomain only. It is currently a global route that you can access from any (sub)domain at discourse/sso.

Not sure how to accomplish this.

Thank you!

@jimmypuckett
Copy link
Member

I am sorry that I am not really following your question without some example code. Maybe post up some of your route file?

@nathan-io
Copy link
Author

nathan-io commented Feb 18, 2022

Hi Jimmy,

We have several route files, one for each subdomain. The SSO routes aren't defined by us in our route files, but by the package itself. So I'm not sure how that would be helpful.

Laravel allows you to bind a route to a (sub)domain (docs).

The way the package registers its routes, they are valid on any subdomain that the application listens on. This is undesired behavior, because we don't want someone (for example) hitting https://api.site.com/discourse/sso, or https://admin.site.com/discourse/sso.

We want to bind the route exclusively to the account. subdomain, so it can only be accessed at https://account.site.com/discourse/sso.

Laravel Nova, Laravel Horizon, and Laravel Telescope are examples of packages that allow you to define the subdomain that the routes are registered on, via a config file. And when Nova (for example) registers its routes, they are then registered only on that subdomain.

    /*
    |--------------------------------------------------------------------------
    | Nova Domain Name
    |--------------------------------------------------------------------------
    |
    | This value is the "domain name" associated with your application. This
    | can be used to prevent Nova's internal routes from being registered
    | on subdomains which do not need access to your admin application.
    |
    */

    'domain' => env('NOVA_DOMAIN_NAME', 'admin.' . config('app.domain')),

So because we've configured that, you can access Nova only on https://admin.site.com and not on any of the other subdomains our application listens on.

Hope this makes sense.

To implement this, I imagine all that's needed is to check a config() value for the presence of a domain name when the package is registering its routes. If found, bind the SSO routes to that domain using the domain() method on Route:

Route::domain(config('services.discourse.route_domain', null)->group(function () {
   // define SSO routes
}

Just not sure if you can pass null there as the fallback value. The idea is just that if it's not defined, we register the routes globally.

@jimmypuckett
Copy link
Member

@nathan-io I just pushed a #44 feature branch, that allows adding a domain into the config. If you want to pull it into your project & test it, then I will clean up it. merge & tag it.

@nathan-io
Copy link
Author

Thanks @jimmypuckett , working great!

Result of php artisan route:list | grep sso

With the services.discourse.domain config value set:

| account.site.test | GET|HEAD      | discourse/sso  | sso.login | Spinen\Discourse\Controllers\SsoController@login

Without that config value being set:

|                   | GET|HEAD      | discourse/sso  | sso.login | Spinen\Discourse\Controllers\SsoController@login

@jimmypuckett
Copy link
Member

OK, I will merge & tag.

@jimmypuckett
Copy link
Member

I just pushed 2.7.0.

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

Successfully merging a pull request may close this issue.

2 participants