Feature EP-765 User Identity based on a Bearer access token - #1181
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe session manager now creates authenticated sessions from JWT access tokens. ChangesSession authentication
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Request
participant SessionManager
participant BasicUser
participant BasicSession
Request->>SessionManager: Provide Authorization access token
SessionManager->>SessionManager: Parse token claims
SessionManager->>BasicUser: Build user identity
SessionManager->>BasicSession: Create authenticated session
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@common/oatbox/user/BasicUser.php`:
- Line 25: Add a directly preceding DocBlock to the BasicUser class declaration,
including the required license information specified by the repository’s path
instructions. Do not alter the class implementation or its User interface
contract.
- Line 1: Add strict typing declarations in both affected files: place
declare(strict_types=1); immediately after the PHP opening tag in
common/oatbox/user/BasicUser.php, and after the file header before imports in
common/session/class.SessionManager.php.
In `@common/session/class.SessionManager.php`:
- Around line 128-150: Update parseAccessToken() to strictly validate Base64URL
decoding and JSON decoding, returning null for invalid input without suppressing
errors, and ensure the decoded payload is an array. In
createAccessTokenSession(), require token['user']['login'] to be a string before
passing it to preg_match(), so malformed tokens fall back to anonymous session
handling.
In `@test/unit/common/session/SessionManagerTest.php`:
- Around line 53-84: Extend SessionManagerTest with isolated negative cases
covering tampered signatures, expired tokens, malformed payloads, and non-string
user.login values. Build each invalid token using the existing token helpers,
invoke SessionManager::getSession(), and assert it does not create a BasicUser;
retain the existing valid-token tests unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: https://raw.githubusercontent.com/oat-sa/tao-code-quality/main/coderabbit/php/authoring/v1/.coderabbit.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
Run ID: a063695f-a5e2-4529-9edd-5db7393eddea
📒 Files selected for processing (6)
.github/workflows/continuous-integration.yamlcommon/oatbox/user/BasicUser.phpcommon/session/class.SessionManager.phpcomposer.jsontest/unit/ConfigurationTest.phptest/unit/common/session/SessionManagerTest.php
8e946bb to
b74546e
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
b74546e to
7994ca2
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
7994ca2 to
5616ec5
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@common/session/class.SessionManager.php`:
- Around line 18-22: Add declare(strict_types=1); immediately after the opening
PHP tag in SessionManager.php, before the namespace or imports.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: https://raw.githubusercontent.com/oat-sa/tao-code-quality/main/coderabbit/php/authoring/v1/.coderabbit.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
Run ID: 226a15fa-e2e2-4ee1-9a7c-d590bafd7bcc
📒 Files selected for processing (6)
.github/workflows/continuous-integration.yamlcommon/oatbox/user/BasicUser.phpcommon/session/class.SessionManager.phpcomposer.jsontest/unit/ConfigurationTest.phptest/unit/common/session/SessionManagerTest.php
🚧 Files skipped from review as they are similar to previous changes (4)
- common/oatbox/user/BasicUser.php
- .github/workflows/continuous-integration.yaml
- composer.json
- test/unit/ConfigurationTest.php
5616ec5 to
ea2673b
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
common/session/class.SessionManager.php (1)
130-133: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the suppressed unused destructuring variable.
Replace the destructuring assignment with an indexed payload lookup. This removes the PHPMD warning and avoids suppressing malformed-token notices.
Proposed refactor
- /** `@noinspection` PhpUnusedLocalVariableInspection */ - @[$_, $payload] = explode('.', $accessToken); + $parts = explode('.', $accessToken, 3); + $payload = $parts[1] ?? '';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@common/session/class.SessionManager.php` around lines 130 - 133, Update the access-token parsing in SessionManager to replace the suppressed destructuring assignment with an indexed lookup of the payload segment from explode('.'). Preserve the existing fallback for a missing payload when decoding, and remove the PhpUnusedLocalVariableInspection suppression.Source: Linters/SAST tools
test/unit/common/session/SessionManagerTest.php (1)
90-93: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAlign the test name with the behavior it verifies.
testGetCookieBasedSession()does not set a cookie or a PHP-session attribute. It only checks the anonymous fallback when no access token exists. Rename it totestGetAnonymousSession()or initialize cookie/session state and assert the expected session.As per path instructions, tests must exercise the behavior they claim to cover.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/common/session/SessionManagerTest.php` around lines 90 - 93, Rename testGetCookieBasedSession() to testGetAnonymousSession() so the test name accurately describes its existing behavior of verifying the anonymous fallback without cookie or PHP-session state.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@common/session/class.SessionManager.php`:
- Around line 130-133: Update the access-token parsing in SessionManager to
replace the suppressed destructuring assignment with an indexed lookup of the
payload segment from explode('.'). Preserve the existing fallback for a missing
payload when decoding, and remove the PhpUnusedLocalVariableInspection
suppression.
In `@test/unit/common/session/SessionManagerTest.php`:
- Around line 90-93: Rename testGetCookieBasedSession() to
testGetAnonymousSession() so the test name accurately describes its existing
behavior of verifying the anonymous fallback without cookie or PHP-session
state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: https://raw.githubusercontent.com/oat-sa/tao-code-quality/main/coderabbit/php/authoring/v1/.coderabbit.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
Run ID: 93211c19-75e8-4270-b244-adf7220c781c
📒 Files selected for processing (6)
.github/workflows/continuous-integration.yamlcommon/oatbox/user/BasicUser.phpcommon/session/class.SessionManager.phpcomposer.jsontest/unit/ConfigurationTest.phptest/unit/common/session/SessionManagerTest.php
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/continuous-integration.yaml
- composer.json
- test/unit/ConfigurationTest.php
- common/oatbox/user/BasicUser.php
ea2673b to
797e887
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@common/session/class.SessionManager.php`:
- Around line 122-139: Remove the new static helpers
extractAccessTokenFromRequest(), parseAccessToken(), and
buildUserIdentityString() from SessionManager. Move authorization-header access
and token parsing into injected collaborators, and update SessionManager to
consume those dependencies instead of global $_SERVER state or static utility
calls; preserve the existing identity-string behavior through the injected
design.
- Around line 136-161: Align buildUserIdentityString() and
createAccessTokenSession() on one unambiguous identity grammar: ensure the
serialized role#user form is parsed into the role and userId components,
including roles without an embedded '#'. Update the parsing regex and capture
handling accordingly, then add a test covering buildUserIdentityString('role',
'user') and verifying BasicUser receives the expected role and identifier.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: https://raw.githubusercontent.com/oat-sa/tao-code-quality/main/coderabbit/php/authoring/v1/.coderabbit.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
Run ID: d48c113c-c893-425c-ac24-83e616823d70
📒 Files selected for processing (6)
.github/workflows/continuous-integration.yamlcommon/oatbox/user/BasicUser.phpcommon/session/class.SessionManager.phpcomposer.jsontest/unit/ConfigurationTest.phptest/unit/common/session/SessionManagerTest.php
🚧 Files skipped from review as they are similar to previous changes (5)
- .github/workflows/continuous-integration.yaml
- common/oatbox/user/BasicUser.php
- test/unit/common/session/SessionManagerTest.php
- composer.json
- test/unit/ConfigurationTest.php
wazelin
changed the title
wazelin
changed the title
Co-authored-by: Andrei Shapiro <59471572+shpran@users.noreply.github.com> Signed-off-by: Sergei Mikhailov <contact@sergeimikhailov.com>
|
|
||
| public static function extractAccessTokenFromRequest(): string | ||
| { | ||
| $authorizationHeader = explode(' ', $_SERVER['HTTP_AUTHORIZATION'] ?? '', 2); |
There was a problem hiding this comment.
You could use $request = common_http_Request::currentRequest() (from tao/generis/common/http/class.Request.php) to fetch the header using $header = $request->getHeaderValue('Authorization');
Lets don't use globals if not needed.
| * | ||
| * You should have received a copy of the GNU General Public License | ||
| * along with this program; if not, write to the Free Software | ||
| * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. |
| * SPDX-FileCopyrightText: 2013-2026 Open Assessment Technologies S.A. | ||
| * Copyright (C) 2013-2026 (original work) Open Assessment Technologies S.A. | ||
| * | ||
| * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-TAO-Commercial-License |
There was a problem hiding this comment.
This extension is not in a list in OCE-202
There was a problem hiding this comment.
That's weird. How can we skip generis out of this?
Version
There are 0 BREAKING CHANGE, 2 features, 0 fix |
4 participants
Ticket: EP-765
What's Changed
This adds support for User Identity based on a bearer access token
Summary by CodeRabbit
New Features
Compatibility
Tests