[INS-1805] Add Auth Header for WebSocket - Part 1 #5108
Conversation
| update(activeRequest, { authentication:newAuthentication }); | ||
| }, [activeRequest]); | ||
| const isCurrent = useCallback((type: string) => { | ||
| if (!activeRequest) { |
There was a problem hiding this comment.
Not sure why this was needed. It made sense to remove it since with this change, it does not refer to a request directly inside any more.
| const onClick = useCallback(async (type: string) => { | ||
| if (!activeRequest) { |
There was a problem hiding this comment.
Not sure why this was needed. It made sense to remove it since with this change, it does not refer to a request directly inside any more.
| <AuthItem type={AUTH_NETRC} {...itemProps} /> | ||
| <DropdownDivider>Other</DropdownDivider> | ||
| <AuthItem type={AUTH_NONE} nameOverride="No Authentication" {...itemProps} /> | ||
| {authTypes.reduce<ReactElement[]>((acc: ReactElement[], authType: AuthType) => { |
There was a problem hiding this comment.
This was needed as we wanted to support only three auth types for the initial websocket support; basic, digest, and bearer.
marckong
changed the title
|
Still need to fix unit tests that are wired up with redux, but I will request a review for early review. |
| <AuthSettingsProvider | ||
| authentication={request.authentication} | ||
| onAuthUpdate={handleAuthUpdate} | ||
| > |
There was a problem hiding this comment.
Can you describe a bit your thinking around using context provider here. Since the useActiveRequest hook was just wrapping redux I can see why you chose it, but it would be nice to have a record of your rationale.
There was a problem hiding this comment.
useActiveRequest was selecting activeRequest model but then it was eventually calling models.request.update(). Please refer to this below line. This gets called throughout the component tree under AuthWrapper or AuthDropdown. Now with WebSocketRequest model, we would have to alternate models.webSocketRequest.update() / models.requestupdate().
I put the context to allow us to pass the request/webSocketRequest update method to be passed depending on the usage.
| * Ensure the request.authentication.type property is added | ||
| * @param request | ||
| */ | ||
| function migrateAuthType(request: WebSocketRequest) { |
There was a problem hiding this comment.
discussed to remove this
|
Closing as it is implemented with the current approach as we discussed - #5115 |
Part 1
This part only includes the UI and adding the authentication to the request model only. The Part 2 would include disabling and enabling the auth fields based on connection status.
Design Decisions:
Ideally it may be beneficial to separate "WrapperDebug" into different pages for debug activity type; gRPC, GraphQL, HTTP, WebSocket or any kind we may want to support in the future.
Currently, WrapperDebug alters what to render by checking boolean, but it would be great if we utilize router for this with params. We have had to have isRequest, isWebSocketRequest, isGrpcRequest methods because we were not separating them in each own component tree. Consequently we are calling db operation in the subset components not in the container component.
The separation of db operation per kind should ideally happen in the container level, not in the sub component level (if they are to be reused). The introduction of Context/Provider pattern was to provide that abstraction to allow resuability of Auth component set without calling the db operation in them but calling them from their container level.
In this design, it is a step forward to attempt the following state demonstrated in the attached screenshot. Note that unlike this PR changes, the screenshot uses AuthSettingsContext only within the tab panel, not in the tab list with the dropdown. This PR did not introduce that as it hit some limitations with the current approach.
Limitations: