8000 Separate OAuth2 info from Sessions into Identities by stnguyen90 · Pull Request #5953 · appwrite/appwrite · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Separate OAuth2 info from Sessions into Identities #5953

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Aug 9, 2023

Conversation

stnguyen90
Copy link
Contributor
@stnguyen90 stnguyen90 commented Aug 7, 2023

What does this PR do?

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.

This PR was created to target cl-1.4.x.

In addition, this PR adds a secrets attribute to the identities collection. These secrets can be used to store data from the provider that may or may not be sensitive.

For example, this will be used by the migration API when connecting to Firebase to store the service account used for the migration.

This data will only be used internally inside Appwrite and not exposed to an end user or developer.

Data here is tied to the user identity so that it can be removed when the user or the user identity is removed.

Test Plan

Manual

Related PRs and Issues

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?

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).
Until we have a clearer picture of why we need it, it would be best to
remove it since it's easier to add it later than to remove it after it's
released.
This will allow developers to set up a job to find expired access tokens
so they can refresh them.
@stnguyen90 stnguyen90 force-pushed the feat-user-identities-cl-1.4.x branch from 84f2df6 to f3fa792 Compare August 8, 2023 15:34
@stnguyen90 stnguyen90 marked this pull request as ready for review August 8, 2023 16:56
These secrets can be used to store data from the provider that may or
may not be sensitive.

For example, this will be used by the migration API when connecting to
Firebase to store the service account used for the migration.

This data will only be used internally inside Appwrite and not exposed
to an end user or developer.
10000
@@ -0,0 +1 @@
Get currently logged in user list of identities.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Get currently logged in user list of identities.
Get the list of identities for the currently logged in user.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I matched the convention of some of the other descriptions like list-sessions.md so I'll update that too like:

-Get currently logged in user list of active sessions across different devices.
+Get the list of active sessions across different devices for the currently logged in user.

and how about these two?

account/get-prefs.md:

-Get currently logged in user preferences as a key-value object.
+Get the preferences as a key-value object for the currently logged in user

account/get.md:

-Get currently logged in user data as JSON object.
+Get currently logged in user.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeap lets do it 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated!

Copy link
Member
@christyjacob4 christyjacob4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets also implement your recommendations for the descriptions

@Meldiron
Copy link
Contributor
Meldiron commented Aug 9, 2023

This has been merged into feat-git-inregration.
We can close this PR if we merge that one

@stnguyen90
Copy link
Contributor Author

This has been merged into feat-git-inregration. We can close this PR if we merge that one

@Meldiron, but when will #5725 be merged considering this is needed for #5938?

@christyjacob4 christyjacob4 merged commit 1f2a3c0 into cl-1.4.x Aug 9, 2023
@stnguyen90 stnguyen90 deleted the feat-user-identities-cl-1.4.x branch August 9, 2023 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0