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.

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(wip): Changes to support presto impersionation with ldap #13156

Closed

Conversation

rijojoseph07
Copy link
Contributor

SUMMARY

Fix for issue #11359, #9406

When using LDAP authentication with presto/trino, the current behavior is just to modify the URL to replace the username which will result in an unauthorized exception. This PR will fix this by updating the connection argument with the effective user.

TEST PLAN

Step 1: Setup database connection to presto without credentials in URL.
Step 2: Provide admin(who can impersonate as any other user in presto) credentials in connection properties via extras

"engine_params": {
           "connect_args":{
              "protocol": "https",
              "username":"<admin>",
              "password":"<admin_password>"
           }
}

Step 3: Enable impersonation for the database connection.
Step 4: Log in with a different user and run the query via SQL lab and you will see the principal user as admin and user as the logged-in user in presto UI.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@rijojoseph07 rijojoseph07 changed the title [WIP] Changes to support presto impersionation with ldap fix:[WIP] Changes to support presto impersionation with ldap Feb 16, 2021
@rijojoseph07 rijojoseph07 changed the title fix:[WIP] Changes to support presto impersionation with ldap fix:(WIP) Changes to support presto impersionation with ldap Feb 16, 2021
@rijojoseph07 rijojoseph07 changed the title fix:(WIP) Changes to support presto impersionation with ldap fix(WIP): Changes to support presto impersionation with ldap Feb 16, 2021
@rijojoseph07 rijojoseph07 changed the title fix(WIP): Changes to support presto impersionation with ldap fix: Changes to support presto impersionation with ldap Feb 16, 2021
@codecov-io
Copy link

codecov-io commented Feb 18, 2021

Codecov Report

Merging #13156 (e07d840) into master (2ce7982) will increase coverage by 5.36%.
The diff coverage is 72.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #13156      +/-   ##
==========================================
+ Coverage   53.06%   58.42%   +5.36%     
==========================================
  Files         489      468      -21     
  Lines       17314    15920    -1394     
  Branches     4482     4088     -394     
==========================================
+ Hits         9187     9301     +114     
+ Misses       8127     6619    -1508     
Flag Coverage Δ
cypress 58.42% <72.22%> (+5.36%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...end/src/SqlLab/components/RunQueryActionButton.tsx 52.77% <ø> (ø)
superset-frontend/src/chart/ChartContainer.jsx 100.00% <ø> (ø)
superset-frontend/src/chart/ChartRenderer.jsx 75.67% <0.00%> (-1.04%) ⬇️
...perset-frontend/src/common/components/Dropdown.tsx 50.00% <ø> (ø)
...rontend/src/components/ListView/CardSortSelect.tsx 78.94% <ø> (ø)
...ontend/src/components/ListViewCard/ImageLoader.tsx 75.00% <0.00%> (ø)
...set-frontend/src/components/URLShortLinkButton.jsx 72.22% <ø> (-27.78%) ⬇️
...src/dashboard/components/HeaderActionsDropdown.jsx 68.91% <ø> (+1.35%) ⬆️
...end/src/dashboard/components/StickyVerticalBar.tsx 100.00% <ø> (ø)
...omponents/nativeFilters/FilterConfigModal/types.ts 100.00% <ø> (ø)
... and 141 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 94893ff...e07d840. Read the comment docs.

@pull-request-size pull-request-size bot added size/L and removed size/M labels Feb 18, 2021
@rijojoseph07 rijojoseph07 changed the title fix: Changes to support presto impersionation with ldap fix(wip): Changes to support presto impersionation with ldap Feb 18, 2021
@DRavikanth
Copy link

@rijojoseph07 Which release is this targeted for? I am having issues with Impersonation on SSv1.0.1

@rijojoseph07 rijojoseph07 changed the title fix(wip): Changes to support presto impersionation with ldap fix: Changes to support presto impersionation with ldap Mar 17, 2021
@rijojoseph07 rijojoseph07 changed the title fix: Changes to support presto impersionation with ldap fix(wip): Changes to support presto impersionation with ldap Mar 17, 2021
@rijojoseph07
Copy link
Contributor Author

@rijojoseph07 Which release is this targeted for? I am having issues with Impersonation on SSv1.0.1

@villebro Can you please help here?

@villebro
Copy link
Member

@rijojoseph07 how can I help? This PR appears to be closed.

@rijojoseph07
Copy link
Contributor Author

@rijojoseph07 how can I help? This PR appears to be closed.

I meant if you have any details on the release date, can you please share it? @villebro

@villebro
Copy link
Member

1.1 has been cut many weeks ago and is approaching the first RC (@betodealmeida might know a more exact timeline), but I'd need to know which PR you are interested in seeing included.

@rijojoseph07
Copy link
Contributor Author

@DRavikanth was asking for a targeted release for this PR since he is facing issues with impersonation.

@villebro
Copy link
Member

@rijojoseph07 AFAIK this PR isn't merged, so it's not in master or any upcoming release, either. If you wish to have this PR reviewed, please reopen it.

@rijojoseph07
Copy link
Contributor Author

@rijojoseph07 AFAIK this PR isn't merged, so it's not in master or any upcoming release, either. If you wish to have this PR reviewed, please reopen it.

Sorry @villebro I got confused with this PR which was merged. #13214

@villebro
Copy link
Member

Ok, now I understand; #13214 is included in the upcoming 1.1 version, so shouldn't be long now 🙂

@rijojoseph07
Copy link
Contributor Author

Ok, now I understand; #13214 is included in the upcoming 1.1 version, so shouldn't be long now 🙂

Thanks, @villebro. @DRavikanth FYI ^^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants