Login with magic link #289
Conversation
duckth
force-pushed
the
feature/magic-link-login
branch
from
fcc8943 to
b7cc783
Compare
duckth
force-pushed
the
feature/magic-link-login
branch
from
2ec47d1 to
625f332
Compare
duckth
force-pushed
the
feature/magic-link-login
branch
from
d62fe22 to
689b2f5
Compare
A-Guldborg
force-pushed
the
feature/magic-link-login
branch
from
330ed37 to
874dada
Compare
There was a problem hiding this comment.
In addition to the specific comments, I would like the tests to use the builders instead, as well as the baseUnitTest class, Integration test @A-Guldborg ?
| @@ -11,6 +11,8 @@ public class EnvironmentSettings : IValidatable | |||
|
|
|||
| [Required] public string DeploymentUrl { get; set; } | |||
|
|
|||
| [Required] public string ShiftyUrl { get; set; } | |||
There was a problem hiding this comment.
You will also need to update the infra files for this to work as intended
There was a problem hiding this comment.
@duckth can you help with this part? I am not quite sure how the deployment works :)
coffeecard/CoffeeCard.Models/DataTransferObjects/v2/User/UserLoginRequest.cs
Show resolved
Hide resolved
coffeecard/CoffeeCard.Models/DataTransferObjects/v2/User/UserLoginResponse.cs
Show resolved
Hide resolved
|
Please see #304. |
This pull requests addresses @TTA777 review on #289 wrt using test data builders. This PR ensures we use builders for our models during testing. Though for our tokens we need to give it our custom instantiator, as we would like to keep the property of having the Token type require the two parameters token hash and token type and using this information to set the default expired `DateTime`, but still allowing us to overwrite this time for multiple purposes (testing, specific purposes at a later stage etc.). This PR also introduces integration tests for the new magic link authentication flow, only mocking the actual `IEmailSender`.
A-Guldborg
force-pushed
the
feature/magic-link-login
branch
from
b3f0736 to
c8c3e01
Compare
A-Guldborg
force-pushed
the
feature/magic-link-login
branch
from
c8c3e01 to
8fb7432
Compare
| public async Task<Token> GetValidTokenByHashAsync(string tokenString) | ||
| { | ||
| var tokenHash = _hashService.Hash(tokenString); | ||
| var foundToken = await _context.Tokens.Include(t => t.User).FirstOrDefaultAsync(t => t.TokenHash == tokenHash); |
There was a problem hiding this comment.
Any reason we are not during the filtering for revoked, and expired in the db?
There was a problem hiding this comment.
Good question.
I've changed it to be part of the LINQ statement by using a DB function for the Expired field. Note that I keep an implementation (though rewritten to be static as required by EFCore DbFunctions) for client-side evaluation which is used by tests and can be used in the future if we need to call it client-side even though this requires more maintenance. (client being the backend and not database server). Please also help me evaluate if we should place DbFunctions elsewhere?
There is one downside as you can see from the related commit: For the malicious cases where a token is used multiple times it doesn't return a foundToken with a user where we can invalidate that user's tokens. This is only possible if we do it client-side but I am not sure how much we weigh that use-case? If we want to invalidate in this use-case, it only makes sense to do the tokenhash (and maybe Expired) check server-side and the rest client-side. This also leads to a removed test case in 3d088b8 (among other fixes found)
Btw, see the following screenshot for the EF Core translation for this implementation:
Also, this rabbit-hole led me to fix a bug (in the same commit) ensuring that we hash the generated magic link token before saving it, since we hash it before checking it in the database.
coffeecard/CoffeeCard.WebApi/Controllers/v2/AccountController.cs
Outdated
Show resolved
Hide resolved
| if (foundToken == null || foundToken.Revoked || foundToken.Expired()) | ||
| { | ||
| await InvalidateRefreshTokensForUser(foundToken?.User); | ||
| throw new ApiException("Invalid token", 401); |
There was a problem hiding this comment.
Don't we have a more appropriate exception defined? I haven't checked, but I'd expect we had one
There was a problem hiding this comment.
Couldn't find one more appropriate. This is also how we handle failed authentication attempts in v1 AccountService.
|
|
@duckth https://github.com/AnalogIO/analog-core/actions/runs/12530002648/job/34946131212?pr=289 fails with some bicep error message that I cannot decode. It seems to indicate (to me) that |
This feature aims at enabling magic link authentication through mails. The mail will then provide a deeplink for the correct application (app/shifty) on valid login and use refresh tokens to stay logged in.
This is only for Shifty at this point, since the implementation is missing for the app frontend. Once implemented in both places, we should do a soft roll-over of requiring users to update the app, possibly with a hard requirement at the beginning of the new semester.