feat: adding okta provider #1164

hoyyeva · 2022-03-09T21:57:31Z

Summary

able to add okta provider from scratch
fix some components to be more generic
TODO: we will need to handle the error state (@FSHA)

Checklist

Wrote appropriate unit tests
Considered security implications of the change
Updated associated docs where necessary
Updated associated configuration where necessary
Change is backwards compatible if it needs to be (user can upgrade without manual steps?)
Nothing sensitive logged
Commit message conforms to Conventional Commit

Related Issues

Resolves #1064

BruceMacD · 2022-03-14T14:04:19Z

ui/components/ExitButtn.js

+  }
+`
+
+const ExitButton = () => {


Is the omission of the "o" in this file name intentional?
ExitButtn.js

BruceMacD · 2022-03-14T14:05:35Z

ui/components/ExitButtn.js

+  return (
+    <ExitBtnContainer>
+      <Link href='/'>
+        <img src='/closeIcon.svg' />


Might be better to use nextjs/image in general for us where we are using Next.js:
https://nextjs.org/docs/basic-features/image-optimization

that is a good point. let me create a issue to change all the image to use nextjs/image

BruceMacD · 2022-03-14T14:17:46Z

ui/pages/providers/connect.js

+    <ConnectProviderContainer>
+      <ConnectProviderContent>
+        <Header
+          header='connect identity Providers'


Suggested change

header='connect identity Providers'

header='Connect Identity Providers'

BruceMacD · 2022-03-14T14:18:48Z

ui/pages/providers/setupOkta.js

+const grantAdminAccess = async (userId, accesskey) => {
+  await axios.post('/v1/grants',
+    { identity: userId, resource: 'infra', privilege: 'admin' },
+    { headers: { Authorization: `Bearer ${accesskey}` } })


Might not need to set this header if they logged in using their access key already at this point. That can be in a different change tho, not related to this.

yup, this will be removed. I already created an issue for it!
#1062

jmorganca · 2022-03-14T18:29:17Z

ui/.gitignore

@@ -34,4 +34,4 @@ yarn-error.log*
 .vercel

 # infra binary
-infra
+dist/*/infra


would change to:

infra dist

jmorganca

Great first iteration

Do we need empty files? e.g. ui/components/pages/Access.js

jmorganca · 2022-03-14T19:11:00Z

ui/pages/providers/connect.js

@@ -0,0 +1,76 @@
+import Router from 'next/router'


slug: /providers/add

jmorganca · 2022-03-14T19:11:33Z

ui/components/ExitButtn.js

@@ -0,0 +1,22 @@
+import Link from 'next/link'


Component name: ExitButton ?

jmorganca · 2022-03-14T19:14:51Z

ui/public/connectedArrow.svg

@@ -0,0 +1 @@
+<svg xmlns="http://www.w3.org/2000/svg" width="14" height="6" fill="none" xmlns:v="https://vecta.io/nano"><path opacity=".2" d="M.637 3l5 2.887V.113L.637 3zm13 0l-5-2.887v5.773l5-2.887zm-8.5.5h4v-1h-4v1z" fill="#fff"/></svg>


would use kebab-case instead of camelCase for these file names

jmorganca · 2022-03-14T19:15:16Z

ui/pages/providers/setupOkta.js

@@ -0,0 +1,145 @@
+import { useCallback, useState, useContext } from 'react'


let's make this slug /providers/add/details

i like the name, but then this page is specifically for okta setup. we will probably need to revisit this once we add more providers

jmorganca · 2022-03-14T19:15:26Z

ui/pages/providers/connect.js

@@ -0,0 +1,76 @@
+import Router from 'next/router'


Consider slug: /providers/add/select

jmorganca · 2022-03-14T19:16:14Z

ui/pages/providers/connect.js

+import Header from '../../components/Header'
+
+const ConnectProviderContainer = styled.section`
+  position: relative;


For future: this is an example of where tailwind could yield tighter code: className='relative'

jmorganca · 2022-03-14T19:16:34Z

ui/pages/providers/connect.js

+  right: .5rem;
+`
+
+const IdentitySourceList = styled.section`


Rename IdentitySource (and similar classes) to IdentityProvider

jmorganca · 2022-03-14T19:18:26Z

ui/pages/providers/setupOkta.js

+
+  return (
+    <>
+      <SetupContainer>


Is this component reusable between other flows or should this be AddProviderContainer ?

jmorganca · 2022-03-14T19:27:51Z

ui/pages/providers/setupOkta.js

+  const content = (pageType) => {
+    switch (pageType) {
+      case page.Setup:
+        return <Setup value={value} parentCallback={updateValue} />


Instead of this switch, consider making this 2 separate pages:

/providers/add/details (to input details)

/providers/add/admins (to set admins)

Then the user flow would be in order:

/providers/add/select /providers/add/details /providers/add/admins

are you sure we want to do that? just because this flow is specifically for Okta provider. if we have any new provider, I would assume that the flow and the required setup input would be slightly different?

Oh! what do you think if we can use the dynamic router for different provider? Nextjs supports dynamic routers (something like this: https://nextjs.org/docs/routing/dynamic-routes)

So in this case we will have

/providers/add/okta, /providers/add/google, etc (to input details)

and since admins should be the same page, so we can always direct them to the /providers/add/admins

so the user flow would be something like this:

/providers/add/select [select okta] /providers/add/okta /providers/add/admins /providers/add/select [select google] /providers/add/google /providers/add/admins

This is a fantastic idea!

hoyyeva · 2022-03-15T02:24:07Z

Great first iteration

Do we need empty files? e.g. ui/components/pages/Access.js

No we don't! I was adding them to prepare for working on the dashboard, but then I decided to make each page an actual page instead of components

hoyyeva requested a review from jmorganca March 9, 2022 21:57

hoyyeva force-pushed the add-provider branch from cf4f44b to cc89b9f Compare March 14, 2022 02:51

BruceMacD approved these changes Mar 14, 2022

View reviewed changes

jmorganca reviewed Mar 14, 2022

View reviewed changes

Eva Ho added 12 commits March 14, 2022 16:12

feat: initial provider setup

53e8653

feat: okta init setup page in progress

440d013

feat: adding provider in progress

5469059

feat: ui adding provider flow done, connecting with server side next

a242c54

feat: cleaning up

e9f4066

feat: connecting with server in progress

a6299d8

feat: able to create provider

e043d47

fix: run standard

e5b55d7

feat: connecting server endpoints

bfa70a2

feat: adding grant access

0780a45

fix: removing console and update igtignore

4c6432a

fix: address comments

383a648

hoyyeva force-pushed the add-provider branch from cc89b9f to 383a648 Compare March 14, 2022 22:23

hoyyeva requested a review from jmorganca March 14, 2022 22:29

fix: address comment

97d82d8

Eva Ho added 3 commits March 14, 2022 22:25

fix: remove empty pages

4facac6

feat: using dynamic router to setup providers

99f3d6b

fix: remove unused file

a895752

jmorganca approved these changes Mar 15, 2022

View reviewed changes

hoyyeva merged commit 4880990 into main Mar 15, 2022

hoyyeva deleted the add-provider branch March 15, 2022 15:02

infrahq-ci mentioned this pull request Mar 15, 2022

chore(main): release 0.5.12 #1182

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: adding okta provider #1164

feat: adding okta provider #1164

hoyyeva commented Mar 9, 2022

BruceMacD Mar 14, 2022

BruceMacD Mar 14, 2022

hoyyeva Mar 14, 2022

BruceMacD Mar 14, 2022

BruceMacD Mar 14, 2022

hoyyeva Mar 14, 2022

jmorganca Mar 14, 2022

jmorganca left a comment

jmorganca Mar 14, 2022

jmorganca Mar 14, 2022

jmorganca Mar 14, 2022

jmorganca Mar 14, 2022

hoyyeva Mar 14, 2022

jmorganca Mar 14, 2022

jmorganca Mar 14, 2022

jmorganca Mar 14, 2022

jmorganca Mar 14, 2022

jmorganca Mar 14, 2022 •

edited

hoyyeva Mar 14, 2022

hoyyeva Mar 15, 2022 •

edited

jmorganca Mar 15, 2022

hoyyeva commented Mar 15, 2022

	header='connect identity Providers'
	header='Connect Identity Providers'

		@@ -0,0 +1 @@
		<svg xmlns="http://www.w3.org/2000/svg" width="14" height="6" fill="none" xmlns:v="https://vecta.io/nano"><path opacity=".2" d="M.637 3l5 2.887V.113L.637 3zm13 0l-5-2.887v5.773l5-2.887zm-8.5.5h4v-1h-4v1z" fill="#fff"/></svg>

		@@ -0,0 +1,145 @@
		import { useCallback, useState, useContext } from 'react'

feat: adding okta provider #1164

feat: adding okta provider #1164

Conversation

hoyyeva commented Mar 9, 2022

Summary

Checklist

Related Issues

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmorganca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmorganca Mar 14, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoyyeva Mar 15, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoyyeva commented Mar 15, 2022

jmorganca Mar 14, 2022 •

edited

hoyyeva Mar 15, 2022 •

edited