-
Notifications
You must be signed in to change notification settings - Fork 279
feat: integrate user's ip address into debugger tool COMPASS-11046 #8397
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: main
Are you sure you want to change the base?
Changes from all commits
3f854a9
3739a9d
6df2384
86acfe8
388d083
179daa3
7566b1f
a09bf88
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| /** | ||
| * Subset of the Atlas Admin API system status object (`GET /api/atlas/v2`) that | ||
| * we consume: the public IP address the request originated from (always | ||
| * returned) and, when the request is authenticated as a user rather than an API | ||
| * key, the user making it. | ||
| */ | ||
| export type AtlasSystemStatus = { | ||
| ipAddress: string; | ||
| user?: { username: string }; | ||
| }; | ||
|
|
||
| export function assertSystemStatus( | ||
| json: unknown | ||
| ): asserts json is AtlasSystemStatus { | ||
| const status = json as { ipAddress?: unknown; user?: { username?: unknown } }; | ||
| if ( | ||
| !json || | ||
| typeof json !== 'object' || | ||
| typeof status.ipAddress !== 'string' | ||
| ) { | ||
| throw new Error( | ||
| 'Got unexpected backend response for Atlas Admin API system status request, missing or malformed ipAddress' | ||
| ); | ||
| } | ||
| if ( | ||
| status.user !== undefined && | ||
| (typeof status.user !== 'object' || | ||
| typeof status.user.username !== 'string') | ||
| ) { | ||
| throw new Error( | ||
| 'Got unexpected backend response for Atlas Admin API system status request, missing or malformed username' | ||
| ); | ||
|
paula-stacho marked this conversation as resolved.
|
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -619,6 +619,17 @@ describe('CompassAuthServiceMain', function () { | |
| expect(authHeaders).to.not.have.property('X-Compass-Auth'); | ||
| }); | ||
|
|
||
| it('should add auth headers for the system status request', async function () { | ||
| const authHeaders = await CompassAuthService.handleAuthHeaders({ | ||
| requestHeaders: { 'X-Compass-Auth': 'true' }, | ||
| url: `${defaultConfig.atlasAdminApiBaseUrl}/v2`, | ||
| }); | ||
| expect(authHeaders).to.have.property( | ||
| 'Authorization', | ||
| `Bearer ${accessToken}` | ||
| ); | ||
| }); | ||
|
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we need to test every endpoint that is on the list, but it won't hurt I suppose |
||
| it('should not add auth headers if they werent asked for', async function () { | ||
| const url = 'http://example.com/api/private/some-endpoint'; | ||
| const oldHeaders = { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -135,17 +135,18 @@ async function getNetworkAccessInfo({ | |
| ipAccessStatus: IpAccessStatus; | ||
| networkAccessDetails: NetworkAccessDetails; | ||
| }> { | ||
| const ipAccessList = await atlasAdminApi.getProjectIPAccessList(projectId); | ||
| // TODO(COMPASS-10981): replace with Atlas Admin API once it's ready | ||
| const userIp = '1.2.3.4'; | ||
| const [ipAccessList, { ipAddress: userIp }] = await Promise.all([ | ||
| atlasAdminApi.getProjectIPAccessList(projectId), | ||
| atlasAdminApi.getSystemStatus(), | ||
| ]); | ||
| return { | ||
| ipAccessStatus: | ||
| ipAccessList && userIp && isUserIpIncluded(ipAccessList, userIp) | ||
| ? 'Client IP Allowed' | ||
| : 'Could not confirm', | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Surprisingly, many customers have problems with ip whitelisting. To give more information, I would separate:
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We left the "can't confirm" intentionally vague for now as we realised we can't always tell (for example we don't know how to check awsSecurityGroup), but distinguishing that the request failed would make sense if we allow partial results. For now the plan is to not go with partial results though, if some of the requests fail we just let the user to retry the tool.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We might want to add a clear 'not match' for when we can tell for sure though, will probably be a follow up improvement |
||
| networkAccessDetails: { | ||
| networkAccessList: ipAccessList, | ||
| userIp, | ||
| ...(userIp && { userIp }), | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.