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

Don't alter routes from the CAS module #90

Open
pfrenssen opened this issue Jun 18, 2019 · 3 comments · May be fixed by #92
Open

Don't alter routes from the CAS module #90

pfrenssen opened this issue Jun 18, 2019 · 3 comments · May be fixed by #92

Comments

@pfrenssen
Copy link
Member

For some reason the module is altering the path of the cas.login route to /eulogin. This is breaking the functionality as documented in the CAS module. There is no value in doing this.

The code responsible for this is as follows, as you can see there is no reason given to explain why this breaking change is required:

  protected function alterRoutes(RouteCollection $collection): void {
    // ...

    // Replace the cas callback route controller.
    if ($route = $collection->get('cas.proxyCallback')) {
      $route->setDefaults([
        '_controller' => '\Drupal\oe_authentication\Controller\ProxyCallbackController::callback',
      ]);
    }

    // Replace default cas login route with eulogin one.
    if ($route = $collection->get('cas.login')) {
      $route->setPath('/eulogin');
    }
  }

Also no reason is given for altering the proxyCallback controller.

Please make sure that these paths are made available without breaking the default URLs provided by the CAS module.

@pfrenssen
Copy link
Member Author

This change was done in #71. Also in the PR no explanation is given why is it necessary to replace the existing path with /eulogin. Since it appears there are no pressing reasons for this I would suggest to make this path available in addition to the existing one.

@brummbar
Copy link
Contributor

Hello @pfrenssen , which parts of the CAS module functionality is broken?
Changing any reference from CAS to EU Login is one of the business requirements that we have.
oe_authentication is meant to be used to connect to EU Login, not a generic CAS system.

For the proxyCallback controller you are indeed right, we are missing quite a bit of documentation.
By looking at the original ticket, it was done to cover this:

EU Login implements a Proxy Credentials system to allow an application (the Proxy) to impersonate users and invoke a backend service protected by EU Login (the Target).

@pfrenssen
Copy link
Member Author

pfrenssen commented Jun 21, 2019

I'm writing a Behat test trait that will make it easy to run authentication tests against CAS. I want to make this trait reusable for all projects that are using the CAS module, but as soon as I install oe_authentication this trait no longer works since the /cas path vanishes.

Since I could not see any reason given for making the default path inaccessible I am assuming the actual requirement was to have the /eulogin path available as a way to redirect to the CAS server. Probably it was unintentional to remove /cas. I have posted a PR that makes /eulogin available while still keeping the default path working.

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