-
Notifications
You must be signed in to change notification settings - Fork 26
Add authentication and SSL/TLS to ServicePulse #2729
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
base: master
Are you sure you want to change the base?
Conversation
da0cc40 to
787ab48
Compare
48fcaed to
c5d166e
Compare
559660c to
62dd485
Compare
This should save ~286KB gzipped
…Add auth detail loading screen.
6abcbdc to
477556a
Compare
|
|
||
| ## Prerequisites | ||
|
|
||
| - ServicePulse built locally (see main README for build instructions) |
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.
| - ServicePulse built locally (see main README for build instructions) | |
| - ServicePulse built locally (see [main README for instructions](../README.md#setting-up-the-project-for-development)) | |
|
|
||
| ## Prerequisites | ||
|
|
||
| - ServicePulse built locally (see main README for build instructions) |
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.
| - ServicePulse built locally (see main README for build instructions) | |
| - ServicePulse built locally (see [main README for instructions](../README.md#setting-up-the-project-for-development)) |
| ## .NET 8 Prerequisites | ||
|
|
||
| - [mkcert](https://github.com/FiloSottile/mkcert) for generating local development certificates | ||
| - ServicePulse built locally (see main README for build instructions) |
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.
| - ServicePulse built locally (see main README for build instructions) | |
| - ServicePulse built locally (see [main README for instructions](../README.md#setting-up-the-project-for-development)) |
|
|
||
| - [Docker Desktop](https://www.docker.com/products/docker-desktop/) installed and running | ||
| - [mkcert](https://github.com/FiloSottile/mkcert) for generating local development certificates | ||
| - ServicePulse built locally (see main README for build instructions) |
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.
| - ServicePulse built locally (see main README for build instructions) | |
| - ServicePulse built locally (see [main README for instructions](../README.md#setting-up-the-project-for-development)) |
|
|
||
| This guide provides scenario-based tests for ServicePulse's OIDC authentication. Use this to verify authentication behavior during local development. | ||
|
|
||
| For additional details on authentication in ServicePulse, see the [ServicePulse Security](https://docs.particular.net/servicepulse/security/configuration/authentication). |
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.
| For additional details on authentication in ServicePulse, see the [ServicePulse Security](https://docs.particular.net/servicepulse/security/configuration/authentication). | |
| For additional details on authentication in ServicePulse, see the [ServicePulse Security](https://docs.particular.net/servicepulse/security/configuration/authentication) documentation. |
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.
Similar to SC PR, this file can probably be removed
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.
Similar to SC PR, this file can probably be removed
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.
Similar to SC PR, this file can probably be removed
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.
Similar to SC PR, this file can probably be removed
|
|
||
| This guide provides scenario-based tests for ServicePulse's direct HTTPS features. Use this to verify HTTPS behavior without a reverse proxy. | ||
|
|
||
| > [!NOTE] HTTP to HTTPS redirection (`RedirectHttpToHttps`) is designed for reverse proxy scenarios where the proxy forwards HTTP requests to ServicePulse. When running with direct HTTPS, ServicePulse only binds to a single port (HTTPS). To test HTTP to HTTPS redirection, see [Reverse Proxy Testing](nginx-testing.md). HSTS should not be tested on localhost because browsers cache the HSTS policy, which could break other local development. To test HSTS, use the [NGINX reverse proxy setup](nginx-testing.md) with a custom hostname (`servicepulse.localhost`). |
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.
| > [!NOTE] HTTP to HTTPS redirection (`RedirectHttpToHttps`) is designed for reverse proxy scenarios where the proxy forwards HTTP requests to ServicePulse. When running with direct HTTPS, ServicePulse only binds to a single port (HTTPS). To test HTTP to HTTPS redirection, see [Reverse Proxy Testing](nginx-testing.md). HSTS should not be tested on localhost because browsers cache the HSTS policy, which could break other local development. To test HSTS, use the [NGINX reverse proxy setup](nginx-testing.md) with a custom hostname (`servicepulse.localhost`). | |
| > [!NOTE] | |
| > HTTP to HTTPS redirection (`RedirectHttpToHttps`) is designed for reverse proxy scenarios where the proxy forwards HTTP requests to ServicePulse. When running with direct HTTPS, ServicePulse only binds to a single port (HTTPS). To test HTTP to HTTPS redirection, see [Reverse Proxy Testing](nginx-testing.md). HSTS should not be tested on localhost because browsers cache the HSTS policy, which could break other local development. To test HSTS, use the [NGINX reverse proxy setup](nginx-testing.md) with a custom hostname (`servicepulse.localhost`). |
johnsimons
left a comment
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.
I reviewed the vue code and left a few comments
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.
Have you considered using TypeScript to create this rule?
It seems you can do it with https://typescript-eslint.io/developers/custom-rules
| position: relative; | ||
| z-index: 1000; |
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.
Why did we have to change this?
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.
Why do we need these changes?
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.
Why is this file added back?
As part of the refactoring @phase and I did, we removed this.
| <div class="logged-out-content"> | ||
| <h1 class="logged-out-title">You have been signed out</h1> | ||
| <p class="logged-out-message">You have successfully signed out of ServicePulse.</p> | ||
| <button v-if="authEnabled" type="button" class="btn btn-primary sign-in-button" @click="handleSignIn">Sign in again</button> |
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 authEnabled is false, doesn't that mean auth is disabled and hence why would we be rendering this view at all?
| </div> | ||
| </div> | ||
| <div v-else-if="isAuthenticating" class="loading-overlay"> | ||
| <div class="loading-spinner"> |
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.
We probably should be using the existing LoadingSpinner component?
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.
I wonder what this would look like if we were to wrap App with some other AuthApp component?
So instead of modifying App and add the auth to it, we would create a new AuthApp component that does the auth and then displays <App />.
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.
This does not seem to be need?
| uses: actions/setup-dotnet@v5.1.0 | ||
| with: | ||
| dotnet-version: 7.0.x | ||
| dotnet-version: 8.0.x |
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.
We're bumping the major of .net.
So we need to bump our major too?
| enabled: boolean; | ||
| client_id: string; | ||
| authority: string; | ||
| api_scopes: string; |
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.
Should this be a string[]?
Addresses: #1723