Support lakectl login: client, controller stub
#9644
Conversation
|
📚 Documentation preview at https://pr-9644.docs-lakefs-preview.io/ (Updated: 11/23/2025, 12:14:48 PM - Commit: 6760013) |
arielshaqed
added
area/UI
arielshaqed
force-pushed
the
feature/1194-lakectl-login-swagger
branch
from
2d941db to
4121fb5
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR introduces browser-based token authentication functionality for lakeFS, enabling users to log in via a web browser and obtain authentication tokens without requiring manual credential input.
Key changes:
- Implements a new login token provider interface and three new API endpoints for browser-based authentication flow
- Adds a new
lakectl logincommand that opens a browser for authentication - Refactors HTTP header setting by introducing a
KeepPrivate()utility function for security headers
Reviewed Changes
Copilot reviewed 39 out of 44 changed files in this pull request and generated 25 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/authentication/tokens.go | Defines the LoginTokenProvider interface for browser-based token authentication |
| pkg/authentication/internalidp/jwt_auth.go | Implements JWT authentication client for login token flow |
| pkg/api/controller.go | Adds three new controller methods for the token authentication endpoints and refactors security headers |
| pkg/httputil/response.go | Introduces KeepPrivate() helper function to set security headers |
| modules/authentication/factory/login_token.go | Provides unimplemented login token provider factory |
| cmd/lakectl/cmd/login.go | Implements new lakectl login command for browser-based authentication |
| cmd/lakectl/cmd/root.go | Adds JWT authentication support and login retry configuration |
| cmd/lakectl/cmd/retry_client.go | Refactors retry logic to be more configurable |
| go.mod, go.sum | Updates dependencies (deckarep/golang-set, matoous/go-nanoid, elnormous/contenttype) |
| api/swagger.yml | Defines OpenAPI specification for the three new authentication endpoints |
| docs/src/reference/cli.md, esti/golden/lakectl_help.golden | Adds documentation for the new login command |
| clients/* | Auto-generated client code for Rust, Python, and Java SDKs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/authentication/tokens.go
Outdated
| // called authenticated, initiated by the web browser running. | ||
| Release(ctx context.Context, loginRequestToken string) error | ||
| // GetToken returns a token waiting on mailbox. It is called unauthenticated, initiated | ||
| // the requesting client, with no user on the context.. |
There was a problem hiding this comment.
Double period at the end of the comment. Should be a single period.
| if r.Header.Get("Accept") == "text/plain" { | ||
| w.Header().Set("Content-Type", "text/plain; charset=utf-8") | ||
| w.Header().Set("X-Content-Type-Options", "nosniff") | ||
| httputil.KeepPrivate(w) |
There was a problem hiding this comment.
The Content-Type header should be set in addition to the other security headers, not replaced by KeepPrivate(). The usage report endpoint expects text/plain; charset=utf-8 content type which is now missing.
pkg/api/controller.go
Outdated
| err = releasedTokenTemplate.ExecuteTemplate(w, "releasedToken", &UserData{Username: username}) | ||
| if c.handleAPIError(ctx, w, r, err) { | ||
| return | ||
| } | ||
|
|
||
| w.WriteHeader(http.StatusOK) |
There was a problem hiding this comment.
The WriteHeader call should come before writing the response body, not after. The template execution writes to the response, so WriteHeader should be called before ExecuteTemplate.
| } else if ( localBasePaths.length > 0 ) { | ||
| basePath = localBasePaths[localHostIndex]; |
There was a problem hiding this comment.
Test is always false.
| } else if ( localBasePaths.length > 0 ) { | |
| basePath = localBasePaths[localHostIndex]; |
| } else if ( localBasePaths.length > 0 ) { | ||
| basePath = localBasePaths[localHostIndex]; |
There was a problem hiding this comment.
Test is always false.
| } else if ( localBasePaths.length > 0 ) { | |
| basePath = localBasePaths[localHostIndex]; |
| " to method get_token_from_mailbox" % _key | ||
| ) | ||
| _params[_key] = _val | ||
| del _params['kwargs'] |
There was a problem hiding this comment.
Modification of the locals() dictionary will have no effect on the local variables.
| "Got an unexpected keyword argument '%s'" | ||
| " to method get_token_redirect" % _key | ||
| ) | ||
| _params[_key] = _val |
There was a problem hiding this comment.
Modification of the locals() dictionary will have no effect on the local variables.
| " to method get_token_redirect" % _key | ||
| ) | ||
| _params[_key] = _val | ||
| del _params['kwargs'] |
There was a problem hiding this comment.
Modification of the locals() dictionary will have no effect on the local variables.
| "Got an unexpected keyword argument '%s'" | ||
| " to method release_token_to_mailbox" % _key | ||
| ) | ||
| _params[_key] = _val |
There was a problem hiding this comment.
Modification of the locals() dictionary will have no effect on the local variables.
| " to method release_token_to_mailbox" % _key | ||
| ) | ||
| _params[_key] = _val | ||
| del _params['kwargs'] |
There was a problem hiding this comment.
Modification of the locals() dictionary will have no effect on the local variables.
arielshaqed
left a comment
arielshaqed
left a comment
There was a problem hiding this comment.
Thanks!
PTAL...
pkg/api/controller.go
Outdated
| err = releasedTokenTemplate.ExecuteTemplate(w, "releasedToken", &UserData{Username: username}) | ||
| if c.handleAPIError(ctx, w, r, err) { | ||
| return | ||
| } | ||
|
|
||
| w.WriteHeader(http.StatusOK) |
| if r.Header.Get("Accept") == "text/plain" { | ||
| w.Header().Set("Content-Type", "text/plain; charset=utf-8") | ||
| w.Header().Set("X-Content-Type-Options", "nosniff") | ||
| httputil.KeepPrivate(w) |
pkg/authentication/tokens.go
Outdated
| // called authenticated, initiated by the web browser running. | ||
| Release(ctx context.Context, loginRequestToken string) error | ||
| // GetToken returns a token waiting on mailbox. It is called unauthenticated, initiated | ||
| // the requesting client, with no user on the context.. |
- OpenAPI support - Login tokens abstraction - Controller hookup to login tokens abstraction (This feature is unimplemented in base lakeFS, and only a trivial login tokens abstraction exists here.)
This is typically only called by the browser -- but it's still handled as OpenAPI in the controller.
Use the same RetryClient type as the rest of lakeFS, only with a different retry policy - one that retries status code 404. This involves refactoring getClient... so do that.
releaseToken is _not_ part of the UI, and there is no implicit redirection there from middleware. Instead, redirect there from the controller.
arielshaqed
force-pushed
the
feature/1194-lakectl-login-swagger
branch
from
bdb4f20 to
58a254b
Compare
pkg/api/controller.go
Outdated
| WithField("accept", r.Header.Get("Accept")). | ||
| Debug("Failed to get user - redirect to login") | ||
| redirectURL := url.URL{ | ||
| Path: "/auth/login", |
There was a problem hiding this comment.
Suggestion:
Instead of /auth/login i think you should use whatever the ui uses to login.
Why now:
because lakectl login means user don't have credentials, taking them to /auth/login page specifically will enable using static credentials but that's the problem: they don't have them.
If we redirect to login flow of the browser users then it will actually enable SSO.
How
- Use the same value for login url we pass to the ui (returned under setup_lakefs GET endpoitn in the UI) and that's the path the ui will use to redirect to login page.
There was a problem hiding this comment.
True, that's always at least as good, and in any nontrivial configuration it should be even better.
Confusingly, the config option with this default value is ui_config.logout_redirect_url 🤦🏽 , and ui_config.login_url is usually blank. Contacting you directly to decide which really to do, and for now doing ui_config.login_url if set, otherwise /auth/login.
pkg/api/controller.go
Outdated
| redirectURL := url.URL{ | ||
| Path: "/auth/login", | ||
| // TODO(ariels): Use a relative URI? | ||
| RawQuery: fmt.Sprintf("next=%s", url.QueryEscape(r.URL.String())), |
There was a problem hiding this comment.
Blocking:
I think this is broken in case the user not already logged in.
For example try:
- logout in browser
- call lakectl login
- browser pops-up > insert correct credentials
- user redirected to /auth/login but is never going to
next - lakectl keeps polling and fails eventually
Another example is simply copy the lakectl url and use in incognito mode, same result.
Authentication changed in #9593. This broke the ability to redirect to a non-React URL after logging in -- which @Isan-Rivkin discovered broke `lakectl login`. Restore the ability to go to the particular route needed under /api/v1. Checked by re-logging-in.
arielshaqed
left a comment
arielshaqed
left a comment
There was a problem hiding this comment.
Thanks for a very thorough review! Each comment could break login in some cases :-/ . Indeed cb3411490 fixes a breakage that was introduced in #9593 😿
PTAL.
pkg/api/controller.go
Outdated
| WithField("accept", r.Header.Get("Accept")). | ||
| Debug("Failed to get user - redirect to login") | ||
| redirectURL := url.URL{ | ||
| Path: "/auth/login", |
There was a problem hiding this comment.
True, that's always at least as good, and in any nontrivial configuration it should be even better.
Confusingly, the config option with this default value is ui_config.logout_redirect_url 🤦🏽 , and ui_config.login_url is usually blank. Contacting you directly to decide which really to do, and for now doing ui_config.login_url if set, otherwise /auth/login.
|
@arielshaqed |
Isan-Rivkin
left a comment
Isan-Rivkin
left a comment
There was a problem hiding this comment.
tested together f2f, asking for changes so that i get notified
It's a query param that contains "/" and ":" and things - encode it as such!
|
Thanks! I retested against an Enterprise version:
We will verify OIDC later, as agreed. PULLING, thanks for the great reviews! |
| type: string | ||
| description: GET the token from this mailbox. Keep the mailbox SECRET! | ||
| 401: | ||
| $ref: "#/components/responses/Unauthorized" |
There was a problem hiding this comment.
Do you also need the 404 case here?
| description: token released | ||
| 401: | ||
| description: bad token or user has not logged in yet | ||
| $ref: "#/components/responses/Unauthorized" |
There was a problem hiding this comment.
Do you also need the 404 case here?
| } | ||
| q := redirectURL.Query() | ||
| q.Set("next", r.URL.String()) // Encode query-escapes this string. | ||
| redirectURL.RawQuery = q.Encode() |
There was a problem hiding this comment.
I figured out the OIDC redirection issue we were seeing.
The code that works for me is:
redirectURL := url.URL{
Path: "/auth/login",
RawQuery: fmt.Sprintf("redirected=true&next=%s", url.QueryEscape(r.URL.String())),
}
instead of:
redirectURL, err := url.Parse(c.Config.AuthConfig().GetLoginURL())
if c.handleAPIError(ctx, w, r, err) {
return
}
q := redirectURL.Query()
q.Set("next", r.URL.String()) // Encode query-escapes this string.
redirectURL.RawQuery = q.Encode()
So c.Config.AuthConfig().GetLoginURL() is no longer needed here.
Why this fixes the OIDC redirection issue?
The problem was that ReleaseTokenToMailbox was redirecting directly to the OIDC login URL (for example login_url: http://localhost:8000/oidc/login), which bypassed our React auth flow.
Our React auth flow does the following:
- When an unauthenticated user hits a protected endpoint, it:
- Stores the requested endpoint in sessionStorage and state. So the user is redirected back correctly after authentication.
- Redirects to /auth/login.
- /auth/login is a the single source of truth for login (via the loginStrategy as can be seen in the webui/src/pages/auth/login.tsx I added recently).
It decides whether to:
- Show the local login form, or
- Redirect to the SSO login URL.
The decision to redirect to SSO is based on theredirected=truequery param: - The login strategy is called if /auth/login was called with redirected=true.
- If so, and an SSO login URL is configured, it redirects to the SSO login URL from there.
By redirecting directly to the OIDC login URL from the server, we were skipping this React-based logic and breaking out of the flow that manages next and session state. That’s why we need to redirect to /auth/login instead, and let the frontend handle:
- Whether to go to SSO or local login.
- Where to redirect back to after authentication.
Note: currently in the SSO flow we don’t pass next as a query param to the IdP; we rely on sessionStorage and state, and the AuthProvider reads from there to navigate back after authentication.
(/auth/login should be const in my code)
Tested in Enterprise with the following configurations: local auth, LDAP, and OIDC - it redirects correctly with this code for all of them.
There was a problem hiding this comment.
@arielshaqed, I had no knowledge of this behavior that if we go through /auth/login while query param redirected=true it'll redirect you to the correct login_url (i.e sso oidc saml) if you are not logged in.
I tested and inspected the code, @Annaseli is right 👏 !
@Annaseli I tested your suggestion and I can confirm as well, it works for both OIDC and SAML out of the box (While current fix only applies to SAML and does not work with OIDC, verified it by me).
The changes I applied (based on anna's suggestion):
redirectURL := url.URL{
Path: "/auth/login",
RawQuery: fmt.Sprintf("redirected=true&next=%s", url.QueryEscape(r.URL.String())),
}And the redirect logic just works.
I think it's worth pushing a fix for this before the release.
@Annaseli , thank you for being on top of this.
There was a problem hiding this comment.
But I will use URL.Query rather than edit RawQuery directly, which is nonscalable and fails easily.
| // Break out of React to the actual URL - do not use Navigate. | ||
| useEffect(() => { | ||
| window.location.replace(location.pathname); | ||
| }, [location.pathname]); |
There was a problem hiding this comment.
I suggest adding the fullPath, in case we need location.search and location.hash in the path future.
useEffect(() => {
const fullPath = location.pathname + location.search + location.hash;
window.location.replace(fullPath);
}, [location.pathname, location.search, location.hash]);
|
|
||
| return <div>Redirecting...</div>; | ||
| }; | ||
|
|
There was a problem hiding this comment.
Could we move the Redirect component to lakefs-oss/webui/src/lib/components so that webui/src/pages/index.jsx is used only for route definitions?
| </Route> | ||
| <Route path="*" element={<Navigate to="/repositories" replace/>}/> | ||
| <Route path="api/v1/auth/get-token/release-token/*" element={<Redirect />}/> | ||
| <Route path="*" element={<Navigate to="/repositories" replace/>}/> |
There was a problem hiding this comment.
For readability, can we move
<Route path="*" element={<Navigate to="/repositories" replace />} />
so it appears directly under
<Route path="api/v1/auth/get-token/release-token/*" element={<Redirect />} />
This will make it clearer that it’s inside the block:
<Route element={<RequiresAuth />} >
|
@Annaseli I think @Isan-Rivkin asked for the exact opposite here - the redirect URL is sometimes used at customer installations, so we cannot ignore it. In any case this is the default value AFAIU - does avoiding it matter so much? Or, is the important bit the |
the |
|
@arielshaqed But if I then log out from the WebUI, I can still run lakectl repo list and get a response instead of a 401. |
This is actually per spec, and also a limitation. Logging out of the web UI "just" drops a JWT that the web UI received. It does nothing to the JWT that lakectl received. So you can continue to use it. Of course, you cannot renew that token without logging into the web - you have no JWT there. We could add a |
3 participants
Add support for
lakectl login:In order actually to use this code, however, you will need an implementation of a LoginTokenProvider. Currently only lakeFS Enterprise provides this.
Closes treeverse/lakefs-enterprise#1194, treeverse/lakefs-enterprise#1196.