[#311] Make client request extend lifetime of bearer token#447
[#311] Make client request extend lifetime of bearer token#447korydraughn wants to merge 4 commits intoirods:mainfrom
Conversation
core/src/common.cpp
Outdated
| // Extend the lifetime of the bearer token. | ||
| static const auto token_lifetime = | ||
| config.at(nlohmann::json::json_pointer{"/http_server/authentication/basic/timeout_in_seconds"}) | ||
| .get<int>(); | ||
| client_info.expires_at = now + std::chrono::seconds{token_lifetime}; |
There was a problem hiding this comment.
One thing I'm thinking about is whether the token_lifetime value should have a dedicated config property.
We can start with this and add a new property later if people want/need that. I think that's the right approach. We're still likely a long way away from 1.0 status, so there's time.
|
Forgot I need to update the README to talk about this too. |
MartinFlores751
left a comment
MartinFlores751
left a comment
There was a problem hiding this comment.
Nice to see the visit addition to the process stash! Most of my comments are just questions, with one suggestion on the readme.
| // token expires. | ||
| // The amount of time before a user's authentication token expires. | ||
| // The lifetime of a token will be extended by this amount on each | ||
| // use as long as the token has not expired. |
There was a problem hiding this comment.
"Use" might be a bit vague to an admin unfamiliar with how we use our tokens. "Request" might help clear up that this is used on every basic HTTP API request.
| // use as long as the token has not expired. | |
| // request as long as the token has not expired. |
There was a problem hiding this comment.
consider...
- has not expired
+ has not yet expired| if (now >= client_info.expires_at) { | ||
| token_expired = true; | ||
| return; | ||
| } | ||
|
|
||
| if (json_res.empty()) { | ||
| logging::error("{}: Could not find bearer token matching [{}].", __func__, bearer_token); | ||
| return {.response = fail(status_type::unauthorized)}; | ||
| } | ||
| // Extend the lifetime of the bearer token. | ||
| static const auto token_lifetime = | ||
| config.at(nlohmann::json::json_pointer{"/http_server/authentication/basic/timeout_in_seconds"}) | ||
| .get<int>(); | ||
| client_info.expires_at = now + std::chrono::seconds{token_lifetime}; | ||
|
|
||
| // Do mapping of user to irods user | ||
| auto user{map_json_to_user(json_res)}; | ||
| if (user) { | ||
| return {.client_info = {.username = *std::move(user)}}; | ||
| } | ||
| client_info_copy = client_info; | ||
| }); |
There was a problem hiding this comment.
Do we always want to increase the token lifetime? In what case(s) do we want the token to timeout?
There was a problem hiding this comment.
Do we always want to increase the token lifetime?
I think so.
In what case(s) do we want the token to timeout?
I'm currently only aware of the "inactive for N seconds" case. Have you identified others?
core/src/process_stash.cpp
Outdated
| std::shared_lock read_lock{g_mtx}; | ||
| if (g_stash.find(_key) != std::end(g_stash)) { | ||
| read_lock.unlock(); | ||
| std::lock_guard write_lock{g_mtx}; | ||
| if (auto iter = g_stash.find(_key); iter != std::end(g_stash)) { | ||
| _visitor(iter->second); | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
If people are using only basic auth mode, would it be better to have the shared_lock -> lock_guard or just have a lock_guard?
If OIDC mode is enabled, do we expect the auth traffic to be split between basic and OIDC?
| // The amount of time before a user's authentication token expires. | ||
| // The lifetime of a token will be extended by this amount on each | ||
| // use as long as the token has not expired. |
There was a problem hiding this comment.
Maybe it's just me, but this wording feels a little inaccurate. The expiration of the token is set to timeout_in_seconds from the point in time of each use, not extended by that amount... right?
To me, it sounds like the expiration will be extended by (by default) an hour each time the token is used. So if I authenticate 4 times in a second, the expiration will be 4 hours away instead of 1 hour away.
Maybe I'm misunderstanding all this.
There was a problem hiding this comment.
Your question indicates the wording isn't clear. Showing some math is likely a better fit. For example:
token_lifetime = <current_time> + <timeout_in_seconds>
The documentation would be updated to align with the idea too.
Thoughts?
|
should we mark this as draft? |
|
Yes. Converting to draft until we determine the best path forward. |
4 participants
Cannot automate a test for this yet since it requires modifying the HTTP API's config file.
Confirmed the changes work by setting the lifetime of bearer tokens (for Basic Authentication) to 15 seconds and stat'ing a collection multiple times near the expiration time.
I'll open an issue as a reminder to get some automated tests around this in the future.