Multi backend

[studersi]

This replaces pull request #36.

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

[bnoordhuis]

@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.

[studersi]

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

[bnoordhuis]

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.)

[studersi]

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

[bnoordhuis]

Thanks, LGTM at a quick glance (and insofar you can sign off on your own code.)

There was an unresolved issue in the old PR. I assume this PR doesn't address that, or does it?

[studersi]

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

[studersi]

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

[dhawes]

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?

[studersi]

@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?

[dhawes]

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.

[studersi]

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?

[dhawes]

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.

[studersi]

@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]

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

[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.

[dhawes]

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 Add support for multiple upstream CAS servers #36 (comment) by adding URL to the ticket metadata.

[studersi]

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]

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

[dhawes]

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

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

[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

GitHub - dhawes/mod_auth_cas at multi-backend-dhawes

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

[studersi]

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