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.

Sign up for GitHub

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

fix: fix warning in ssh tunnel #22912

Merged
merged 2 commits into from
Jan 31, 2023
Merged

fix: fix warning in ssh tunnel #22912

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
245a258
fix warning in ssh tunnel
AAfghahi Jan 30, 2023
56069d7
add eric feedback
AAfghahi Jan 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ import {
ExtraJson,
} from 'src/views/CRUD/data/database/types';
import Loading from 'src/components/Loading';
import { omit } from 'lodash';
import ExtraOptions from './ExtraOptions';
import SqlAlchemyForm from './SqlAlchemyForm';
import DatabaseConnectionForm from './DatabaseConnectionForm';
Expand Down Expand Up @@ -374,21 +375,21 @@ export function dbReducer(
};
case ActionType.setSSHTunnelLoginMethod:
if (action.payload.login_method === AuthType.privateKey) {
const { password, ...rest } = trimmedState?.ssh_tunnel ?? {};
return {
...trimmedState,
ssh_tunnel: {
...rest,
...omit(trimmedState?.ssh_tunnel, ['password']),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AAfghahi It might be worth adding a conditional check to skip cases where no ssh tunnel info exists and consolidating the omit to a single check like:

case ActionType.setSSHTunnelLoginMethod:
  let ssh_tunnel;
  if (trimmedState?.ssh_tunnel) {
    // remove any attributes that are considered sensitive
    ssh_tunnel = omit(trimmedState.ssh_tunnel, [
      'password'
      'private_key',
      'private_key_password',
    ]);
  }

  return {
    ...trimmedState,
    ssh_tunnel,
  };

This way if additional fields get added at some point that also need to be filtered you can just add to the array of fields passed to omit without having more if blocks to differentiate between the AuthType. omit will work fine getting attributes that may or may not exist on trimmedState.ssh_tunnel so we can just keep a single array of sensitive attributes

Copy link
Member

@Antonio-RiveroMartnez Antonio-RiveroMartnez Jan 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

omit handles undefined and null values (will return an empty object) already. About the conditionals, we need the properties to be removed individually so there's always a login method available in the tunnel, i.e, we shouldn't delete them all at once. The use case for this action is:

If you select login method === password -> remove any private_key & private_key_password from the tunnel object we send in the payload to the API and vice versa.

That way the tunnel always has only 1 login method and not the two of them which will cause issues later when trying to connect because you won't know which method to use.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eric's method of making sure that there is no issue around null and undefined makes me a bit more comfortable, personally.

Instead of omitting, could we use a pick here instead? @Antonio-RiveroMartnez is there a place where we define what columns the object should have? Are they the same btwn the two login methods?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure you could use pick. superset-frontend/src/views/CRUD/data/database/types.ts has the SSHTunnelObject type definition.

You would have to pick id, server_address, server_port, username , and finally the password OR private_key & private_key_password depending on your selected login method. More typing and prone to more changes if we add more properties in the tunnel definition later on, but totally doable 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why would we pick password/private_key/private_key_password aren't those the ones we don't want included?

Copy link
Member

@Antonio-RiveroMartnez Antonio-RiveroMartnez Jan 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tunnel must have a login method, only one between password or private_key. If you select password as login method, you remove the private_key, and vice versa. So, before with the omit, if login method === password you omit the private_key and if you select login method === private_key you omit the password, that way your tunnel has only 1 login credentials (the one that you selected)

Now, you want to use pick, in that case if you select login method === password you need to pick the common properties and include the password, otherwise there's no login credentials associated to the tunnel. Same if you select login method === private_key, you need to pick the common properties and include the private_key credentials.

The goal for this action is to make sure your selected login method is the only one in the tunnel. That can be done by excluding the other method (omit) or by adding just the selected credentials to the common properties (pick)

},
};
}
if (action.payload.login_method === AuthType.password) {
const { private_key, private_key_password, ...rest } =
trimmedState?.ssh_tunnel ?? {};
return {
...trimmedState,
ssh_tunnel: {
...rest,
...omit(trimmedState?.ssh_tunnel, [
'private_key',
'private_key_password',
]),
},
};
}
Expand Down