Separate OAuth2 data out of Sessions and into User Identities #5551

stnguyen90 · 2023-05-18T01:12:18Z

What does this PR do?

(Provide a description of what this PR does and why it's needed.)

Test Plan

Manual

New User - New Session - No Matching Identities

New User - New Session - Matching provider identity

No new user or identity created. Only new session.

Before:

After:

New Session - Matching Another User's Email Identity

If there is an identity with the same email already, there should be an error:

Before:

Error:

Existing User - No Identities - New Session

This should create a new session successfully for backwards compatibility on mobile platforms where we still need to make sure an existing session is used.

Before:

After:

Existing (matching) User - No Identies - Existing Session

This should create a new identity:

Before:

After:

Existing (non-matching) User - No Identities - New Session

Trying to connect an identity where providerEmail matches another user should result in error.

Before:

Error:

Deleting a User Deletes the Identities

Delete job deleted 64b1c0422411984aa897:

Which was the identity:

Related PRs and Issues

Create RFC for Identities rfc#57

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?

app/config/collections.php

eldadfux · 2023-07-17T21:29:42Z

app/controllers/api/users.php

@@ -47,6 +47,14 @@ function createUser(string $hash, mixed $hashOptions, string $userId, ?string $e
        $email = \strtolower($email);
    }

+    // Makes sure this email is not already used in another identity
+    $identityWithMatchingEmail = $dbForProject->findOne('identities', [
+        Query::equal('providerEmail', [$email]),


Are we making sure to attach only verified emails? Otherwise anyone could fake an email in a 3rd party provider and gain access to it on Appwrite.

We're actually not really checking verified at all today because not all providers verify. Notice how $isVerified isn't used anywhere:

appwrite/app/controllers/api/account.php

Line 491 in c8689d1

$isVerified = $oauth2->isEmailVerified($accessToken);

eldadfux · 2023-07-17T21:30:39Z

src/Appwrite/Auth/OAuth2.php

        \curl_close($ch);

+        if ($code != 200) {
+            $dump = function($value) {


Reminder, remove debugs.

eldadfux · 2023-07-17T21:40:50Z

app/controllers/api/account.php

@@ -98,6 +99,14 @@
            }
        }

+        // Makes sure this email is not already used in another identity
+        $identityWithMatchingEmail = $dbForProject->findOne('identities', [


Should a user be able to log in with an email associated solely with the provider that is different than the main email?

Are we blocking users to attach OAuth providers with emails associated to other users?

Should a user be able to log in with an email associated solely with the provider that is different than the main email?

Using OAuth2, If the email is tied to an existing user via an identity, they won't be able to log in. That's what this check does:

appwrite/app/controllers/api/account.php

Lines 564 to 570 in 8fe688a

// Makes sure this email is not already used in another identity

$identityWithMatchingEmail = $dbForProject->findOne('identities', [

Query::equal('providerEmail', [$email]),

]);

if ($identityWithMatchingEmail !== false && !$identityWithMatchingEmail->isEmpty()) {

$failureRedirect(Exception::USER_EMAIL_ALREADY_EXISTS);

}

Using OAuth2, If the email is not associated with an existing identity or user, a new user will be created.

Using Create Email Session, a user must use their main user email and cannot use an identity email.

Are we blocking users to attach OAuth providers with emails associated to other users?

Yes, here:

appwrite/app/controllers/api/account.php

Lines 500 to 511 in 8fe688a

// Check if this identity is connected to a different user

if (!$user->isEmpty()) {

$userId = $user->getId();

$identitiesWithMatchingEmail = $dbForProject->find('identities', [

Query::equal('providerEmail', [$email]),

Query::notEqual('userId', $userId),

]);

if (!empty($identitiesWithMatchingEmail)) {

throw new Exception(Exception::USER_ALREADY_EXISTS);

}

}

eldadfux · 2023-07-17T21:42:25Z

app/workers/deletes.php

+
+        // Delete identities
+        $this->deleteByGroup('identities', [
+            Query::equal('userId', [$userId])


Use internalId for the deletion of child documents.

eldadfux · 2023-07-17T21:43:03Z

src/Appwrite/Auth/OAuth2/Exception.php

+
+    public function getError() : string
+    {
+        return $this->error;


Why aren't we using the native getMessage interface?

This is the error from the oauth2 provider. The message will include error and error_description.

See https://datatracker.ietf.org/doc/html/rfc6749#section-5.2

fogelito · 2023-07-18T11:53:34Z

app/config/collections.php

+                '$id' => ID::custom('_key_provider_providerUid'),
+                'type' => Database::INDEX_UNIQUE,
+                'attributes' => ['provider', 'providerUid'],
+                'lengths' => [100, 100],


Provider size is 128 , and providerUid is 2048 (does it have to be so long?)
So will do :
'lengths' => [128, 640],
Since both together is the max of 768

and providerUid is 2048 (does it have to be so long?)

providerUid in the sessions collection is 2048, so I wanted to make it consistent. Is that okay?

fogelito · 2023-07-18T11:56:40Z

app/controllers/api/account.php

+            $userId = $user->getId();
+
+            $identitiesWithMatchingEmail = $dbForProject->find('identities', [
+                Query::equal('providerEmail', [$email]),


I do not see an index for providerEmail

fogelito · 2023-07-18T12:03:06Z

app/controllers/api/account.php

+            $failureRedirect(Exception::USER_BLOCKED); // User is in status blocked
+        }
+
+        $identity = $dbForProject->findOne('identities', [


To the existing userInternalId index , you can add these 2 attributes: provider, providerUid
['userInternalId', 'provider', 'providerUid']

You're suggesting to make sure there's an index on this query (userInternalId, provider, and providerUid)?

yes , since there is a unique index on [provider,providerUid],
I think adding an additional index on the above will be good

fogelito · 2023-07-18T12:06:11Z

app/controllers/api/account.php

+
+        $filterQueries = Query::groupByType($queries)['filters'];
+
+        $results = $dbForProject->find('identities', $queries);


Since $queries are dynamic, will have to add indexes for the most common attributes, that will be queried for

This allows us to retain the OAuth2 info even if the session is deleted. This also provides a foundation for allowing multiple emails, phone numbers, etc, not from an OAuth2 provider.

Setting a password can cause problems with other APIs that expect the password to be null. In addition, it doesn't match the implementation for the other APIs that create a user without a password (Create Magic URL Session, Create Phone Session, Create Anonymous Session, etc).

Update the OAuth2 class to throw an exception if an API call to the OAuth2 provider fails and update the endpoint to redirect to the failure url with the information from the OAuth2 provider.

Prevent missing project ID error.

stnguyen90 marked this pull request as draft May 18, 2023 01:12

stnguyen90 changed the base branch from master to 1.4.x May 18, 2023 01:12

stnguyen90 force-pushed the feat-connected-accounts branch from ecde406 to 1383ce9 Compare June 23, 2023 21:17

stnguyen90 force-pushed the feat-connected-accounts branch 2 times, most recently from 76ab919 to 345c70c Compare July 15, 2023 00:29

eldadfux reviewed Jul 17, 2023

View reviewed changes

stnguyen90 force-pushed the feat-connected-accounts branch from 345c70c to 66376de Compare July 17, 2023 21:44

stnguyen90 requested a review from fogelito July 17, 2023 21:44

stnguyen90 force-pushed the feat-connected-accounts branch 5 times, most recently from 434da25 to a906f23 Compare July 17, 2023 22:21

stnguyen90 requested a review from eldadfux July 17, 2023 22:22

fogelito reviewed Jul 18, 2023

View reviewed changes

stnguyen90 force-pushed the feat-connected-accounts branch from a906f23 to 5a52151 Compare July 20, 2023 05:04

stnguyen90 requested a review from fogelito July 20, 2023 05:04

stnguyen90 force-pushed the feat-connected-accounts branch from 5a52151 to 4cdbd65 Compare July 20, 2023 22:02

stnguyen90 marked this pull request as ready for review July 21, 2023 21:58

stnguyen90 changed the title ~~Feat connected accounts~~ Separate OAuth2 data out of Sessions and into User Identities Jul 22, 2023

stnguyen90 added 7 commits July 24, 2023 18:43

Separate OAuth2 info from Sessions into Identities

2442cc4

This allows us to retain the OAuth2 info even if the session is deleted. This also provides a foundation for allowing multiple emails, phone numbers, etc, not from an OAuth2 provider.

Ensure a user's identities are deleted when user is deleted

21bea8e

Fix typing issues in OAuth2

e9c2786

Create OAuth2 exception class

2bced2e

Improve OAuth2 Error Handling

c3636b3

Update the OAuth2 class to throw an exception if an API call to the OAuth2 provider fails and update the endpoint to redirect to the failure url with the information from the OAuth2 provider.

Remove the OAuth2 Callback endpoints from api group

2e07ac1

Prevent missing project ID error.

stnguyen90 force-pushed the feat-connected-accounts branch from 4cdbd65 to 2e07ac1 Compare July 25, 2023 01:44

stnguyen90 marked this pull request as draft July 28, 2023 00:41

This was referenced Jul 31, 2023

Separate OAuth2 info from Sessions into Identities #5919

Closed

Separate OAuth2 info from Sessions into Identities #5921

Merged

Separate OAuth2 info from Sessions into Identities #5920

Closed

stnguyen90 changed 9A33 the base branch from 1.4.x to cl-1.4.x August 7, 2023 15:49

stnguyen90 mentioned this pull request Aug 7, 2023

Separate OAuth2 info from Sessions into Identities #5953

Merged

2 tasks

stnguyen90 closed this Aug 7, 2023

github-actions bot mentioned this pull request Sep 1, 2023

PRs Closed in August 2023 appwrite/.github#70

Closed

stnguyen90 deleted the feat-connected-accounts branch September 10, 2023 16:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Separate OAuth2 data out of Sessions and into User Identities #5551

Separate OAuth2 data out of Sessions and into User Identities #5551

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

	// Makes sure this email is not already used in another identity
	$identityWithMatchingEmail = $dbForProject->findOne('identities', [
	Query::equal('providerEmail', [$email]),
	]);
	if ($identityWithMatchingEmail !== false && !$identityWithMatchingEmail->isEmpty()) {
	$failureRedirect(Exception::USER_EMAIL_ALREADY_EXISTS);
	}

	// Check if this identity is connected to a different user
	if (!$user->isEmpty()) {
	$userId = $user->getId();

	$identitiesWithMatchingEmail = $dbForProject->find('identities', [
	Query::equal('providerEmail', [$email]),
	Query::notEqual('userId', $userId),
	]);
	if (!empty($identitiesWithMatchingEmail)) {
	throw new Exception(Exception::USER_ALREADY_EXISTS);
	}
	}


		$filterQueries = Query::groupByType($queries)['filters'];

		$results = $dbForProject->find('identities', $queries);

Separate OAuth2 data out of Sessions and into User Identities #5551

Separate OAuth2 data out of Sessions and into User Identities #5551

Uh oh!

Conversation

Uh oh!

What does this PR do?

Test Plan

Manual

New User - New Session - No Matching Identities

New User - New Session - Matching provider identity

New Session - Matching Another User's Email Identity

Existing User - No Identities - New Session

Existing (matching) User - No Identies - Existing Session

Existing (non-matching) User - No Identities - New Session

Deleting a User Deletes the Identities

Related PRs and Issues

Checklist

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh! There was an error while loading. Please reload this page.

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

There was an error while loading. Please reload this page.