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

Add Basic Authentication to Druid #20412

Merged
merged 32 commits into from
May 17, 2023
Merged

Add Basic Authentication to Druid #20412

merged 32 commits into from
May 17, 2023

Conversation

nbotelhodev
Copy link
Contributor

@nbotelhodev nbotelhodev commented Feb 10, 2022

Objective

This PR allows using of basic header authentication only in Druid Database connection, files modified are exclusive of Druid code.

Feature

It's a simple adjustment, I added three fields in additional options UI and receive the values in Druid driver core to put in the basic header option of clj-http.

Tests

  • My tests check if the basic header option is in clj-http request options or not.
  • Run the frontend and Cypress end-to-end tests with yarn lint && yarn test)
  • If there are changes to the backend codebase, run the backend tests with Clojure -X:dev:test
  • Sign the Contributor License Agreement
    (unless it's a tiny documentation change).

This change is Reviewable

@nbotelhodev nbotelhodev mentioned this pull request Feb 10, 2022
4 tasks
@codecov
Copy link

codecov bot commented Feb 10, 2022

Codecov Report

Patch coverage has no change and project coverage change: -0.03 ⚠️

Comparison is base (7557684) 70.80% compared to head (17c0a60) 70.78%.

❗ Current head 17c0a60 differs from pull request most recent head 203f5d3. Consider uploading reports for the commit 203f5d3 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #20412      +/-   ##
==========================================
- Coverage   70.80%   70.78%   -0.03%     
==========================================
  Files        2897     2893       -4     
  Lines      101037   100922     -115     
  Branches    12672    12653      -19     
==========================================
- Hits        71542    71435     -107     
+ Misses      24038    24037       -1     
+ Partials     5457     5450       -7     
Flag Coverage Δ
back-end 86.60% <ø> (-0.01%) ⬇️

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

see 68 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@flamber flamber changed the title Nbotelho/issue18808 Add Basic Authentication to Druid Feb 10, 2022
@flamber flamber linked an issue Feb 10, 2022 that may be closed by this pull request
@flamber flamber requested a review from camsaul February 10, 2022 08:12
Copy link
Member

@camsaul camsaul left a comment

Choose a reason for hiding this comment

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

Cool, this LGTM.

I had to approve this to run on CI so when that's done we can merge. Some of the jobs should fail since we don't inject credentials into third-party PRs but we can still merge anyway if Druid tests and linters pass

@gpodevijn
Copy link

Thanks again for the work, it'll be very useful for us.
Is there any chance that this PR will be merged in the next release?

@camsaul camsaul self-assigned this Feb 24, 2022
@gpodevijn
Copy link

I just realised that the token's box is not a "password" box which makes the token visible from the UI (and probably not encrypted in the DB).

We're using Imply's Druid API pass as the token value to be able to connect to Druid. Would it be possible to hide and encrypt the token value?

Thank you!

image

@flamber
Copy link
Contributor

flamber commented Feb 28, 2022

@gpodevijn Encryption is handled for everything in metabase_database.details https://www.metabase.com/docs/latest/operations-guide/encrypting-database-details-at-rest.html

And change type: string to type: password or type: secret, that should automatically prevent it from being included in responses.

@camsaul camsaul removed their assignment Jun 7, 2022
camsaul
camsaul previously requested changes Jun 7, 2022
Copy link
Member

@camsaul camsaul left a comment

Choose a reason for hiding this comment

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

  • Fix merge conflicts
  • Fix plaintext passwords

Then request another review 💪

@Ray33
Copy link

Ray33 commented Sep 4, 2022

@flamber @camsaul Can you please update about the status of this PR ?

@flamber
Copy link
Contributor

flamber commented Sep 4, 2022

@Ray33 It's written right above your comment.

@Ray33
Copy link

Ray33 commented Sep 6, 2022

@flamber Changes were done . Please check.

@flamber
Copy link
Contributor

flamber commented Sep 6, 2022

@Ray33 And @nbotelhodev has already requested a review from Cam a few hours ago, so you'll have to wait until there's a review.

@nbotelhodev
Copy link
Contributor Author

nbotelhodev commented Oct 6, 2022

Could you help me to merge this PR, please?

@Ray33
Copy link

Ray33 commented Oct 12, 2022

Could you help to merge this PR, please?

@nbotelhodev
Copy link
Contributor Author

nbotelhodev commented Oct 18, 2022

Hey guys @gpodevijn @flamber
Help me merge this MR, please!

@Ray33
Copy link

Ray33 commented Nov 10, 2022

@flamber can you please help with merge and rollout ?

@camsaul camsaul requested review from a team and removed request for camsaul February 25, 2023 01:16
@camsaul camsaul dismissed their stale review February 25, 2023 01:19

out of date

@camsaul
Copy link
Member

camsaul commented Feb 27, 2023

@metabase/core-backend-qp let's see if we can get this over the line for the next cycle

@carstenkadmos
Copy link

having this would help us a great deal as well! Thanks

Copy link
Contributor

@metamben metamben left a comment

Choose a reason for hiding this comment

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

LGTM

@darksciencebase darksciencebase merged commit 2bd0b3c into metabase:master May 17, 2023
@nbotelhodev nbotelhodev deleted the nbotelho/issue18808 branch May 24, 2023 02:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Basic Authentication to Druid Queries
9 participants