Skip to content
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

Login with magic link #289

Open
wants to merge 34 commits into
base: main
Choose a base branch
from
Open

Login with magic link #289

wants to merge 34 commits into from

Conversation

A-Guldborg
Copy link
Contributor

@A-Guldborg A-Guldborg commented Oct 22, 2024

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.

@duckth duckth force-pushed the feature/magic-link-login branch from fcc8943 to b7cc783 Compare October 22, 2024 17:10
@duckth duckth force-pushed the feature/magic-link-login branch from 2ec47d1 to 625f332 Compare November 12, 2024 17:19
@duckth duckth force-pushed the feature/magic-link-login branch from d62fe22 to 689b2f5 Compare November 15, 2024 11:31
@A-Guldborg A-Guldborg force-pushed the feature/magic-link-login branch from 330ed37 to 874dada Compare November 19, 2024 19:04
@A-Guldborg A-Guldborg marked this pull request as ready for review November 19, 2024 19:42
@A-Guldborg
Copy link
Contributor Author

Email service is going to be hard to test compared to the testing probably required, considering it is mostly building templates by replacing structured values. This is due to it opening files which is different between running tests locally and in our workflow.

This leaves us slightly below required testing coverage requirements but imo it should be okay in this very case.

Other than that, ready for review :)

Copy link
Member

@TTA777 TTA777 left a comment

Choose a reason for hiding this comment

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

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; }
Copy link
Member

Choose a reason for hiding this comment

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

You will also need to update the infra files for this to work as intended

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@duckth can you help with this part? I am not quite sure how the deployment works :)

Copy link
Member

Choose a reason for hiding this comment

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

Yes

coffeecard/CoffeeCard.Library/Services/v2/TokenService.cs Outdated Show resolved Hide resolved
@A-Guldborg
Copy link
Contributor Author

A-Guldborg commented Nov 26, 2024

Please see #304.

duckth and others added 5 commits December 3, 2024 16:09
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 A-Guldborg force-pushed the feature/magic-link-login branch from b3f0736 to c8c3e01 Compare December 3, 2024 19:11
@A-Guldborg A-Guldborg force-pushed the feature/magic-link-login branch from c8c3e01 to 8fb7432 Compare December 3, 2024 19:11
Copy link

sonarcloud bot commented Dec 3, 2024

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.

4 participants