add CASRedirectAfterValidation parameter to mimic java-cas-client's parameter #136
Conversation
…edirectAfterValidation parameter
| @@ -1844,8 +1856,10 @@ char *getResponseFromServer (request_rec *r, cas_cfg *c, char *ticket) | |||
| curl_easy_setopt(curl, CURLOPT_URL, apr_uri_unparse(r->pool, &validateURL, 0)); | |||
|
|
|||
| if(curl_easy_perform(curl) != CURLE_OK) { | |||
| if(c->CASDebug) | |||
| if(c->CASDebug) { | |||
| ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, "MOD_AUTH_CAS: query: %s", validateURL.query); | |||
There was a problem hiding this comment.
I'd probably move the previous log addition to line 1855 after validateURL.query is set so we get the whole validateURL printed out.
There was a problem hiding this comment.
The log at line 1858 is redundant since it's logged at 1852 now.
| apr_table_add(r->headers_out, "Location", newLocation); | ||
| return HTTP_MOVED_TEMPORARILY; | ||
| } else { | ||
| return OK; |
There was a problem hiding this comment.
Is it intended that we go all the way to https://github.com/ruckc/mod_auth_cas/blob/eba78415e60b2d597c5ee03afd5245f53129a15a/src/mod_auth_cas.c#L2231 to return OK? Should it return before that?
There was a problem hiding this comment.
The return OK; was unmodified (except additional indention).
There was a problem hiding this comment.
It's now inside the if block that checks CASDisableRedirectAfterValidation ( eba7841#diff-b823cf0e10152100b941acd0fb5838a8R2141 ), so it never returns if CASDisableRedirectAfterValidation is On.
There was a problem hiding this comment.
I added an additional } else { return OK; }. In my office environment I had caught this but forgot to commit it back.
There was a problem hiding this comment.
If that's the intention, this can be simplified by changing the check to:
if(parametersRemoved == TRUE && d->CASDisableRedirectAfterValidation == NULL )
I mostly ask because the original merge request would fall through to a later return where cas attributes were populated and headers were set. The side effect of this is that Require cas-attribute statements would work. In this commit that won't happen, so authorization will fail. What is desired here?
|
I believe these two commits address all of your concerns. |
|
If there are no further issues I can close this PR and resubmit with these changes in a single commit. |
|
There's no need to open another PR. When we're happy we'll just rebase, squash the commits, and make it nice and tidy. |
| printPort = TRUE; | ||
|
|
||
| if(c->CASRootProxiedAs.is_initialized) { | ||
| if(d->CASDisableRedirectAfterValidation == NULL) { |
There was a problem hiding this comment.
Why not:
if(d->CASDisableRedirectAfterValidation) {
return OK;
}
and keep everything in the "if(parametersRemoved == TRUE)" unchanged? The patch and code are much more readable like that.
This is a redo of #51. This is necessary when having APIs CAS protected, as it makes the logic even more cumbersome.