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

Configure webui login URLs and details dynamically #5093

Merged
merged 8 commits into from
Feb 6, 2023

Conversation

arielshaqed
Copy link
Contributor

Fixes #5085.

@arielshaqed arielshaqed marked this pull request as draft January 25, 2023 11:23
@arielshaqed arielshaqed changed the base branch from master to chore/deprecate-oidc-login January 25, 2023 11:24
@arielshaqed arielshaqed force-pushed the chore/deprecate-oidc-login branch from 8b620d7 to 794d435 Compare January 25, 2023 11:26
Base automatically changed from chore/deprecate-oidc-login to master January 25, 2023 11:57
@arielshaqed arielshaqed added new-feature Issues that introduce new feature or capability area/UI Improvements or additions to UI area/auth IAM, authorization, authentication, audit, AAA, and integrations with all those include-changelog PR description should be included in next release changelog team/cloud-native Team cloud native labels Jan 25, 2023
@arielshaqed arielshaqed force-pushed the feature/5085-configurable-ui-login branch 3 times, most recently from 18c2fd9 to cb70cca Compare January 25, 2023 15:39
@arielshaqed
Copy link
Contributor Author

arielshaqed commented Jan 25, 2023

Manual verifications performed:

  • Nothing configured in login_config, everything works normally.
  • OIDC configured and enabled, everything continues to work. (But this
    option WILL go away shortly!)
  • login_config configured, all fields work. Full end-to-end tests will
    have to wait until we have an alternative login controller service, but
    we can already validate the UI works according to the login_config that
    it receives from lakeFS.

@arielshaqed
Copy link
Contributor Author

Validation with OIDC configuration

I used this configuration snippet:

  oidc:
    enabled: true
    url: https://XXX.YYY.IDP.com/   # elided
    client_id: abcdefg
    client_secret: hijklmnoprshhhhhh
    callback_base_url: http://localhost:8000/

and login still works.

@arielshaqed
Copy link
Contributor Author

Validation of ui_config.login_failed_message

Added this snippet:

  ui_config:
    login_failed_message: That didn't work.  Does a link to <a href="https://cnn.com">CNN</a> work?
    # ...

Failed to login, and got a text with a link to CNN.

@arielshaqed
Copy link
Contributor Author

Validation of fallback configuration

With this configuration:

  ui_config:
    fallback_login_url: https://login.example.com
    fallback_login_label: |
      Didn't work?  Maybe try SSL Stone Souper Login, "just add water  and login!"

the fallback button has the desired label and tries to redirect to the
server.

@arielshaqed
Copy link
Contributor Author

arielshaqed commented Jan 25, 2023

Validation of login redirection

With this configuration:

  ui_config:
    login_url: https://another.example.com/go/login?secret=1234

the URL localhost:8000/auth/login?redirected=true&next=%2Frepositories
redirects to the pretend login server.

Note that it is only relevant when redirected=true; we never go directly
to a login URL. @johnnyaug amirite?

@johnnyaug
Copy link
Contributor

johnnyaug commented Jan 26, 2023

Note that it is only relevant when redirected=true; we never go directly
to a login URL. @johnnyaug amirite?

@arielshaqed, Yes, this is to avoid jumping to this URL when the user explicitly navigates to the "internal" login page. It may or may not still be relevant today.

Copy link
Contributor

@johnnyaug johnnyaug left a comment

Choose a reason for hiding this comment

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

Cool! Some comments, nothing major.

api/swagger.yml Outdated
description: label to place on fallback_login_url.
type: string
login_cookies:
description: cookies to store JWT
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
description: cookies to store JWT
description: cookie names used to store JWT

Copy link
Contributor

Choose a reason for hiding this comment

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

Possibly also change the property name to login_cookie_names. Not feeling too strongly about this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Done (60d1393bf9e319cdf6893ae6406a92cb7748be1f).

Comment on lines 3497 to 3498
// TODO(ariels): Configure by c.Auth.OIDC.Enabled if set, otherwise
// from C.Auth.UIConfig
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this TODO is already done?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup 😳

Comment on lines 3508 to 3509
defaultFallbackLoginURL = "/oidc/login?prompt=login"
defaultFallbackLoginLabel = "Sign in with SSO provider"
Copy link
Contributor

Choose a reason for hiding this comment

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

These are only used when OIDC is enabled, so they are not really default. Can hard-code them in the if below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks!

Comment on lines 3500 to 3506
cookies = c.Auth.UIConfig.LoginCookies
// []string{
// "internal_auth_session",
// }
loginFailedMessage = c.Auth.UIConfig.LoginFailedMessage // "The credentials don't match."
fallbackLoginURL = c.Auth.UIConfig.FallbackLoginURL // nil
fallbackLoginLabel = c.Auth.UIConfig.FallbackLoginLabel // nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Are the comments here the default values? Because I don't see them set anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Made these true defaults.

@@ -10,9 +10,18 @@ import {Error} from "../../lib/components/controls"
import {useRouter} from "../../lib/hooks/router";
import {useAPI} from "../../lib/hooks/api";

const OIDC_LOGIN_URL = "/oidc/login?prompt=login";
//const OIDC_LOGIN_URL = "/oidc/login?prompt=login";
Copy link
Contributor

Choose a reason for hiding this comment

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

/dd?

pkg/api/controller.go Outdated Show resolved Hide resolved
pkg/api/controller.go Show resolved Hide resolved
Copy link
Contributor

@Isan-Rivkin Isan-Rivkin left a comment

Choose a reason for hiding this comment

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

Nice work! left some comments, specifically regarding logout

pkg/api/controller.go Show resolved Hide resolved
@arielshaqed arielshaqed marked this pull request as ready for review February 5, 2023 10:20
@arielshaqed
Copy link
Contributor Author

Thanks!

PTAL... (Will rebase on trunk after, to keep ease of commenting on this PR)

Copy link
Contributor Author

@arielshaqed arielshaqed left a comment

Choose a reason for hiding this comment

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

THANKS!

PTAL...

api/swagger.yml Outdated
description: label to place on fallback_login_url.
type: string
login_cookies:
description: cookies to store JWT
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Done (60d1393bf9e319cdf6893ae6406a92cb7748be1f).

pkg/api/controller.go Show resolved Hide resolved
Comment on lines 3497 to 3498
// TODO(ariels): Configure by c.Auth.OIDC.Enabled if set, otherwise
// from C.Auth.UIConfig
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup 😳

pkg/api/controller.go Outdated Show resolved Hide resolved
Comment on lines 3508 to 3509
defaultFallbackLoginURL = "/oidc/login?prompt=login"
defaultFallbackLoginLabel = "Sign in with SSO provider"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks!

Comment on lines 3500 to 3506
cookies = c.Auth.UIConfig.LoginCookies
// []string{
// "internal_auth_session",
// }
loginFailedMessage = c.Auth.UIConfig.LoginFailedMessage // "The credentials don't match."
fallbackLoginURL = c.Auth.UIConfig.FallbackLoginURL // nil
fallbackLoginLabel = c.Auth.UIConfig.FallbackLoginLabel // nil
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Made these true defaults.

pkg/api/controller.go Show resolved Hide resolved
@arielshaqed arielshaqed force-pushed the feature/5085-configurable-ui-login branch from 0ecec3a to 38dd55b Compare February 5, 2023 15:52
Copy link
Contributor

@Isan-Rivkin Isan-Rivkin left a comment

Choose a reason for hiding this comment

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

LGTM Great work!

@arielshaqed
Copy link
Contributor Author

arielshaqed commented Feb 6, 2023

Reverify after all changes: unchecked above boxes, re-checking now before pulling.

Manual verifications performed:

  • Nothing configured in login_config, everything works normally.

  • OIDC configured and enabled, everything continues to work. (But this
    option WILL go away shortly!)

  • login_config configured, all fields work. Full end-to-end tests will
    have to wait until we have an alternative login controller service, but
    we can already validate the UI works according to the login_config that
    it receives from lakeFS.

@arielshaqed
Copy link
Contributor Author

  • login_config configured, all fields work. Full end-to-end tests will
    have to wait until we have an alternative login controller service, but
    we can already validate the UI works according to the login_config that
    it receives from lakeFS.

Everything works except for logout_url. Opening a separate issue #5195 to support that, as requirements will be part of building a complete system. @Isan-Rivkin FYI, please add a comment there about actual requirements :-)

@arielshaqed
Copy link
Contributor Author

Thanks! Pulling...

Only required check not reported is license/cla. But it's me, guv, honest! So pulling by adminiforce.

1 "expected" test is license/cla, not run for reasons unknown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/auth IAM, authorization, authentication, audit, AAA, and integrations with all those area/UI Improvements or additions to UI include-changelog PR description should be included in next release changelog new-feature Issues that introduce new feature or capability team/cloud-native Team cloud native
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optionally configure log{in,out} URLs and cookies in web UI
3 participants