feat(cas-auth): support logged-in user header #11445
Conversation
|
@lucasarnulphy Do we need to consider using a general method that can forward more info that returned from CAS to upstream, for extensibility purpose? And test cases needed? :D |
|
I think you might be right about returning more info, but I'm not sure it should be exclusive. Many systems just read a single header with a username and then build all permissions on that. Returning a lot of information in a single header would make that harder to implement. I do agree it would be great for the upstreams to have more information, maybe as a UserInfo header as it is done in the openid connect plugin, I'll probably implement that next. As far as tests are concerned... it's only a few lines of codes to return a variable that was already stored and tested before, so I think we're in the clear... but I'll let you be the judge of that. |
apisix/plugins/cas-auth.lua
Outdated
| else | ||
| -- refresh the TTL | ||
| store:set(session_id, user, SESSION_LIFETIME) | ||
| if conf.set_user_header then |
There was a problem hiding this comment.
the set_user_header field is not needed, this feature can be implemented using the user_header field alone. Also please choose a better name to allow any header to be passed.
eg: field name: send_headers and the type of this field should be array, to allow multiple headers to be sent.
There was a problem hiding this comment.
I'm fine with removing set_user_header, and having user_header empty by default.
As far as send_headers is concerned, I'm not sure I understand what you mean.
Do you mean allowing the plugin to put in a header any value from the cas response ? Because that would require a lot more work as currently only the user is stored.
Or do you mean having a general array which we can search in in the code when other headers are implemented ? because that would remove the ability to overwrite the headers names, which would make it painful to integrate with some apps.
There was a problem hiding this comment.
What about the hash/mapping way as following:
{
"XPATH_such_as: /auth/user/node": "X-Forwarded-User", // could be the default setting
"other_xpath_node_to_extract_value": "X-Forwarded-Foo"
}
Some related Lua libs:
There was a problem hiding this comment.
Because that would require a lot more work as currently only the user is stored.
Why would it require a lot more work? I don't understand you. If any of the headers configured in send_headers is not present in the request then the plugin won't add it. Isn't this simple?
There was a problem hiding this comment.
@shreemaan-abhishek I still don't understand what you mean.
For this part:
the set_user_header field is not needed, this feature can be implemented using the user_header field alone. Also please choose a better name to allow any header to be passed.
I agree.
But for this part:
Why would it require a lot more work? I don't understand you. If any of the headers configured in send_headers is not present in the request then the plugin won't add it. Isn't this simple?
I have no understanding of what you mean. If send_headers is not present in which request ? The CAS response ?
I feel like we're not talking about the same thing. The goal of this change is to allow an upstream service to retrieve a value already stored: the username of the authenticated user.
Yes it could be made more generic, by storing more values from the cas response body:
ngx_re_match(res.body, "cas:user(.*?)</cas:user>", "jo");
But it's currently an issue for many users, and it can be improved afterwards to allow for any value for the response.
Other authentication plugins on Apisix have a user_header parameter to pass the authenticated user to upstream services, why does this have to be different ?
There was a problem hiding this comment.
Other authentication plugins on Apisix have a user_header parameter to pass the authenticated user to upstream services, why does this have to be different ?
can you show an example?
There was a problem hiding this comment.
never mind, I understood what you are trying to do.
I'm fine with removing set_user_header, and having user_header empty by default.
please push this change then we can resolve this conversation.
|
Ping @lucasarnulphy :D |
I'd be happy to implement that, but I'm waiting for @shreemaan-abhishek to confirm that it would be acceptable, as I have no intention to implement a change that would later be rejected again. |
@shreemaan-abhishek We need you. :-) |
dosubot
bot
added
the
size:S
|
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the [email protected] list. Thank you for your contributions. |
I'd dont really see what kind of tests you'd like me to add. |
|
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the [email protected] list. Thank you for your contributions. |
|
This pull request/issue has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
3 participants
Description
The cas-auth plugin is great, but does not currently support retrieving the logged-in user from upstreams, making it really hard to do any kind of access control appart from "a user is logged-in".
This PR adds the
set_user_headeranduser_headerattributes that enables setting the logged-in user in a header, and allows for the header's name customization.Some users have already asked for it.
As far as backward compatibility is concerned: The header is set by default, so one might experience issues if setting the header before reaching apisix... which would make little sense considering apisix handles the authentication.
Fixes #9524.
Checklist