rewrite: implicitly handle internal redirects #119
Conversation
src/mod_auth_gssapi.c
Outdated
| while (main_req->prev) | ||
| main_req = main_req->prev; | ||
|
|
||
| if (main_req->main) { |
There was a problem hiding this comment.
Can you explain this check ?
Also it would be nice if you can explain the intended logic for getting to the "proper" main request.
There was a problem hiding this comment.
It was intended as just-in-case check, to confirm we got the initial request (as we just checked for prev), however I didn't see such scenario in logs, and there is no such check in mod_auth_digest.c so I think it shouldn't happen.
src/environ.c
Outdated
| "KRB5CCNAME", | ||
| "GSS_SESSION_EXPIRATION", | ||
| NULL | ||
| }; |
There was a problem hiding this comment.
How do we make sure to keep this list current if we export new variables later ?
Should we just have tests confirming each env variable is present ? Or should we come up with some more elaboarate way to reference/set env vars that forces the programmer to update this list ?
There was a problem hiding this comment.
Right, I wanted something better but it got complicated. What I had in mind was some intermediate function which will be called from both mag_set_req_data() and mag_dup_req_data(), in which we centralize the export of environment variables.
Another idea is to create a table in which variables are added as they are being determined, at the end, we save this table with ap_set_module_config(req->request_config) and only then call a generic function to export the table into req's environment.
Later, in subrequest / internal-redirect, we retrieve the table with ap_get_module_config(main_req->request_config), and export it via the generic function (which by the way would also confirm we got the main request which we have authenticated).
Thoughts?
There was a problem hiding this comment.
I think using a table is a very good idea, this way if we add later we do not risk the two functions getting out of sync.
Please add the table stuff.
frozencemetery
left a comment
frozencemetery
left a comment
There was a problem hiding this comment.
Barring already discussed changes, this looks good to me. Please rebase onto master while you're there. Thanks for adding a test.
iboukris
force-pushed
the
internal_redirects
branch
from
f74280e to
1cfb1e5
Compare
|
Updated based on review comments, please review - thanks! |
iboukris
force-pushed
the
internal_redirects
branch
from
1cfb1e5 to
7a784b5
Compare
simo5
left a comment
simo5
left a comment
There was a problem hiding this comment.
Looks good, just a minor change is needed.
src/mod_auth_gssapi.c
Outdated
|
|
||
| /* It is either a subrequest or an internal redirect */ | ||
| while (!ap_is_initial_req(main_req)) | ||
| main_req = main_req->main ?: main_req->prev; |
There was a problem hiding this comment.
Please use and explicit if/else condition here, this syntax is hard to read and can easily lead to mistakes
tests/httpd.conf
Outdated
| IncludeOptional conf.d/*.conf | ||
|
|
||
| CoreDumpDirectory /tmp | ||
| CoreDumpDirectory "${HTTPROOT}/logs" |
There was a problem hiding this comment.
Why this change ? Seems unrelated to the patch
There was a problem hiding this comment.
Right, I'll remove it from this commit.
Do you want it in a separate commit? I like all test data to be kept in scratchdir.
There was a problem hiding this comment.
Yeah probably better to have a spearate PR, and the dirs should probably be something like "${HTTPROOT}/cores" ?
There was a problem hiding this comment.
I'm fine with simply "${HTTPROOT}" to bump it up.
Internal redirects are a special case of subrequest - they have no req->main but req->prev instead, so we should check for that too in case the request is not initial. Also, make sure to export MAG environment variables to subrequests and internal redirects. Signed-off-by: Isaac Boukris <iboukris@gmail.com> Reported-by: scopev24
iboukris
force-pushed
the
internal_redirects
branch
from
7a784b5 to
1805eb9
Compare
3 participants
Internal redirects are a special case of subrequest - they
have no req->main but req->prev instead, so we should check
for that too in case the request is not initial.
Also, make sure to export MAG environment variables to
subrequests and internal redirects.
Signed-off-by: Isaac Boukris iboukris@gmail.com
Reported-by: scopev24
Fixes #114