Refactor and isolate OAuth / OIDC helpers in enterprise/authentication#1148
Conversation
Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>
🤖 Augment PR SummarySummary: This PR refactors the enterprise OAuth/OIDC implementation by extracting the protocol helpers into isolated, header-only modules under Changes:
🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| target_compile_definitions(sourcemeta_one_authentication PUBLIC SOURCEMETA_ONE_ENTERPRISE) | ||
| # The OAuth and OIDC modules are header-only and enterprise-only, kept apart | ||
| # from the shared include tree so they extract cleanly when upstreamed | ||
| target_include_directories(sourcemeta_one_authentication PUBLIC |
There was a problem hiding this comment.
sourcemeta_one_authentication now exposes enterprise/authentication as a PUBLIC include dir (OAuth/OIDC are header-only), but the libraries those headers call into (sourcemeta::core::crypto, sourcemeta::core::uri, etc.) are still linked PRIVATE. This can cause downstream targets that include these headers (e.g., the enterprise server actions) to hit unresolved symbols unless they also link those deps explicitly.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| sourcemeta_test(NAMESPACE sourcemeta PROJECT one NAME enterprise_authentication | ||
| SOURCES authentication_test.cc session_test.cc discovery_test.cc | ||
| oidc_test.cc) | ||
| SOURCES authentication_test.cc session_test.cc) |
There was a problem hiding this comment.
This drops discovery_test.cc/oidc_test.cc from the enterprise auth unit target, which removes coverage for the refactored discovery parsing, PKCE challenge derivation, and ID-token parsing/validation helpers. Since these helpers are still security-critical behavior (even though now header-only), it seems worth ensuring equivalent unit coverage still exists.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| #endif | ||
|
|
||
| #include <sourcemeta/one/authentication_discovery.h> | ||
| #include <sourcemeta/one/authentication_error.h> |
There was a problem hiding this comment.
sourcemeta/one/authentication.h no longer includes the discovery/OIDC helper headers (and those public headers are removed), which is a breaking change for any downstream code that previously relied on those symbols via this umbrella include. If this API reduction is intentional, consider ensuring release/upgrade notes (or an enterprise-only replacement include path) cover the change so consumers don’t silently lose those interfaces.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 16 files
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="src/authentication/CMakeLists.txt">
<violation number="1" location="src/authentication/CMakeLists.txt:11">
P1: The `enterprise/authentication` include directory is exposed as `PUBLIC`, making the header-only OAuth/OIDC helpers available to downstream targets. However, the libraries those inline functions call into (`sourcemeta::core::crypto`, `sourcemeta::core::uri`, `sourcemeta::core::jose` via the OIDC header) are still linked `PRIVATE`. Downstream consumers that include these headers and instantiate the inline functions in their own translation units will hit unresolved symbols at link time unless they independently link those deps. Either promote `crypto`, `uri`, and `jose` to `PUBLIC` here, or keep them `PRIVATE` and make the include directory `PRIVATE` as well (requiring downstream targets to add the include path themselves alongside explicit link deps).</violation>
</file>
<file name="enterprise/unit/authentication/CMakeLists.txt">
<violation number="1" location="enterprise/unit/authentication/CMakeLists.txt:2">
P2: Removing `discovery_test.cc` and `oidc_test.cc` from the enterprise unit target drops ~450 lines of tests covering security-critical behavior: PKCE challenge derivation (pinned against RFC 7636 Appendix B), ID-token signature verification, nonce binding, audience restriction, and subject validation. Since the logic is unchanged (just moved to header-only), equivalent unit coverage should still exist. If these tests have been relocated to a different target, this is fine—but I don't see replacement tests in the changeset.</violation>
</file>
<file name="enterprise/authentication/authentication_oidc.h">
<violation number="1" location="enterprise/authentication/authentication_oidc.h:80">
P1: OIDC discovery responses with a mismatched `issuer` are accepted because this metadata parser drops the required issuer field. That violates Discovery provider-configuration validation and lets an otherwise valid JSON response redirect authorization, token, and JWKS operations to endpoints belonging to a different issuer; retaining and checking the returned issuer against the configured value would keep discovery bound to the configured provider.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| sourcemeta_test(NAMESPACE sourcemeta PROJECT one NAME enterprise_authentication | ||
| SOURCES authentication_test.cc session_test.cc discovery_test.cc | ||
| oidc_test.cc) | ||
| SOURCES authentication_test.cc session_test.cc) |
There was a problem hiding this comment.
P2: Removing discovery_test.cc and oidc_test.cc from the enterprise unit target drops ~450 lines of tests covering security-critical behavior: PKCE challenge derivation (pinned against RFC 7636 Appendix B), ID-token signature verification, nonce binding, audience restriction, and subject validation. Since the logic is unchanged (just moved to header-only), equivalent unit coverage should still exist. If these tests have been relocated to a different target, this is fine—but I don't see replacement tests in the changeset.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At enterprise/unit/authentication/CMakeLists.txt, line 2:
<comment>Removing `discovery_test.cc` and `oidc_test.cc` from the enterprise unit target drops ~450 lines of tests covering security-critical behavior: PKCE challenge derivation (pinned against RFC 7636 Appendix B), ID-token signature verification, nonce binding, audience restriction, and subject validation. Since the logic is unchanged (just moved to header-only), equivalent unit coverage should still exist. If these tests have been relocated to a different target, this is fine—but I don't see replacement tests in the changeset.</comment>
<file context>
@@ -1,6 +1,5 @@
sourcemeta_test(NAMESPACE sourcemeta PROJECT one NAME enterprise_authentication
- SOURCES authentication_test.cc session_test.cc discovery_test.cc
- oidc_test.cc)
+ SOURCES authentication_test.cc session_test.cc)
target_link_libraries(sourcemeta_one_enterprise_authentication_unit
</file context>
| SOURCES authentication_test.cc session_test.cc) | |
| SOURCES authentication_test.cc session_test.cc discovery_test.cc | |
| oidc_test.cc) |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Benchmark Index (community)
Details
| Benchmark suite | Current: a257b66 | Previous: 43e17de | Ratio |
|---|---|---|---|
Add one schema (0 existing) |
333 ms |
327 ms |
1.02 |
Add one schema (100 existing) |
28 ms |
30 ms |
0.93 |
Add one schema (1000 existing) |
85 ms |
87 ms |
0.98 |
Add one schema (10000 existing) |
708 ms |
783 ms |
0.90 |
Update one schema (1 existing) |
21 ms |
22 ms |
0.95 |
Update one schema (101 existing) |
29 ms |
30 ms |
0.97 |
Update one schema (1001 existing) |
87 ms |
89 ms |
0.98 |
Update one schema (10001 existing) |
701 ms |
817 ms |
0.86 |
Cached rebuild (1 existing) |
7 ms |
7 ms |
1 |
Cached rebuild (101 existing) |
9 ms |
9 ms |
1 |
Cached rebuild (1001 existing) |
29 ms |
30 ms |
0.97 |
Cached rebuild (10001 existing) |
244 ms |
267 ms |
0.91 |
Index 100 schemas |
492 ms |
441 ms |
1.12 |
Index 1000 schemas |
1486 ms |
1467 ms |
1.01 |
Index 10000 schemas |
13521 ms |
14032 ms |
0.96 |
Index 10000 schemas (custom meta-schema) |
16443 ms |
17074 ms |
0.96 |
Index 10000 schemas ($ref fan-out) |
16351 ms |
16623 ms |
0.98 |
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: a257b66 | Previous: 43e17de | Ratio |
|---|---|---|---|
Add one schema (0 existing) |
396 ms |
400 ms |
0.99 |
Add one schema (100 existing) |
105 ms |
104 ms |
1.01 |
Add one schema (1000 existing) |
159 ms |
157 ms |
1.01 |
Add one schema (10000 existing) |
798 ms |
759 ms |
1.05 |
Update one schema (1 existing) |
97 ms |
96 ms |
1.01 |
Update one schema (101 existing) |
102 ms |
102 ms |
1 |
Update one schema (1001 existing) |
160 ms |
156 ms |
1.03 |
Update one schema (10001 existing) |
765 ms |
767 ms |
1.00 |
Cached rebuild (1 existing) |
8 ms |
8 ms |
1 |
Cached rebuild (101 existing) |
10 ms |
10 ms |
1 |
Cached rebuild (1001 existing) |
31 ms |
30 ms |
1.03 |
Cached rebuild (10001 existing) |
252 ms |
250 ms |
1.01 |
Index 100 schemas |
749 ms |
497 ms |
1.51 |
Index 1000 schemas |
1579 ms |
1525 ms |
1.04 |
Index 10000 schemas |
13945 ms |
13610 ms |
1.02 |
Index 10000 schemas (custom meta-schema) |
17195 ms |
16533 ms |
1.04 |
Index 10000 schemas ($ref fan-out) |
17023 ms |
16426 ms |
1.04 |
This comment was automatically generated by workflow using github-action-benchmark.
1 participant
Signed-off-by: Juan Cruz Viotti jv@jviotti.com