-
Notifications
You must be signed in to change notification settings - Fork 19
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
Add API endpoint for retreiving an auth token #575
Conversation
Switch the login process from the web to api controller
@leepeuker I think it might be better to return a HTTP code 200 instead of the redirection to 303, because the clients should decide what to do when the authentication token is returned, not the server. This applies to the official web client as well |
@@ -41,7 +41,6 @@ public function up() : void | |||
) | |||
SQL, | |||
); | |||
$this->execute('INSERT INTO `user_auth_token_tmp` (`id`, `user_id`, `token`, `expiration_date`, `created_at`) SELECT * FROM `user_auth_token`'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there are existing auth tokens the migration will fail, because the new columns device_name and user_agent are required. I think it is best to purge all existing auth tokens during the migration to prevent issues or having to use empty strings as fallback values.
); | ||
} catch (InvalidCredentials $e) { | ||
return Response::createBadRequest(Json::encode([ | ||
'error' => basename(str_replace('\\', '/', get_class($e))), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix: This not really a good idea, because we tell the user exactly what went wrong which we do not want in some cases, e.g. we do not want to tell a bad actor that an email exists but the password is wrong.
I will adjust this to only return a generic Invalid credentials
again and only be a concrete error for ttop issues.
); | ||
} | ||
|
||
return Response::createJson( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
info: the authenication endpoint always returns the same response on success regardless of the client
This PR adds
/authentication/request-token
where an HTTP POST request should be sent.It also adds a new header,
X-Movary-Client
, which allows the backend to distinguish the default Movary frontend from any 3rd-party apps that may want to use Movary's authentication.This is important, because if it's just a 3rd-party, Movary will only return the token in JSON format while if it's the Movary frontend, the backend will return a HTTP 303 response. I stole this idea from the Plex API, which does the same, only with
X-Plex-Product
instead.Additionally, I have added two columns to the
user_auth_tokens
table containing the Device Name (aka the value fromX-Movary-Client
) and the user agent string from the HTTP request. This allows us to add a new feature in the future, where the users can see all the tokens / devices and invalidate any token / authorized sessions. The user can see which token belongs to which app and which user agent was used to request the token. Maybe we should store the IP as well? And maybe another column containing the date that the token was last used.Anyway, the default client name for the Movary frontend is 'Movary Web'.
The login process for the current Twig frontend has already been migrated from the old web endpoint to the API. It no longer works with the POST form, but instead JS is used to send the POST request; this is to allow a smooth transition to a TOTP form if it's necessary.
Also, the verification of the TOTP code has been moved from the
TwoFactorAuthenticationController
to the new APIAuthenticationController
. I did this because now the user can log in with the TOTP code with one HTTP request, instead of two different requests. The 2FA controller is still used for enabling / disabling the 2FA though.Part of #572