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 #6079] Web UI support Basic authN #6258

Closed
wants to merge 6 commits into from

Conversation

beryllw
Copy link
Contributor

@beryllw beryllw commented Apr 4, 2024

🔍 Description

Issue References 🔗

This pull request fixes #6079

Describe Your Solution 🔧

image

zpHy0SGauD

image

Types of changes 🔖

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Test Plan 🧪


Checklist 📝

Be nice. Be informative.

@pan3793
Copy link
Member

pan3793 commented Apr 4, 2024

the frontend code needs to be modified so that all http request will attach a basic auth header

@beryllw
Copy link
Contributor Author

beryllw commented Apr 7, 2024

the frontend code needs to be modified so that all http request will attach a basic auth header

frontend code means web-ui code?
If user login success, other http request will auto attach a basic auth header.
@pan3793 cc

@codecov-commenter
Copy link

codecov-commenter commented Apr 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 58.45%. Comparing base (c8a40d9) to head (7a90286).
Report is 18 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #6258      +/-   ##
============================================
+ Coverage     58.42%   58.45%   +0.03%     
  Complexity       24       24              
============================================
  Files           651      652       +1     
  Lines         39513    39607      +94     
  Branches       5441     5450       +9     
============================================
+ Hits          23086    23154      +68     
- Misses        13954    13976      +22     
- Partials       2473     2477       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pan3793
Copy link
Member

pan3793 commented Apr 8, 2024

If user login success, other http request will auto attach a basic auth header.

interesting, let me verify it locally

@pan3793
Copy link
Member

pan3793 commented Apr 8, 2024

found 2 issues:

  • if user input the wrong username/password, no way to correct
  • only SQL editor works

@beryllw beryllw changed the title [KYUUBI #4079] Web UI support Basic authN [KYUUBI #6079] Web UI support Basic authN Apr 8, 2024
@beryllw
Copy link
Contributor Author

beryllw commented Apr 8, 2024

  • if user input the wrong username/password, no way to correct

Fixing this will change the current authentication logic, i will try to fix it soon.
We need a new Exception class (different from AuthenticationException) to distinguish between authentication failure and forbidden access.

For now, users can correct user and password by clearing history.

  • only SQL editor works

Other api return http status code 405? Because the current user is not an administrator.
And set kyuubi conf kyuubi.server.administrators=Set('user') can resolve this.
image

org.apache.kyuubi.server.api.v1.AdminResource#sessions
    if (!fe.isAdministrator(userName)) {
      throw new NotAllowedException(
        s"$userName is not allowed to list all live sessions")
    }

@pan3793
Copy link
Member

pan3793 commented Apr 8, 2024

Other api return http status code 405? Because the current user is not an administrator.

Oh, I see ...

@beryllw
Copy link
Contributor Author

beryllw commented Apr 9, 2024

@pan3793 cc

@pan3793
Copy link
Member

pan3793 commented Apr 9, 2024

seems a explicitly logout button is required, otherwise user can not switch login user

@beryllw
Copy link
Contributor Author

beryllw commented Apr 9, 2024

seems a explicitly logout button is required, otherwise user can not switch login user

https://stackoverflow.com/questions/233507/how-to-log-out-user-from-web-site-using-basic-authentication

What you have to do is have the user click a logout link, and send a ‘401 Unauthorized’ in response, using the same realm and at the same URL folder level as the normal 401 you send requesting a login.

Simple implementation is that when the user clicks the logout button, the server returns 401.

@pan3793
Copy link
Member

pan3793 commented Apr 10, 2024

Simple implementation is that when the user clicks the logout button, the server returns 401.

We can use a specific endpoint for that, so other endpoints behavior won't be affected

@beryllw
Copy link
Contributor Author

beryllw commented Apr 10, 2024

Simple implementation is that when the user clicks the logout button, the server returns 401.

Login users cannot be displayed in this way.Implemented on the frontend code instead.
Please help to review the code. @pan3793 cc
image
zpHy0SGauD
image

support input valid
image

@pan3793 pan3793 added this to the v1.9.1 milestone Apr 11, 2024
@pan3793 pan3793 closed this in 8bfae02 Apr 11, 2024
pan3793 pushed a commit that referenced this pull request Apr 11, 2024
# 🔍 Description
## Issue References 🔗

This pull request fixes #6079

## Describe Your Solution 🔧

<img width="1160" alt="image" src="https://github.com/apache/kyuubi/assets/25627922/eef26a0e-478a-4e65-a0a4-629e0afa9a52">

![zpHy0SGauD](https://github.com/apache/kyuubi/assets/25627922/176d4436-509b-406a-984b-8eb0dab9698e)

![image](https://github.com/apache/kyuubi/assets/25627922/3d637718-b177-48b2-bd6a-9ec8065b8b9c)

## Types of changes 🔖

- [ ] Bugfix (non-breaking change which fixes an issue)
- [x] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)

## Test Plan 🧪

---

# Checklist 📝

- [ ] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html)

**Be nice. Be informative.**

Closes #6258 from beryllw/kyuubi-6079.

Closes #6079

7a90286 [wangjunbo] support input valid
6e2093e [wangjunbo] format code
a781c0a [wangjunbo] add License
788bdfa [wangjunbo] [KYUUBI #6079] Web UI support Basic authN
97772e5 [wangjunbo] [KYUUBI #6079] Web UI support Basic authN
5560d4f [wangjunbo] [KYUUBI #6079] Web UI support Basic authN

Authored-by: wangjunbo <[email protected]>
Signed-off-by: Cheng Pan <[email protected]>
(cherry picked from commit 8bfae02)
Signed-off-by: Cheng Pan <[email protected]>
@pan3793
Copy link
Member

pan3793 commented Apr 11, 2024

Thanks, merged to master/1.9.1

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

Successfully merging this pull request may close these issues.

[TASK][MEDIUM] Web UI support Basic authN
3 participants