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

Multi backend #160

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Multi backend #160

wants to merge 3 commits into from

Conversation

studersi
Copy link
Contributor

@studersi studersi commented Oct 2, 2018

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
Copy link
Contributor

@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
Copy link
Contributor Author

studersi commented Oct 8, 2018

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

@bnoordhuis
Copy link
Contributor

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

This commit makes it possible to use mod_auth_cas with more than one one CAS
server.

It is now possible to configure CASLoginURL, CASValidateURL and other directives
on a per-directory level, not just on the per-server level.
@studersi
Copy link
Contributor Author

studersi commented Oct 9, 2018

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

Copy link
Contributor

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

studersi commented Oct 9, 2018

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

@studersi
Copy link
Contributor Author

@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
Copy link
Contributor

dhawes commented Mar 22, 2019

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
Copy link
Contributor Author

studersi commented Nov 4, 2019

@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
Copy link
Contributor

dhawes commented Nov 14, 2019

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
Copy link
Contributor Author

studersi commented Mar 2, 2020

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
Copy link
Contributor

dhawes commented Apr 6, 2020

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
Copy link
Contributor Author

studersi commented May 5, 2020

@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
Copy link
Contributor Author

studersi commented Jul 1, 2020

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

@studersi
Copy link
Contributor Author

studersi commented Aug 3, 2020

@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
Copy link
Contributor

dhawes commented Aug 11, 2020

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
Copy link
Contributor Author

studersi commented Oct 6, 2020

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
Copy link
Contributor Author

studersi commented Feb 2, 2021

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

@dhawes
Copy link
Contributor

dhawes commented Feb 18, 2021

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

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

dhawes added a commit to dhawes/mod_auth_cas that referenced this pull request Oct 18, 2021
dhawes added a commit to dhawes/mod_auth_cas that referenced this pull request Oct 18, 2021
@dhawes
Copy link
Contributor

dhawes commented Oct 21, 2021

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

@studersi
Copy link
Contributor Author

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

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 this pull request may close these issues.

3 participants