passwordless-server icon indicating copy to clipboard operation
passwordless-server copied to clipboard

Use `ApplicationOverrides` to bypass magic link restrictions for Admin Console

Open Tyrrrz opened this issue 10 months ago • 6 comments

We already have this mechanism so might as well use it for AdminConsole as well. This makes the IsAdminConsole check redundant, and we don't have to hard-code the name of the app.

  • Local setup: part of appsetings.Development.json, overrides the test app
  • Devtest, QA, Prod setup: set the ApplicationOverrides__admin__IsRateLimitBypassEnabled=true and ApplicationOverrides__admin__IsMagicLinkQuotaBypassEnabled=true environment variables (replace admin with the actual name of the AdminConsole app)

This simplifies the initial local development setup quite a bit.

Related to https://bitwarden.atlassian.net/browse/PAS-429, although takes a different approach than described in the ticket.

Tyrrrz avatar Apr 18 '24 20:04 Tyrrrz

Codecov Report

Attention: Patch coverage is 55.88235% with 15 lines in your changes are missing coverage. Please review.

Project coverage is 32.51%. Comparing base (a2ea77a) to head (9f95738).

Files Patch % Lines
src/AdminConsole/Services/SetupService.cs 0.00% 7 Missing :warning:
src/AdminConsole/Program.cs 0.00% 6 Missing :warning:
src/Service/MagicLinks/MagicLinkService.cs 0.00% 1 Missing and 1 partial :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #565      +/-   ##
==========================================
- Coverage   32.56%   32.51%   -0.05%     
==========================================
  Files         525      524       -1     
  Lines       26533    26529       -4     
  Branches      858      859       +1     
==========================================
- Hits         8640     8627      -13     
- Misses      17774    17781       +7     
- Partials      119      121       +2     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Apr 18 '24 20:04 codecov[bot]

@jrmccannon something I noticed later: it seems that, currently, AC will redirect to /initialize, even if the data is already seeded. I assume it was changed recently after our call. Since the seeded data is now valid and we don't have to go through the /initialize sequence in local development, can we re-add a check that lets us avoid the initialization sequence? If so, could you advise how to do it?

Tyrrrz avatar Apr 19 '24 16:04 Tyrrrz

@jrmccannon something I noticed later: it seems that, currently, AC will redirect to /initialize, even if the data is already seeded. I assume it was changed recently after our call. Since the seeded data is now valid and we don't have to go through the /initialize sequence in local development, can we re-add a check that lets us avoid the initialization sequence? If so, could you advise how to do it?

What should currently happen is that AC will check to see if its own database has had a migration ran. If there have been any migrations applied (database initialized), then it redirects to Login or Overview. Otherwise we go to Initialize.

When you say the data has been seeded, are you talking about admin console existing in API or within its own database?

The initialization step in AC sets up the AdminConsole application in API, runs all migrations (creating the database) for AC, then adds AC as an application and organization to itself.

Having all this done ensures both AC and API are in valid states and running like every other application.

Which part would we need to skip and where?

jrmccannon avatar Apr 19 '24 18:04 jrmccannon

@jrmccannon something I noticed later: it seems that, currently, AC will redirect to /initialize, even if the data is already seeded. I assume it was changed recently after our call. Since the seeded data is now valid and we don't have to go through the /initialize sequence in local development, can we re-add a check that lets us avoid the initialization sequence? If so, could you advise how to do it?

What should currently happen is that AC will check to see if its own database has had a migration ran. If there have been any migrations applied (database initialized), then it redirects to Login or Overview. Otherwise we go to Initialize.

When you say the data has been seeded, are you talking about admin console existing in API or within its own database?

The initialization step in AC sets up the AdminConsole application in API, runs all migrations (creating the database) for AC, then adds AC as an application and organization to itself.

Having all this done ensures both AC and API are in valid states and running like every other application.

Which part would we need to skip and where?

Let me explain. With the changes in this PR, the startup flow looks like this:

  • Run API
  • Apply migrations & seed data in the API
  • Run AdminConsole
  • Going to the home page immediately redirects to the /initialize page, where it asks me to create a new Admin account

With the Admin app being seeded by the API, I believe we'd only need to migrate the DB in AC and it should be good to go, without the need to create a new Admin account.

Maybe AC can check if the provided keys in the configuration are valid (by sending a test request) and only redirect to the /initialize page if they are not, and just perform migrations if they are.

What do you think @jrmccannon?

Tyrrrz avatar Apr 22 '24 14:04 Tyrrrz

Ok done

Tyrrrz avatar Apr 22 '24 17:04 Tyrrrz

Did we validate that self-hosting starts up and runs fine with these changes?

I couldn't get it to run locally using run_self-hosting-image.sh:

image

That said, isn't self-host running in production environment, not development? In which case it shouldn't be affected.

Tyrrrz avatar Apr 23 '24 14:04 Tyrrrz