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

Ensure failures at signup are recoverable #44

Open
1 task done
Supereg opened this issue Aug 31, 2024 · 3 comments
Open
1 task done

Ensure failures at signup are recoverable #44

Supereg opened this issue Aug 31, 2024 · 3 comments
Labels
enhancement New feature or request

Comments

@Supereg
Copy link
Member

Supereg commented Aug 31, 2024

Problem

Several async calls have to complete such that a signup is considered successful.

  1. The initial account creation.
  2. Updating the initial display name.
  3. Storing external account details. The account creation must be the first step in order to retrieve the unique account identifier.

If one of the later steps fail, the signup operation cannot be repeated as the account with the provided credentials are already in use. Instead the user has to login and manually repeat the failed steps (e.g., update the display name or update externally stored account details). This is not a great user experience. Further, we break the guarantee that required account details are saved.

Solution

A good solution would be to move the final account creation to the end of the method. However, all the other operations require a valid Firebase user identifier (accountId). To resolve this, we could always create an anonymous user account first (if not provided already), update the display name and store external account details and link the credentials as the last step.
If any of the operations fail, we could try to delete and sign out the anonymous user account (and externally stored data) but silently fail if that doesn't work. The new worst case is, that we have a slight database pollution. However, one can resolve this by periodically clean all entries that are not associated with a valid firebase user account.

Additional context

The errors returned from link are different to createUser are different (e.g., credentials that are already present). We would need to make sure that we maintain the same error behavior.

Code of Conduct

  • I agree to follow this project's Code of Conduct and Contributing Guidelines
@Supereg Supereg added the enhancement New feature or request label Aug 31, 2024
@PSchmiedmayer
Copy link
Member

Thank you for outlining this issue.

Wondering if we could at least cover the workflow where an account creation has been successful. I could imagine that we jump back to the Signup screen that can then show an account overview as we do for the Template Application if a user is still signed in after having deleted the application (as keychain elements are not removed when an app is uninstalled).
From there, we could let the user proceed and use the existing mechanism that re-displays the sheet to provide the required account details. If saving these indeed fails repeatedly, we can not proceed (and this is most likely due to any server issue or something else that is misconfigured), but at least we can indicate to the user that their credentials are saved, and they are not confused if they have to sign up again or login, we did that job for them. What do you think about this approach?

Anonymous Account Idea

Using an Anonymous account to kickstart the process is not a desired idea; this requires a small side quest of an explanation:

  1. The Firebase instance could be configured so that no Anonymous account creation is allowed
  2. In addition, Firebase is terrible at safeguarding account creation using an Anonymous account. IMO, this is one of the main motivations for moving away from Firebase in the long run, and we can stop recommending anything more complex than a prototype once we have a more reusable Spezi server infrastructure.

Their "blocking functions" that should safeguard account creation are not triggered for anonymous accounts and linking credentials to an anonymous account is a de-facto backdoor to create full-fledged accounts without any way to stop a user using blocked functions:

  1. Blocked functions are never called when an Anonymous account is created (IMO, generally not a great idea, but at least documented in their docs).
  2. Normal linking operations to an existing account trigger the blocking function in sign-in. This is not happening for linking to an anonymous account (a clear missing element, not documented at all, and we reached out to Firebase Support and were just repeatedly told what is written in the docs)

As a result, a bad actor can easily circumvent all blocking account functions if anonymous authentication is enabled. And there seems to be no concern from the Firebase team about addressing this. Any, e.g., invitation code-based sign-in process that first uses an anonymous account + cloud function with an initial secret shared out-of-band that we can then check in a blocking function is therefore impossible to enforce). There is also no way to pass in any additional context to a sign up where we could pass in that shared secrete to be read and used to create a blocking decision by any of the Firebase cloud functions. We suggested this as a feature addition to the Firebase team.

The only way we can implement something close to this is a scheduled cloud function that removes accounts that got in through this mechanism as well as a could function that needs to be called after the creation with the secret (e.g., invitation code) shared out-of-band + rely on claims set by this cloud function to ensure that an account created outside of this mechanism has no Firestore or Firebase Storage access. Still ideal as we start to sure user information in the system in Auth and we can't really block this.
I am sure that a lot of Firebase projects out there don't go through that process and are basically open to account creation using the mechanism above even though they think blocking functions could secure this.

In conclusion, that leads me to believe that we shouldn't push users to go the Anonymous route until Firebase has acknowledged and fixed this issue. We had some good discussions in the team, and in combination with a challenging scoping of permissions for Firebase deployment, we are considering how useful this will be for us beyond smaller scoped projects that can benefit from the simple Firebase setup and especially Anonymous authentication.

@Supereg
Copy link
Member Author

Supereg commented Sep 1, 2024

If saving these indeed fails repeatedly, we can not proceed (and this is most likely due to any server issue or something else that is misconfigured), but at least we can indicate to the user that their credentials are saved, and they are not confused if they have to sign up again or login, we did that job for them. What do you think about this approach?

I thought of a similar approach before I knew all the implications of anonymous account sign in you mentioned above (thank you for all the details!). This approach is just the harder to do/design right. We would need to find a mechanism to model a signup operation to fail partially and SpeziAccount to know that. We would probably want to display an error to the user that not everything could be saved (and the reason for that). At the same time the signup should be considered successful.

I will brainstorm around the topic a bit 👍

@PSchmiedmayer
Copy link
Member

Sound good; let's put this on the agenda for the next meeting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants