Skip to content
This repository has been archived by the owner on Feb 8, 2024. It is now read-only.

Read localAuth config from backend #44

Merged
merged 8 commits into from
Mar 9, 2020
Merged

Conversation

kimlisa
Copy link
Contributor

@kimlisa kimlisa commented Mar 7, 2020

Fixes issue gravitational/teleport#2789
Part of PR gravitational/teleport#3412

  • Add a getter in teleport/config to get getLocalAuth flag. Set to default true.
  • Login.jsx gets and sets the isLocalAuthEnabled prop
  • FormLogin.jsx receives isLocalAuthEnabled prop (defaulted to true)
  • Unit tested flag in FormLogin.test.js

Various states in FormLogin:

If local auth is disabled, only SSO providers are rendered:
Screenshot from 2020-03-06 18-24-34

If local auth is disabled, but no SSO providers are provided:
Screenshot from 2020-03-09 11-51-06

If local auth is enabled (default) and SSO providers are provided, renders everything:
Screenshot from 2020-03-06 18-21-44

Lisa Kim added 4 commits March 6, 2020 19:48
- Render input and login elements based on isLocalAuthEnabled flag
- Render error message if local auth is disabled but no SSO provider is configured
@kimlisa kimlisa requested a review from alex-kovoy March 7, 2020 04:02
@kimlisa kimlisa changed the title Lisa/handle local auth Read localAuth config Mar 7, 2020
@kimlisa kimlisa changed the title Read localAuth config Read localAuth config from backend Mar 7, 2020
/>
{otpEnabled && (
<Flex flexDirection="row">
{!isLocalAuthEnabled && !ssoEnabled && (
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for this error here (why error btw?).
For now, lets do nothing in this case. We will add a text placeholder for the empty state in another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I must've misunderstood our communication.

removed

@@ -102,6 +102,10 @@ const cfg = {
return cfg.auth ? cfg.auth.second_factor : null;
},

getLocalAuthFlag() {
return cfg.auth && cfg.auth.localAuth ? cfg.auth.localAuth : true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets add a default value to config.js for this property and then

return cfg.auth.localAuth;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

revised

Comment on lines 214 to 220
expect(getByText(/github/i)).toBeInTheDocument();
expect(getByText(/google/i)).toBeInTheDocument();

expect(queryByText(/username/i)).not.toBeInTheDocument();
expect(queryByText(/password/i)).not.toBeInTheDocument();
expect(queryByText(/two factor token/i)).not.toBeInTheDocument();
expect(queryByText(/login/i)).not.toBeInTheDocument();
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets test the rendering part using snapshot testing by creating FormLogin.story.test.js. You can check out the FormInvite.story.test.js for example.

And lets keep only functional tests inside FormLogin.test.js (this is when we test login click callbacks, etc).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

revised

@kimlisa kimlisa merged commit 83f8c89 into master Mar 9, 2020
@kimlisa kimlisa deleted the lisa/handle-localAuth branch March 9, 2020 21:37
@benarent
Copy link
Contributor

Congrats on your Teleport Feature PR @kimlisa 🎉

My one observation, would be that in this case.. we should change the copy, since a header to instruct people to 'Sign into Teleport' is inaccurate.

image

I would propose a better and more friendly user message would be replace Sign into Teleport with

SSO Access hasn’t been configured for this cluster.

If you think this an error please contact your administrator
or check Teleport logs

I know this is close, let me know if I should create this as a new ticket in Teleport or if you would like this issue created here?

@kimlisa
Copy link
Contributor Author

kimlisa commented Mar 11, 2020

@benarent I think Alexey wanted this to be another PR. Pinging @alex-kovoy

pierrebeaucamp pushed a commit that referenced this pull request Mar 26, 2020
…/deployment-updates

* 'master' of github.com:gravitational/webapps: (27 commits)
  [teleport] Receive and display nodeCount and publicURL in cluster table (#52)
  Remove unused imports from makeEvent.ts
  Typescript migration (#51)
  [Teleport] Prompt user with a confirmation window for session tabs (#49)
  Refactor tabs creation to a separate hook and add unit-tests (#50)
  Do not rerender in-active document (#47)
  regenerate dist files
  New Terminal (#46)
  Unit test rest of Dialog*.jsx and TopNav*.jsx (#45)
  Read localAuthEnabled config from backend (#44)
  Unit Test Popover (#43)
  Unit test teleport/Login (#40) closes #39
  Test rendering of SideNav, SideNavItem, SideNavItemIcon (#41)
  Unit test featureBase (#38)
  Unit test useStore (#37)
  Unit test FormPassword (#36)
  Unit test FormLogin (#35)
  Unit test FieldSelect (#34)
  Test useRule unsubscribe behavior and some cleanup (#33)
  Unit test FieldInput (#32)
  ...
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants