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

[KYUUBI #641]Support custom authentication #642

Closed
wants to merge 4 commits into from

Conversation

hddong
Copy link
Contributor

@hddong hddong commented May 25, 2021

Why are the changes needed?

Users may have their own authentication system

How was this patch tested?

  • Add some test cases that check the changes thoroughly including negative and positive cases if possible

  • Add screenshots for manual tests if appropriate

  • Run test locally before make a pull request

val AUTHENTICATION_CUSTOM_CLASS: OptionalConfigEntry[String] =
buildConf("authentication.custom.class")
.doc("User define authentication class.")
.version("1.0.0")
Copy link
Member

Choose a reason for hiding this comment

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

1.3.0

@@ -0,0 +1,47 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

Can we rename to xxxSuite?

@hddong hddong force-pushed the support-custom-auth branch from ef7beca to 15c93cc Compare May 25, 2021 02:29
import org.apache.kyuubi.Logging
import org.apache.kyuubi.config.KyuubiConf

class UserDefineAuthenticationProviderImpl(conf: KyuubiConf)
Copy link
Member

Choose a reason for hiding this comment

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

passing conf here seems unnecessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

passing conf here seems unnecessary?

Yes, it's not used. Just want to test constructor. Will remove if needed.

Copy link
Member

@yaooqinn yaooqinn May 25, 2021

Choose a reason for hiding this comment

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

oh, I mean can we make the custom.class implementation a zero-arg constructor? or kyuubiConf is necessary for them to call

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, had address it.

" <li>LDAP: Lightweight Directory Access Protocol authentication.</li></ul>")
.version("1.0.0")
.stringConf
.transform(_.toUpperCase(Locale.ROOT))
.checkValues(AuthTypes.values.map(_.toString))
.createWithDefault(AuthTypes.NONE.toString)

val AUTHENTICATION_CUSTOM_CLASS: OptionalConfigEntry[String] =
buildConf("authentication.custom.class")
.doc("User defined authentication class.")
Copy link
Member

Choose a reason for hiding this comment

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

User-defined authentication implementation of org.apache.kyuubi.service.authentication.PasswdAuthenticationProvider

@yaooqinn
Copy link
Member

Hi @hddong, thanks very much for driving this feature!

BTW, please update the setting.md by running tests locally on your laptop.

Also, we need to update https://kyuubi.readthedocs.io/en/latest/security/authentication.html manually according to this change.

Would you mind if we can have a section right after using-kerberos, e.g. Using Custom Authentication and give an example about how to implement and use this excellent feature?

@hddong
Copy link
Contributor Author

hddong commented May 25, 2021

@yaooqinn : thanks for your review, and of course not.

@codecov-commenter
Copy link

codecov-commenter commented May 25, 2021

Codecov Report

Merging #642 (b4ce0a8) into master (66dcf45) will decrease coverage by 0.05%.
The diff coverage is 70.83%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #642      +/-   ##
============================================
- Coverage     79.87%   79.81%   -0.06%     
- Complexity        0      132     +132     
============================================
  Files           120      120              
  Lines          4671     4692      +21     
  Branches        562      567       +5     
============================================
+ Hits           3731     3745      +14     
- Misses          624      627       +3     
- Partials        316      320       +4     
Impacted Files Coverage Δ
...authentication/AuthenticationProviderFactory.scala 60.86% <58.82%> (-5.80%) ⬇️
...in/scala/org/apache/kyuubi/config/KyuubiConf.scala 94.06% <100.00%> (+0.07%) ⬆️
...he/kyuubi/service/authentication/AuthMethods.scala 100.00% <100.00%> (ø)
...ache/kyuubi/service/authentication/AuthTypes.scala 100.00% <100.00%> (ø)

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 66dcf45...b4ce0a8. Read the comment docs.

@yaooqinn yaooqinn added this to the v1.3.0 milestone May 26, 2021
@yaooqinn yaooqinn closed this in c8b2ec4 May 26, 2021
@yaooqinn
Copy link
Member

Thanks for your first contribution and welcome!

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