8000 Create RFC for Identities by stnguyen90 · Pull Request #57 · appwrite/rfc · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Create RFC for Identities #57

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Create RFC for Identities #57

wants to merge 1 commit into from

Conversation

stnguyen90
Copy link
@stnguyen90 stnguyen90 commented May 16, 2023 •
edited
Loading

Rendered: https://github.com/appwrite/rfc/blob/feat-023-connected-accounts/023-identities/README.md

What does this PR do?

Proposal for separating OAuth2 from Sessions

Related PRs and Issues

TBD

Have you read the Contributing Guidelines on issues?

Yes

@stnguyen90 stnguyen90 force-pushed the feat-023-connected-accounts branch 3 times, most recently from 645773c to a0a15fe Compare May 17, 2023 17:45
@stnguyen90 stnguyen90 mentioned this pull request May 18, 2023
Closed
2 tasks
@stnguyen90 stnguyen90 force-pushed the feat-023-connected-accounts branch from a0a15fe to b51e46b Compare May 24, 2023 15:50
@stnguyen90 stnguyen90 marked this pull request as ready for review June 23, 2023 12:38
eldadfux
eldadfux requested changes Jun 24, 2023
View reviewed changes
Copy link
Member
@eldadfux eldadfux left a comment

Choose a reason for hiding this comment

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

Steven, great work, super interesting feature. I left a few comments, thoughts, and questions.

023-connected-accounts/README.md Outdated

##### List Connected Accounts

**GET /v1/account/connected**
Copy link
Member

Choose a reason for hiding this comment

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

Connected accounts is very misleading. When I first heard of it I thought it was about connecting 2 existing Appwrite accounts and allowing to switch between them. This features is actually just refactoring of how we handle OAuth2.

023-connected-accounts/README.md Outdated

##### List Connected Accounts

**GET /v1/account/connected**
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 /v1/account/connected**
**GET /v1/account/oauth2/connections**

Connected accounts is very misleading. When I first heard of it I thought it was about connecting 2 existing Appwrite accounts and allowing to switch between them. This features is actually just refactoring of how we handle OAuth2. Also, the connected is a past tense verb where a REST API should be represented by nouns.

Copy link
Author

Choose a reason for hiding this comment

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

Wouldn't it be useful to make this flexible so that we could support other connections? Maybe multiple phone numbers or linking multiple emails? Merging Appwrite users can also be helpful.

023-connected-accounts/README.md Outdated

##### Get a Connected Account

**GET /v1/account/connected/{id}**
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 /v1/account/connected/{id}**
**GET /v1/account/oauth/connections/{id}**

023-connected-accounts/README.md Outdated

##### Connecting to an OAuth2 provider

1. User makes authenticated API call to `GET /v1/account/sessions/oauth2/{provider}`
Copy link
Member

Choose a reason for hiding this comment

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

Is it safe to assume that the main difference here from the existing flow is that if a user is already logged in with Appwrite when creating an OAuth session the OAuth connection will be attached to their current account regardless of the email?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, during the /redirect.

023-connected-accounts/README.md Outdated
If possible, explain what actions we can take to avoid that.
-->

The existing OAuth2 APIs will still work as they do now. The only difference is a connected account will be created if one does not exist.
Copy link
Member

Choose a reason for hiding this comment

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

This is a better place in the RFC to discuss data migrations due to changes in data structure.

Copy link
Author

Choose a reason for hiding this comment

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

Added a line.

023-connected-accounts/README.md Outdated

<!-- Write your answer below. -->

1. How to connect an account on mobile?
Copy link
Member

Choose a reason for hiding this comment

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

What is the main challenge? Passing the current use session token to the webview?

Copy link
Member

Choose a reason for hiding this comment

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

If yes, we could consider another form of short living token with limited access rights. We have similar problems with file previews and other resources as well.

023-connected-accounts/README.md Outdated
<!-- Write your answer below. -->

1. How to connect an account on mobile?
2. Should we support a way to get all connected accounts from a particular provider?
Copy link
Member

Choose a reason for hiding this comment

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

We can do this using the list endpoint queries.

023-connected-accounts/README.md Outdated

1. How to connect an account on mobile?
2. Should we support a way to get all connected accounts from a particular provider?
3. What to do after a connected accounts refresh token no longer works?
Copy link
Member

Choose a reason for hiding this comment

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

I would expect maybe something 401, which can notify the developer they need to re-authenticate the OAuth session to obtain a new access token. How often does this happen today for the different providers? how long would an average access token last before expiration?

Copy link
Author

Choose a reason for hiding this comment

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

I would expect maybe something 401

You mean if the user is using an OAuth2 session? This means we would need to check the refresh token on each request or something.

In addition, we may need to check the tokens on each list connected accounts API call so that we properly return the correct status.

023-connected-accounts/README.md Outdated
1. How to connect an account on mobile?
2. Should we support a way to get all connected accounts from a particular provider?
3. What to do after a connected accounts refresh token no longer works?
4. What should happen to related sessions when a connected account is deleted?
Copy link
Member

Choose a reason for hiding this comment

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

I would leave this unattached. If dev wish to take action here, they can delete relevant sessions with existing APIs. I wouldn't be opinionated and decide that deleting an account is necessarily connected with a security concern, this is not always the case.

023-connected-accounts/README.md Outdated
2. Should we support a way to get all connected accounts from a particular provider?
3. What to do after a connected accounts refresh token no longer works?
4. What should happen to related sessions when a connected account is deleted?
5. What should happen to a connected account when the related session is deleted?
Copy link
Member

Choose a reason for hiding this comment

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

Nothing. You can logout, it doesn't imply you want your connection removed. If developers want for any reason to tie the two together they can do it using the two relevant APIs.

@stnguyen90 stnguyen90 force-pushed the feat-023-connected-accounts branch from b51e46b to d82c2b2 Compare June 29, 2023 02:23
@stnguyen90 stnguyen90 requested a review from eldadfux June 29, 2023 02:24
@stnguyen90 stnguyen90 force-pushed the feat-023-connected-accounts branch from d82c2b2 to 19579b8 Compare July 13, 2023 23:26
@stnguyen90
Create RFC for identities
400229a 
Allow users to connect multiple accounts to their Appwrite account
regardless of the email address in the external system.
@stnguyen90 stnguyen90 force-pushed the feat-023-connected-accounts branch from 19579b8 to 400229a Compare July 13, 2023 23:47
@stnguyen90 stnguyen90 changed the title DRAFT: Create RFC for Connected Accounts DRAFT: Create RFC for Identities Jul 13, 2023
@stnguyen90 stnguyen90 changed the title DRAFT: Create RFC for Identities Create RFC for Identities Jul 20, 2023
@stnguyen90 stnguyen90 marked this pull request as draft October 5, 2023 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TorstenDittmann TorstenDittmann Awaiting requested review from TorstenDittmann

@eldadfux eldadfux Awaiting requested review from eldadfux

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@stnguyen90 @eldadfux
0