Let an expired session renew itself at the provider - #1178
Conversation
Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>
🤖 Augment PR SummarySummary: This PR adds “silent session renewal” for OIDC SSO so an expired local session can re-check the provider without showing the sign-in page. Changes:
🤖 Was this summary useful? React with 👍 or 👎 |
| // The marker goes either way: an attempt that did not come back with a | ||
| // grant must not be made again on the next navigation, or every one of | ||
| // them takes the same detour to the same answer | ||
| if (silent != nullptr) { |
There was a problem hiding this comment.
In enterprise_server_action_auth_callback_v1.h:169, silent renewals are only special-cased on ?error, but other silent-failure paths (missing code, token exchange/validation failures) still go through fail()/incomplete() and can become user-visible 4xx/5xx responses. Since the renewal marker is only cleared in this error branch, those failures could also keep re-triggering renewal attempts on later denials.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
1 issue found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="enterprise/authentication/authentication.cc">
<violation number="1" location="enterprise/authentication/authentication.cc:835">
P2: Silent renewal can redirect a path to `/auth/login/{name}` even when that endpoint resolves the same duplicate name to a different policy. Preserve the first-match name semantics here (then test whether that entry governs `path`), or reject duplicate OIDC names when serializing.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| const auto *policies{ | ||
| static_cast<const AuthenticationPolicyEntry *>(this->policies_)}; | ||
| for (std::uint32_t index{0}; index < this->policy_count_; index += 1) { | ||
| if ((mask & (PolicySet{1} << index)) == 0) { |
There was a problem hiding this comment.
P2: Silent renewal can redirect a path to /auth/login/{name} even when that endpoint resolves the same duplicate name to a different policy. Preserve the first-match name semantics here (then test whether that entry governs path), or reject duplicate OIDC names when serializing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At enterprise/authentication/authentication.cc, line 835:
<comment>Silent renewal can redirect a path to `/auth/login/{name}` even when that endpoint resolves the same duplicate name to a different policy. Preserve the first-match name semantics here (then test whether that entry governs `path`), or reject duplicate OIDC names when serializing.</comment>
<file context>
@@ -821,6 +821,43 @@ struct Authentication::Impl {
+ const auto *policies{
+ static_cast<const AuthenticationPolicyEntry *>(this->policies_)};
+ for (std::uint32_t index{0}; index < this->policy_count_; index += 1) {
+ if ((mask & (PolicySet{1} << index)) == 0) {
+ continue;
+ }
</file context>
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
Benchmark Index (community)
Details
| Benchmark suite | Current: 8c6fdc9 | Previous: 4ff0741 | Ratio |
|---|---|---|---|
Add one schema (0 existing) |
324 ms |
314 ms |
1.03 |
Add one schema (100 existing) |
31 ms |
24 ms |
1.29 |
Add one schema (1000 existing) |
96 ms |
73 ms |
1.32 |
Add one schema (10000 existing) |
833 ms |
639 ms |
1.30 |
Update one schema (1 existing) |
22 ms |
17 ms |
1.29 |
Update one schema (101 existing) |
32 ms |
24 ms |
1.33 |
Update one schema (1001 existing) |
96 ms |
77 ms |
1.25 |
Update one schema (10001 existing) |
823 ms |
640 ms |
1.29 |
Cached rebuild (1 existing) |
8 ms |
5 ms |
1.60 |
Cached rebuild (101 existing) |
10 ms |
6 ms |
1.67 |
Cached rebuild (1001 existing) |
34 ms |
20 ms |
1.70 |
Cached rebuild (10001 existing) |
286 ms |
173 ms |
1.65 |
Index 100 schemas |
619 ms |
387 ms |
1.60 |
Index 1000 schemas |
1450 ms |
1234 ms |
1.18 |
Index 10000 schemas |
13849 ms |
11301 ms |
1.23 |
Index 10000 schemas (custom meta-schema) |
16783 ms |
14394 ms |
1.17 |
Index 10000 schemas ($ref fan-out) |
16799 ms |
14188 ms |
1.18 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Benchmark Index (enterprise)
Details
| Benchmark suite | Current: 8c6fdc9 | Previous: 4ff0741 | Ratio |
|---|---|---|---|
Add one schema (0 existing) |
405 ms |
313 ms |
1.29 |
Add one schema (100 existing) |
107 ms |
157 ms |
0.68 |
Add one schema (1000 existing) |
163 ms |
635 ms |
0.26 |
Add one schema (10000 existing) |
820 ms |
734 ms |
1.12 |
Update one schema (1 existing) |
99 ms |
78 ms |
1.27 |
Update one schema (101 existing) |
105 ms |
80 ms |
1.31 |
Update one schema (1001 existing) |
164 ms |
134 ms |
1.22 |
Update one schema (10001 existing) |
808 ms |
736 ms |
1.10 |
Cached rebuild (1 existing) |
9 ms |
5 ms |
1.80 |
Cached rebuild (101 existing) |
11 ms |
6 ms |
1.83 |
Cached rebuild (1001 existing) |
33 ms |
18 ms |
1.83 |
Cached rebuild (10001 existing) |
269 ms |
144 ms |
1.87 |
Index 100 schemas |
528 ms |
597 ms |
0.88 |
Index 1000 schemas |
1485 ms |
1104 ms |
1.35 |
Index 10000 schemas |
14224 ms |
12830 ms |
1.11 |
Index 10000 schemas (custom meta-schema) |
17013 ms |
13782 ms |
1.23 |
Index 10000 schemas ($ref fan-out) |
17257 ms |
16617 ms |
1.04 |
This comment was automatically generated by workflow using github-action-benchmark.
Signed-off-by: Juan Cruz Viotti jv@jviotti.com