Skip session secret validation and add session expiry check in case of JWT authentication #8313

Souptik2001 · 2024-06-24T09:23:53Z

What does this PR do?

As per mentioned in the attached issue, when using JWT as an authentication method, if we don't pass the session in the "get session" endpoint, then the authentication fails.
But that shouldn't be the case ideally, as JWT is meant for authentication and it should be able to authenticate even without the need of session. Or more specifically it should include the session data in itself, which should be used for authentication.

The reason why the authentication is failing because the Auth::$secret value which should be equals to the $session['secret'], where $session is the decoded value of the client session string.
But when the client session string is not passed, then the Auth::$secret value is not populated, and that's why later where we check if the hash of Auth::$secret value is equal to the secret of the matched session object, it is failing.
Therefore our target is to populate Auth::$secret value when using JWT. But that is only possible when we get the decoded value of the client session string as mentioned in the second point.
So, to get that data we have to store that data in the JWT and then we can easily get that data.
Now to store that data in JWT, we have to get the client session data in the accounts/jwt handler function. But there is not resource currently present which provides that data.
So, we have to create a new resource called clientSession in app/init.php and now consume that resource in the above mentioned handler function and pass the data to the JWT.

Fixes #8174

Test Plan

We can test this in Postman -

Create a new session usinghttp://localhost/v1/account/sessions/email endpoint.
- It will automatically set the cookies. If not then in the next step you have to pass the session value using x-appwrite-session header.
Now create a new JWT using this endpoint - http://localhost/v1/account/jwt.
- Copy the JWT.
Now create a request to the get session endpoint - http://localhost/v1/account/sessions/current, with the JWT set and also the session set.
- It should work.
Now delete the cookies (session). If you were using header, then remove the header.
Now request again and it should still work (this step should have been failed earlier without this PR changes)

Related PRs and Issues

🐛 Bug Report: Unable to get current user session on functions using jwt #8174

Checklist

Have you read the Contributing Guidelines on issues?
If the PR includes a change to an API's metadata (desc, label, params, etc.), does it also include updated API specs and example docs?

… JWT Signed-off-by: Souptik Datta <souptikdatta2001@gmail.com>

stnguyen90 · 2024-06-24T19:22:28Z

app/init.php

+App::setResource('clientSession', function (string $mode, Document $project, Document $console, Appwrite\Utopia\Request $request) {
+    Auth::setCookieName('a_session_' . $project->getId());
+
+    if (APP_MODE_ADMIN === $mode) {
+        Auth::setCookieName('a_session_' . $console->getId());
+    }
+
+    $session = Auth::decodeSession(
+        $request->getCookie(
+            Auth::$cookieName, // Get sessions
+            $request->getCookie(Auth::$cookieName . '_legacy', '')
+        )
+    );
+
+    // Get session from header for SSR clients
+    if (empty($session['id']) && empty($session['secret'])) {
+        $sessionHeader = $request->getHeader('x-appwrite-session', '');
+
+        if (!empty($sessionHeader)) {
+            $session = Auth::decodeSession($sessionHeader);
+        }
+    }
+
+    if (empty($session['id']) && empty($session['secret'])) {
+        $fallback = $request->getHeader('x-fallback-cookies', '');
+        $fallback = \json_decode($fallback, true);
+        $session = Auth::decodeSession(((isset($fallback[Auth::$cookieName])) ? $fallback[Auth::$cookieName] : ''));
+    }
+
+    return $session;
+}, ['mode', 'project', 'console', 'request']);
+


Can we avoid setting an additional resource considering we already have the user resource?

Also, we have a session resource below this.

stnguyen90 · 2024-06-24T19:27:00Z

app/controllers/api/account.php

@@ -2386,6 +2387,7 @@
                // 'iss'    => 'http://api.mysite.com',
                'userId' => $user->getId(),
                'sessionId' => $current->getId(),
+                'session' => json_encode($clientSession),


I'd prefer to avoid adding this session.

stnguyen90

I think we need to rely a little less on the session secret. We should validate once and then rely on the Auth::$unique, instead. Relying on Auth::$secret requires us to iterate over session and hash, which can be expensive.

Please also add a test case to verify calling get current session works with a JWT.

Signed-off-by: Souptik Datta <souptikdatta2001@gmail.com>

tests/e2e/Services/Account/AccountCustomClientTest.php

Souptik2001 · 2024-06-25T05:15:08Z

Thanks @stnguyen90! Got your point. 👍 It makes sense to use Auth::$secret at only one place instead of multiple places.

Therefore I have removed Auth::$secret check from all other places except the user resource callback. User resource will be the source of truth and the check will be done over there.

But the Auth::$secret was used at all other places to get the session ID, but now as we are not relying on Auth::$secret therefore we have to pass the session ID to be consumed at other places.
Therefore I have added another static variable in the Auth class called - $sessionId. And this will be set to the current session ID in the user resource callback function.

And now this Auth::$sessionId is used everywhere else.

How does this look now?

I have also added the test case. 👍

Signed-off-by: Souptik Datta <souptikdatta2001@gmail.com>

Souptik2001 · 2024-06-25T18:45:08Z

app/init.php

-        if (empty($user->find('$id', $jwtSessionId, 'sessions'))) { // Match JWT to active token
+        $session = $user->find('$id', $jwtSessionId, 'sessions');
+
+        if (empty($session) || Auth::isSessionExpired($session)) { // Match JWT to active token


@stnguyen90 This missing time check was the cause for this #8000 issue. Therefore added this session expired check over here. That should be also fixed now, as per my tests. 🤞

Just FYI - For JWT we still don't have a secret check like we have in sessions, that's because we don't have that data in JWT. But I think that's the point of directly storing the userID, so that we don't have to depend on the session secret, right?

Just FYI - For JWT we still don't have a secret check like we have in sessions, that's because we don't have that data in JWT. But I think that's the point of directly storing the userID, so that we don't have to depend on the session secret, right?

Yes. We just need to check the JWT is valid ($jwt->decode()) and if it is, trust the value is valid.

Souptik2001 · 2024-06-25T18:48:20Z

tests/e2e/Services/Account/AccountCustomClientTest.php

+        ]));
+
+        $this->assertEquals(200, $response['headers']['status-code']);
+


@stnguyen90 I was thinking to add a test for checking the expiry time condition for JWT to validate the changes of #8000 I did above.

Should we add that? If yes, is there any recommended way here to change the default expiry time, or use some other session creation endpoint which allows setting the expiry time?

Can you make an API call to update the project auth settings and change the session expiry before creating the session?

Otherwise, you can try doing a manual test and add screenshot(s) as evidence.

@stnguyen90 Thanks for the idea of calling project auth API to change the expiry time!

Did that, and added the test to test if JWT properly respects session expiry time!

stnguyen90 · 2024-06-26T23:43:23Z

app/init.php

-        if (empty($user->find('$id', $jwtSessionId, 'sessions'))) { // Match JWT to active token
+        $session = $user->find('$id', $jwtSessionId, 'sessions');
+
+        if (empty($session) || Auth::isSessionExpired($session)) { // Match JWT to active token


Just FYI - For JWT we still don't have a secret check like we have in sessions, that's because we don't have that data in JWT. But I think that's the point of directly storing the userID, so that we don't have to depend on the session secret, right?

Yes. We just need to check the JWT is valid ($jwt->decode()) and if it is, trust the value is valid.

stnguyen90 · 2024-06-26T23:44:36Z

tests/e2e/Services/Account/AccountCustomClientTest.php

+        ]));
+
+        $this->assertEquals(200, $response['headers']['status-code']);
+


Can you make an API call to update the project auth settings and change the session expiry before creating the session?

Otherwise, you can try doing a manual test and add screenshot(s) as evidence.

stnguyen90 · 2024-06-26T23:47:03Z

src/Appwrite/Auth/Auth.php

+    /**
+     * User session ID.
+     *
+     * @var string
+     */
+    public static $sessionId = '';
+


Oye..I didn't realize we don't have this yet...

@TorstenDittmann, is this okay so that we:

prevent finding the session using the secret multiple times as it can be expensive to iterate over every session and verify the secret hash

not rely on a non-existent session secret when a JWT is provided.

Signed-off-by: Souptik Datta <souptikdatta2001@gmail.com>

fix: Pass session data in JWT and use that to authenticate when using…

4c229aa

… JWT Signed-off-by: Souptik Datta <souptikdatta2001@gmail.com>

stnguyen90 self-requested a review June 24, 2024 19:20

stnguyen90 requested changes Jun 24, 2024

View reviewed changes

Souptik2001 added 3 commits June 25, 2024 10:13

fix: Eliminate use of session secret for verification multiple times

3fbf271

Signed-off-by: Souptik Datta <souptikdatta2001@gmail.com>

fix: Fix session ID verification condition

367ca92

Signed-off-by: Souptik Datta <souptikdatta2001@gmail.com>

feat: Add test case for verifying current session fetch using JWT

d9f40bc

Signed-off-by: Souptik Datta <souptikdatta2001@gmail.com>

Souptik2001 commented Jun 25, 2024

View reviewed changes

tests/e2e/Services/Account/AccountCustomClientTest.php Show resolved Hide resolved

Souptik2001 added 3 commits June 25, 2024 21:43

Merge branch 'master' into fix-8174-pass-session-data-in-jwt

513bb47

feat: Add session expiry time check for JWT

1602a7b

Signed-off-by: Souptik Datta <souptikdatta2001@gmail.com>

chore: Add test case for isSessionExpired function

382cc4b

Signed-off-by: Souptik Datta <souptikdatta2001@gmail.com>

Souptik2001 commented Jun 25, 2024

View reviewed changes

Souptik2001 mentioned this pull request Jun 25, 2024

🐛 Bug Report: JWT still valid after Session Timeout #8000

Open

2 tasks

Souptik2001 requested a review from stnguyen90 June 26, 2024 04:43

Souptik2001 changed the title ~~Pass session data in JWT and use that to authenticate when using JWT~~ Skip session secret validation and add session expiry check in case of JWT authentication Jun 26, 2024

stnguyen90 reviewed Jun 26, 2024

View reviewed changes

chore: Add test case to test if jwt respects expiry time

ec18bb4

Signed-off-by: Souptik Datta <souptikdatta2001@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip session secret validation and add session expiry check in case of JWT authentication #8313

Skip session secret validation and add session expiry check in case of JWT authentication #8313

Souptik2001 commented Jun 24, 2024 •

edited by stnguyen90

Loading

stnguyen90 Jun 24, 2024

stnguyen90 Jun 24, 2024

stnguyen90 left a comment

Souptik2001 commented Jun 25, 2024

Souptik2001 Jun 25, 2024

stnguyen90 Jun 26, 2024

Souptik2001 Jun 25, 2024

stnguyen90 Jun 26, 2024

Souptik2001 Jun 27, 2024

stnguyen90 Jun 26, 2024

stnguyen90 Jun 26, 2024

stnguyen90 Jun 26, 2024

		]));

		$this->assertEquals(200, $response['headers']['status-code']);

Skip session secret validation and add session expiry check in case of JWT authentication #8313

Are you sure you want to change the base?

Skip session secret validation and add session expiry check in case of JWT authentication #8313

Conversation

Souptik2001 commented Jun 24, 2024 • edited by stnguyen90 Loading

What does this PR do?

Test Plan

Related PRs and Issues

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stnguyen90 left a comment

Choose a reason for hiding this comment

Souptik2001 commented Jun 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Souptik2001 commented Jun 24, 2024 •

edited by stnguyen90

Loading