mod_auth_cas icon indicating copy to clipboard operation
mod_auth_cas copied to clipboard

Multi backend

Open studersi opened this issue 7 years ago • 20 comments

This replaces pull request #36.

This pull request consists of the three commits made by @bnoordhuis plus a merge commit that resolves the conflicts.

studersi avatar Oct 02 '18 15:10 studersi

@studersi Can you try git rebase to rebase onto master and then force-push? If you have git rerere configured, you probably don't need to anything else, it'll auto-fix the merge conflicts for you.

bnoordhuis avatar Oct 08 '18 09:10 bnoordhuis

@bnoordhuis Is there any advantage to rebasing instead of a merge commit?

studersi avatar Oct 08 '18 12:10 studersi

I could write a small novel about the pros and cons but specifically for this PR and project, fast-forward commits are easier to review and bisect (as in: git bisect - it frequently stops at the merge commit.)

bnoordhuis avatar Oct 08 '18 13:10 bnoordhuis

@bnoordhuis Ok, I did as you suggested and rebased the branch against the master branch.

studersi avatar Oct 09 '18 06:10 studersi

@bnoordhuis Correct, the issue from the old PR is not addressed. All I did was to resolve the merge conflicts.

studersi avatar Oct 09 '18 19:10 studersi

@dhawes & @forsetti I saw mod_auth_cas v1.2 was released last month without the multi-backend feature, even though the PR https://github.com/apereo/mod_auth_cas/pull/36 was tagged for the milestone 1.2. Is it being held back because of this issue: https://github.com/apereo/mod_auth_cas/pull/36#issuecomment-10226107? If yes, are there any plans to address this?

studersi avatar Mar 18 '19 14:03 studersi

I simply have not been able to review and test it closely. I do intend on reviewing it.

My primary concern without looking at the code is will this break configuration for users?

dhawes avatar Mar 22 '19 20:03 dhawes

@dhawes Has there been any progress regarding this pull request?

Just as a side note, we intend to mitigate the unresolved issue mentioned here by using the CASRenew directive. Whenever the user navigates to a path that is handled by a different CAS server, the user is forced to log in again. Do you think this would be a viable solution for the moment?

studersi avatar Nov 04 '19 11:11 studersi

No progress has been made mostly because I don't have an easy way to test multiple CAS servers. I should have those resources by the end of this year.

Has any analysis been done on whether current user configurations may be affected?

I don't understand the unresolved issue enough to comment on whether CASRenew is a viable solution.

dhawes avatar Nov 14 '19 22:11 dhawes

We have not done an extensive analysis of whether the behaviour changes for users. For us, it just provides the additional option of configuring multiple CAS backends, having only one is still an option.

When can we expect this to be merged? Or do you need anything else from our side?

studersi avatar Mar 02 '20 10:03 studersi

I've finally done some testing with multiple CAS servers, and to that end this patch works as expected. My simple configs were not affected by the changes as well.

Are the attributes that were moved from server config to dir config just the ones you needed, or the ones that make sense? Do you anticipate users will want other directives moved in the future?

I feel like I need to fully understand the issue referenced here before merging, especially since @pames felt strongly that it should be resolved. I'm not quite there yet.

The merge conflicts need to be resolved as well. That was fairly straightforward when I tested this today.

dhawes avatar Apr 06 '20 21:04 dhawes

@dhawes Thanks for looking into it.

I do not think we need more directives to be set on a directory level. We have had this patch in use for a while now and weren't missing any directives.

As for the other problem, since the different auth services are used for different directories, the CASScope directive can be used to limit the scope of a ticket to that directory. Also, there is the CASRenew directive to force the user to log in again. Those two mechanisms should mitigate the issue.

I could fix the merge conflicts but I will wait until the other questions are settled.

studersi avatar May 05 '20 11:05 studersi

@dhawes Is this topic settled, or is there anything else?

studersi avatar Jul 01 '20 06:07 studersi

@dhawes Is there anything else or is this ready to be merged? In that case I could resolve the merge conflicts but I do not want to do it just to wait for other merge conflicts to come up again.

studersi avatar Aug 03 '20 07:08 studersi

So the proposal here is to limit or mitigate the URL issue solely with configuration? In that case I would expect documentation updates to the README. I think using the config options you mention will work for most cases, but fixing the underlying issue does seem to be the correct solution here.

Another config option could be to set a different CASCookiePath for each directory to force a redirect to the correct server.

Honestly, I don't know what to do with this PR. I see the value, but I worry that configuration will be confusing to users and cause them to open up more of their servers than they expected. Of course, it's likely that most users will not even use this, so maybe that's unfounded.

I'm genuinely on the fence as to whether to:

  1. Update documentation to give warnings about using multiple servers and ways to work around them.
  2. Resolve https://github.com/apereo/mod_auth_cas/pull/36#issuecomment-10226107 by adding URL to the ticket metadata.

dhawes avatar Aug 11 '20 21:08 dhawes

If the issue can be mitigated by adding information to the ticket, this is surely better than just documenting the issue and relying on people to check the documentation. I cannot tell, however, how much work this would entail and whether or not I could be of assistance to implement this.

studersi avatar Oct 06 '20 13:10 studersi

What is the status here? Will this be added to the ticket?

studersi avatar Feb 02 '21 08:02 studersi

I think adding the URL to the ticket metadata is probably best.

Is this something you'd like to try and implement?

dhawes avatar Feb 18 '21 22:02 dhawes

I pushed a branch with a proof of concept for adding the CASLoginURL to the cache:

https://github.com/dhawes/mod_auth_cas/tree/multi-backend-dhawes

GitHub
An Apache 2.0/2.2 compliant module that supports the CASv1 and CASv2 protocols. - GitHub - dhawes/mod_auth_cas at multi-backend-dhawes

dhawes avatar Oct 21 '21 19:10 dhawes

@dhawes Thanks! I will check if we can try this out with one of our services.

studersi avatar Nov 22 '21 12:11 studersi