Skip to content

Commit 42a714b

Browse files
cconard96cconard96
authored
Fix HL API doc authorization code authentication
1 parent 7412cd1 commit 42a714b

File tree

7 files changed

+146
-67
lines changed

7 files changed

+146
-67
lines changed

‎install/empty_data.php

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,8 @@
3939
use Glpi\RichText\UserMention;
4040
use Glpi\Socket;
4141

42+
use function Safe\json_encode;
43+
4244
// Use anonymous class so we can have constants that define special values without polluting the global table
4345
// and adding unnecessary variables to IDE autocomplete data that may result in errors
4446
$empty_data_builder = new class {
@@ -9522,6 +9524,17 @@ public function getEmptyData(): array
95229524
'is_recursive' => 1,
95239525
'is_dynamic' => 0,
95249526
];
9527+
9528+
$tables['glpi_oauthclients'][] = [
9529+
'name' => 'Test E2E OAuth Client',
9530+
'redirect_uri' => json_encode(["/api.php/oauth2/redirection"]),
9531+
'grants' => json_encode(['authorization_code']),
9532+
'scopes' => json_encode(['api', 'user']),
9533+
'is_active' => 1,
9534+
'is_confidential' => 1,
9535+
'identifier' => '9246d35072ff62193330003a8106d947fafe5ac036d11a51ebc7ca11b9bc135e',
9536+
'secret' => (new GLPIKey())->encrypt('d2c4f3b8a0e1f7b5c6a9d1e4f3b8a0e1f7b5c6a9d1e4f3b8a0e1f7b5c6a9d1'),
9537+
];
95259538
}
95269539

95279540
return $tables;

‎phpunit/functional/Glpi/Api/HL/Controller/CoreControllerTest.php

Lines changed: 0 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -320,55 +320,6 @@ public function testOAuthPasswordGrantHeader()
320320
});
321321
}
322322

323-
public function testOAuthAuthCodeGrant()
324-
{
325-
// Not a complete end to end test. Not sure how that could be done. Should probably be using Cypress.
326-
global $DB;
327-
328-
// Create an OAuth client
329-
$client = new \OAuthClient();
330-
$client_id = $client->add([
331-
'name' => __FUNCTION__,
332-
'is_active' => 1,
333-
'is_confidential' => 1,
334-
]);
335-
$this->assertGreaterThan(0, $client_id);
336-
337-
$client->update([
338-
'id' => $client_id,
339-
'grants' => ['authorization_code'],
340-
'redirect_uri' => ["/api.php/oauth2/redirection"],
341-
]);
342-
343-
// get client ID and secret
344-
$it = $DB->request([
345-
'SELECT' => ['identifier', 'secret', 'redirect_uri'],
346-
'FROM' => \OAuthClient::getTable(),
347-
'WHERE' => ['id' => $client_id],
348-
]);
349-
$this->assertCount(1, $it);
350-
$client_data = $it->current();
351-
352-
// Test authorize endpoint
353-
$request = new Request('GET', '/Authorize', [], null);
354-
$request = $request->withQueryParams([
355-
'response_type' => 'code',
356-
'client_id' => $client_data['identifier'],
357-
'scope' => '',
358-
'redirect_uri' => json_decode($client_data['redirect_uri'])[0],
359-
]);
360-
361-
$this->api->call($request, function ($call) {
362-
/** @var \HLAPICallAsserter $call */
363-
$call->response
364-
->status(fn($status) => $this->assertEquals(302, $status))
365-
->headers(function ($headers) {
366-
global $CFG_GLPI;
367-
$this->assertMatchesRegularExpression('/^' . preg_quote($CFG_GLPI['url_base'], '/') . '\/\?redirect=/', $headers['Location']);
368-
});
369-
});
370-
}
371-
372323
public function testOAuthClientCredentialsGrant(): void
373324
{
374325
global $DB;

‎src/Glpi/Api/HL/Middleware/CookieAuthMiddleware.php

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -41,17 +41,17 @@ class CookieAuthMiddleware extends AbstractMiddleware implements AuthMiddlewareI
4141
{
4242
public function process(MiddlewareInput $input, callable $next): void
4343
{
44-
$auth = new \Auth();
45-
if ($auth->getAlternateAuthSystemsUserLogin(\Auth::COOKIE)) {
46-
// User could be authenticated by a cookie
47-
// Need to use cookies for session and start it manually
48-
ini_set('session.use_cookies', '1');
49-
Session::start();
44+
// User could be authenticated by a cookie
45+
// Need to use cookies for session and start it manually
46+
ini_set('session.use_cookies', '1');
47+
Session::start();
48+
49+
if (($user_id = Session::getLoginUserID()) !== false) {
5050
// unset the response to indicate a successful auth
5151
$input->response = null;
5252
$input->client = [
5353
'client_id' => 'internal', // Internal just means the user was authenticated internally either by cookie or an already existing session.
54-
'users_id' => Session::getLoginUserID(),
54+
'users_id' => $user_id,
5555
'scopes' => [],
5656
];
5757
} else {

‎src/Glpi/Api/HL/Middleware/IPRestrictionRequestMiddleware.php

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,13 +53,14 @@ public function process(MiddlewareInput $input, callable $next): void
5353

5454
$request_ip = $_SERVER['REMOTE_ADDR'];
5555

56-
$allowed_ips = $DB->request([
56+
$result = $DB->request([
5757
'SELECT' => ['allowed_ips'],
5858
'FROM' => 'glpi_oauthclients',
5959
'WHERE' => [
6060
'identifier' => $client['client_id'],
6161
],
62-
])->current()['allowed_ips'];
62+
])->current();
63+
$allowed_ips = $result['allowed_ips'] ?? [];
6364

6465
if (empty($allowed_ips)) {
6566
// No IP restriction

‎src/Glpi/Kernel/Listener/PostBootListener/SessionStart.php

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,17 @@ public static function getSubscribedEvents(): array
6666

6767
public function onPostBoot(): void
6868
{
69+
// Always set the session files path, even when session is not started automatically here.
70+
// It can indeed be started later manually and the path must be correctly set when it is done.
71+
if (Session::canWriteSessionFiles()) {
72+
Session::setPath();
73+
} else {
74+
\trigger_error(
75+
sprintf('Unable to write session files on `%s`.', GLPI_SESSION_DIR),
76+
E_USER_WARNING
77+
);
78+
}
79+
6980
// The session must be started even in CLI context.
7081
// The GLPI code refers to the session in many places
7182
// and we cannot safely remove its initialization in the CLI context.
@@ -120,15 +131,6 @@ public function onPostBoot(): void
120131
}
121132

122133
if ($start_session) {
123-
if (Session::canWriteSessionFiles()) {
124-
Session::setPath();
125-
} else {
126-
\trigger_error(
127-
sprintf('Unable to write session files on `%s`.', GLPI_SESSION_DIR),
128-
E_USER_WARNING
129-
);
130-
}
131-
132134
Session::start();
133135
}
134136
}

‎tests/cypress/e2e/oauth_authcode.cy.js

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
/**
2+
* ---------------------------------------------------------------------
3+
*
4+
* GLPI - Gestionnaire Libre de Parc Informatique
5+
*
6+
* http://glpi-project.org
7+
*
8+
* @copyright 2015-2025 Teclib' and contributors.
9+
* @licence https://www.gnu.org/licenses/gpl-3.0.html
10+
*
11+
* ---------------------------------------------------------------------
12+
*
13+
* LICENSE
14+
*
15+
* This file is part of GLPI.
16+
*
17+
* This program is free software: you can redistribute it and/or modify
18+
* it under the terms of the GNU General Public License as published by
19+
* the Free Software Foundation, either version 3 of the License, or
20+
* (at your option) any later version.
21+
*
22+
* This program is distributed in the hope that it will be useful,
23+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
24+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
25+
* GNU General Public License for more details.
26+
*
27+
* You should have received a copy of the GNU General Public License
28+
* along with this program. If not, see <https://www.gnu.org/licenses/>.
29+
*
30+
* ---------------------------------------------------------------------
31+
*/
32+
33+
describe('OAuth - Authorization Code Grant', () => {
34+
const oauthclient_id = '9246d35072ff62193330003a8106d947fafe5ac036d11a51ebc7ca11b9bc135e';
35+
const oauthclient_secret = 'd2c4f3b8a0e1f7b5c6a9d1e4f3b8a0e1f7b5c6a9d1e4f3b8a0e1f7b5c6a9d1';
36+
37+
function doAuthCodeGrant(expect_already_logged_in = false, remember_me = false) {
38+
function doGLPILogin() {
39+
cy.findByRole('textbox', {'name': "Login"}).type('e2e_tests');
40+
cy.findByLabelText("Password").type('glpi');
41+
if (remember_me) {
42+
cy.findByRole('checkbox', {name: "Remember me"}).check();
43+
} else {
44+
cy.findByRole('checkbox', {name: "Remember me"}).uncheck();
45+
}
46+
cy.getDropdownByLabelText("Login source").selectDropdownValue('GLPI internal database');
47+
cy.findByRole('button', {name: "Sign in"}).click();
48+
}
49+
50+
function doAuthorization() {
51+
// Should be on a page asking the user to approve or reject the authorization request
52+
cy.findByRole('heading', {name: 'Test E2E OAuth Client wants to access your GLPI account'}).should('be.visible');
53+
cy.findByText('Access to the API').should('be.visible');
54+
cy.findByText('Access to the user\'s information').should('be.visible');
55+
cy.findByRole('button', {name: 'Deny'}).should('be.visible');
56+
// Clicking the Accept button would go to a 401 error page, because we didn't give a real redirect URL, which Cypress will have issues with because it isn't an HTML page
57+
// We only care that the redirect URL includes the code parameter
58+
cy.url().then((url) => {
59+
cy.request({
60+
url: `${url}&accept=1`,
61+
failOnStatusCode: false
62+
}).then((response) => {
63+
expect(response.status).to.eq(401);
64+
expect(response.redirects[0]).to.include('code=');
65+
// extract the code from the URL
66+
const code = response.redirects[0].split('code=')[1];
67+
// Request the token now
68+
cy.request({
69+
method: 'POST',
70+
url: '/api.php/token',
71+
body: {
72+
grant_type: 'authorization_code',
73+
client_id: oauthclient_id,
74+
client_secret: oauthclient_secret,
75+
code: code,
76+
redirect_uri: '/api.php/oauth2/redirection'
77+
},
78+
headers: {
79+
'Content-Type': 'application/json'
80+
}
81+
}).then((response) => {
82+
expect(response.status).to.eq(200);
83+
expect(response.body.access_token).to.exist;
84+
expect(response.body.refresh_token).to.exist;
85+
});
86+
});
87+
});
88+
}
89+
90+
cy.visit(`/api.php/Authorize?response_type=code&client_id=${oauthclient_id}&scope=api user&redirect_uri=/api.php/oauth2/redirection`);
91+
if (!expect_already_logged_in) {
92+
doGLPILogin();
93+
}
94+
doAuthorization();
95+
}
96+
97+
it('Should authorize without cookie - no remember me', () => {
98+
doAuthCodeGrant();
99+
});
100+
it('Should authorize without cookie - remember me', () => {
101+
doAuthCodeGrant(false, true);
102+
});
103+
it('Should authorize with cookie', () => {
104+
cy.login();
105+
doAuthCodeGrant(true);
106+
});
107+
});

‎tests/src/Command/TestUpdatedDataCommand.php

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -171,6 +171,11 @@ public function __construct($dbhost, $dbuser, $dbpassword, $dbdefault)
171171
continue;
172172
}
173173

174+
// Ignore e2e oauth client
175+
if ($table_name === 'glpi_oauthclients' && $row_data['name'] === 'Test E2E OAuth Client') {
176+
continue;
177+
}
178+
174179
foreach ($row_data as $key => $value) {
175180
if (in_array($key, $excluded_fields)) {
176181
continue; // Ignore fields that would be subject to legitimate changes

0 commit comments

Comments
 (0)