feat: SSR #5777
feat: SSR #5777
Conversation
app/controllers/api/users.php
Outdated
| App::post('/v1/users/:userId/sessions') | ||
| ->desc('Create session') | ||
| ->groups(['api', 'users']) | ||
| ->label('event', 'users.[userId].sessions.create') |
There was a problem hiding this comment.
This needs to be users.[userId].sessions.[sessionId].create
app/controllers/api/users.php
Outdated
| if ($user !== false && !$user->isEmpty()) { | ||
| $user->setAttributes($user->getArrayCopy()); | ||
| } else { |
There was a problem hiding this comment.
what is the need for this ?
app/controllers/api/users.php
Outdated
| $user->setAttributes([ | ||
| '$id' => $userId, | ||
| '$permissions' => [ | ||
| Permission::read(Role::any()), | ||
| Permission::update(Role::user($userId)), | ||
| Permission::delete(Role::user($userId)), | ||
| ], | ||
| 'email' => null, | ||
| 'emailVerification' => false, | ||
| 'status' => true, | ||
| 'password' => null, | ||
| 'hash' => Auth::DEFAULT_ALGO, | ||
| 'hashOptions' => Auth::DEFAULT_ALGO_OPTIONS, | ||
| 'passwordUpdate' => null, | ||
| 'registration' => DateTime::now(), | ||
| 'reset' => false, | ||
| 'prefs' => new \stdClass(), | ||
| 'sessions' => null, | ||
| 'tokens' => null, | ||
| 'memberships' => null, | ||
| 'search' => implode(' ', [$userId]), | ||
| 'accessedAt' => DateTime::now(), | ||
| ]); | ||
|
|
||
| $user->removeAttribute('$internalId'); | ||
| Authorization::skip(fn () => $dbForProject->createDocument('users', $user)); | ||
| } |
There was a problem hiding this comment.
This is a createSession endpoint right? I see this is performing 2 functions
- Creating a user
- Creating a session for the user
this goes against REST principles where each endpoint should handle only one responsibility unless absolutely necessary.
What is the need to create a user here when we already have the createUser endpoint?
This createSession endpoint should return 404 if the user does not exist
There was a problem hiding this comment.
I agree it seems to violate REST principle, but it offers pontentially better DX, and Matej proposed this change justifying it as the existing behaviour for endpoints in account service.
Should we:
- Auto-create users for endpoints in account service and NOT users service?
- Auto-create users for endpoints in both account and users services?
- Change accounts service so we never auto-create users?
app/controllers/api/users.php
Outdated
| $user->setAttributes([ | ||
| '$id' => $userId, | ||
| '$permissions' => [ | ||
| Permission::read(Role::any()), | ||
| Permission::update(Role::user($userId)), | ||
| Permission::delete(Role::user($userId)), | ||
| ], | ||
| 'email' => null, | ||
| 'emailVerification' => false, | ||
| 'status' => true, | ||
| 'password' => null, | ||
| 'hash' => Auth::DEFAULT_ALGO, | ||
| 'hashOptions' => Auth::DEFAULT_ALGO_OPTIONS, | ||
| 'passwordUpdate' => null, | ||
| 'registration' => DateTime::now(), | ||
| 'reset' => false, | ||
| 'prefs' => new \stdClass(), | ||
| 'sessions' => null, | ||
| 'tokens' => null, | ||
| 'memberships' => null, | ||
| 'search' => implode(' ', [$userId]), | ||
| 'accessedAt' => DateTime::now(), | ||
| ]); |
There was a problem hiding this comment.
same comment as above. The endpoint should only perform one function.
app/controllers/api/users.php
Outdated
| App::post('/v1/users/:userId/tokens') | ||
| ->desc('Create token') | ||
| ->groups(['api', 'users']) | ||
| ->label('event', 'users.[userId].tokens.create') |
There was a problem hiding this comment.
This event needs to be added to the events.php file
There was a problem hiding this comment.
And it should also be users.[userId].tokens.[tokenId].create
app/controllers/shared/api.php
Outdated
| case 'token': | ||
| if (($auths['token'] ?? true) === false) { | ||
| throw new Exception(Exception::USER_AUTH_METHOD_UNSUPPORTED, 'Token authentication is disabled for this project'); | ||
| } | ||
| break; |
There was a problem hiding this comment.
This needs to be added to the auth.php file in the config directory. Else it will break the updateAuthStatus endpoint
There was a problem hiding this comment.
Also what is "token" based auth? How does it sit well in this screen?
I am to understand that tokens are created by emailPassword, phone, magic URL etc. So if we add a new check box for token here, what does it mean to disable token based auth?
does it mean, we also disable emailPassword, phone, magicURL etc? Because a token is generated by all those methods right?
There was a problem hiding this comment.
Good point, I think we should remove these. It doesn't fit in this screen.
The user can already control where a token is created with the existed toggles.
If we let them disable token auth it would break magic url and phone auth.
| @@ -42,7 +42,7 @@ | |||
| 'name' => 'Phone', | |||
| 'key' => 'phone', | |||
| 'icon' => '/images/users/phone.png', | |||
| 'docs' => 'https://appwrite.io/docs/client/account?sdk=web-default#accountCreatePhoneSession', | |||
| 'docs' => 'https://appwrite.io/docs/references/cloud/client-web/account#accountCreatePhoneToken', | |||
| 'enabled' => true, | |||
| ], | |||
There was a problem hiding this comment.
we're missing a new array item for token
app/config/scopes.php
Outdated
| 'account' => [ | ||
| 'description' => 'Access to make actions on behalf of a user account', | ||
| ], |
There was a problem hiding this comment.
Can you give me some reasons why this is not account.read and account.write
There was a problem hiding this comment.
There was an existing hidden scope called account.
I wanted keep this name to reduce the scope of the changes.
app/config/scopes.php
Outdated
| 'account' => [ | ||
| 'description' => 'Access to make actions on behalf of a user account', | ||
| ], | ||
| 'sessions' => [ |
There was a problem hiding this comment.
similarly why is this not sessions.write ?
What does this PR do?
Session Secrets for API key authenticated clients
secretfield to the Session response object when authenticated using API key, including the response of all existing Account session creation endpoints.sessionsscope, used for session creation endpoints. It is added available to guests by default and will be optional for API key.Tokens
Create sessionin users servicePOST /users/:userId/sessionCreate tokenin users servicePOST /users/:userId/tokenCreate session [from token]in account servicePOST /account/sessions/tokenPUT /account/sessions/phonetoPOST /account/sessions/tokenPUT /account/sessions/magic-urltoPOST /account/sessions/tokenToken for SSR OAuth
userId&secretfrom a generated token to the final OAuth2 redirect to allow SSR clients to exchange this for a session.Test Plan
Related PRs and Issues
RFC
Checklist