feat: UI Auth/Login #41

markphelps · 2023-01-19T21:42:06Z

Adds OIDC login support to the UI 🎉

Fixes: FLI-70

Supported by: flipt-io/flipt#1279

DEMO: https://imgur.com/a/jUHFHJO

Auth Checks

We query if we have a 'session' object stored in browser localstorage (flipt) via the new SessionProvider and useStorage / useSession hooks
If we do, then we are either logged in, or no login is required
If we don't, then we hit the /meta/info endpoint on the API. if the API returns an error (likely 401), then auth is required, so we'll redirect to the /login page
If no auth is required, then we'll get a response from /meta/info and store that in the session and continue on

Login

If auth is required (if we redirect the user to the login page), then we load the list of available OIDC providers (we only support OIDC for now, in the future this will need some refactoring to support other methods)
We render the buttons in the UI, checking for any known providers with prettier display names and icons
On click the buttons make the request to the authorizationURL configured in the Flipt Server
The user then performs the OIDC dance, on success it calls back to our API, passing along the token for us to store in our DB and then set the appropriate cookies in the browser

API Calls

All API calls now have the credentials: include option. This ❓ should we change to same-origin instead? it looks like this is actually the default
This will forward all cookies set on this domain to the backend API, which we then use to authenticate each request on the backend

User Profile

The Flipt backend API returns metadata provider by the Auth Provider, including name and optionally an image
We then render the profile image and name with a dropdown allowing the user to logout
TODO: test what happens if image is not available

Logout

feat: auth expire self / logout flipt#1279 added the ability to expire auth tokens and also clears the cookies mentioned above if the user requests /v1/auth/expire
When the user clicks logout, we issue the request to this API, resulting the auth token being expired in the DB as well as the browser cookies being cleared, then redirecting the user to /login again.

TODO

Test what happens if no name or image is available for the user after login
Determine if we should use same-origin instead of include for credential forwarding
Test with more providers

Future Improvements

I'd like to refactor the code that returns the login providers to be more flexible so that we can support other methods in the future (ie OAuth)

…into hooks-cleanup * 'hooks-cleanup' of https://github.com/flipt-io/flipt-ui: chore(deps-dev): bump prettier-plugin-organize-imports (#28)

* main: fix(fetch): pass credentials: include (#6) chore: rename error hook; cleanup (#31) chore(deps-dev): bump prettier from 2.8.2 to 2.8.3 (#29) chore(deps-dev): bump @typescript-eslint/eslint-plugin (#34) chore(deps-dev): bump @typescript-eslint/parser from 5.48.1 to 5.48.2 (#35) chore(deps-dev): bump eslint from 8.31.0 to 8.32.0 (#30) chore(deps-dev): bump eslint-plugin-react from 7.32.0 to 7.32.1 (#33) chore(deps-dev): bump eslint-plugin-import from 2.27.4 to 2.27.5 (#32)

* main: feature: Validate distributions (#37)

* 'login' of https://github.com/flipt-io/flipt-ui: chore(deps): bump react-router-dom from 6.6.2 to 6.7.0 (#39) chore(deps-dev): bump @types/react from 18.0.26 to 18.0.27 (#40) chore(deps): bump swr from 2.0.0 to 2.0.1 (#38)

darinmclain

Changes requested in the comments. Overall, looks good though!

src/components/SessionProvider.tsx

darinmclain · 2023-01-20T02:38:45Z

src/app/auth/Login.tsx

+  }, [setError]);
+
+  useEffect(() => {
+    loadAvailableProviders();


This seems like it would cause an endless loop of loading providers?

i checked with console.log it only calls once, I'm wondering if its because its wrapped in a useCallback?

Could this be because you're passing the loadAvailableProviders function as an arg below?

useEffect docs suggest the second argument is compared between each render.
If it doesn't change then the first argument function is not called again.
Seems to me that function will never change. Or maybe there is some overloaded alternative behaviour when a function is passed as second arg.

Seems you could also pass [] for the same effect:
https://reactjs.org/docs/hooks-effect.html#tip-optimizing-performance-by-skipping-effects

It seems like more often than not you want state in your dependencies array passed to useEffect.
There are cases where you might pass a function. Though that seems to be when they're defined out of scope:
https://reacttraining.com/blog/when-to-use-functions-in-hooks-dependency-array/

(I admit as I kept reading this I started to glaze over and maybe missed something key)

* main: chore: change doc icon (#42)

GeorgeMac · 2023-01-20T11:01:28Z

@markphelps I didn't realize it was sameorigin by default. Yeah, I think we can get away with that.
I think we mandate that the UI and the API are under the same domain for now and not support other deployment strategies yet.
We can change that down the line.

This does mean continued used of things like vite for local dev.

GeorgeMac · 2023-01-20T11:27:25Z

src/types/Auth.ts

+  'io.flipt.auth.oidc.email': string;
+  'io.flipt.auth.oidc.email_verified': string;
+  'io.flipt.auth.oidc.name': string;
+  'io.flipt.auth.oidc.picture'?: string;
+  'io.flipt.auth.oidc.provider': string;


Would be good to explore this with different requested scopes.
I see picture is required. However, I think technically they all could be required.
Since you can configure Flipt with no scopes (apart from the oidc scope which is required) and we can authenticate them but obtain next to no information.

Which, I think, is good. It is a feature the operator can decide on.

GeorgeMac

In my very limited knowledge, this looks great.
I put a couple rambling thoughts in here.

markphelps added 24 commits January 16, 2023 18:16

chore: rename hooks; cleanup

a10c20b

chore: remove auth provider

df414ae

Merge branch 'main' into hooks-cleanup

de1deb3

chore: rename hooks back

f388093

Merge branch 'hooks-cleanup' of https://github.com/flipt-io/flipt-ui …

0a78af8

…into hooks-cleanup * 'hooks-cleanup' of https://github.com/flipt-io/flipt-ui: chore(deps-dev): bump prettier-plugin-organize-imports (#28)

chore: forgot to stage renamed files

da6bb65

feat(auth): wip

032b24d

chore: add include creds everywhere; create delete helper func

bbaf690

chore: wip login

2e7fdd4

chore: wip

2081096

Merge branch 'main' into login

35d9b79

* main: feature: Validate distributions (#37)

chore: wip

686e4b0

chore: wip fix auth

28ec7bd

chore: load self data

b1850a5

feat: load user profile

cd11338

chore: fix login redirect when auth not required

172d858

chore: rename session, fix exhaustive deps issue

fbc0081

fix: api calls

e78804b

chore: show name on hover

04a7c50

chore: get images working in dev mode; opacity on user ring

fb8ce1d

feat: implement logout

559baf4

feat: add known providers/icons

e2fe2ef

Merge branch 'main' into login

158d53b

markphelps requested review from darinmclain and GeorgeMac January 19, 2023 23:57

markphelps added 2 commits January 19, 2023 21:28

chore: set session null on logout

588943f

darinmclain suggested changes Jan 20, 2023

View reviewed changes

chore: rm credentials include

494c7b7

markphelps added 2 commits January 19, 2023 22:22

Merge branch 'main' into login

c539acf

* main: chore: change doc icon (#42)

fix: linter warning

d117a98

markphelps requested a review from darinmclain January 20, 2023 03:37

GeorgeMac reviewed Jan 20, 2023

View reviewed changes

GeorgeMac approved these changes Jan 20, 2023

View reviewed changes

markphelps and others added 10 commits January 20, 2023 10:20

fix: loading of providers

b2b3ed5

fix: login page

6a15387

fix: session infinite loop

e153987

feat(api): add CSRF token support

baf347c

fix: show empty user if no imageURL

10d07bc

fix: set headers

225b9eb

fix: try to fix these loadeffect loops

4aa0794

chore: disable refresh interval in swr

277156c

fix(csrf): drop type constraints on setCsrf

9155a09

chore: make login dynamic import

137d98d

markphelps merged commit 86acc1c into main Jan 20, 2023

markphelps deleted the login branch January 20, 2023 17:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: UI Auth/Login #41

feat: UI Auth/Login #41

markphelps commented Jan 19, 2023 •

edited

Loading

darinmclain left a comment

darinmclain Jan 20, 2023

markphelps Jan 20, 2023

GeorgeMac Jan 20, 2023

GeorgeMac Jan 20, 2023

GeorgeMac commented Jan 20, 2023

GeorgeMac Jan 20, 2023

GeorgeMac left a comment

feat: UI Auth/Login #41

feat: UI Auth/Login #41

Conversation

markphelps commented Jan 19, 2023 • edited Loading

Auth Checks

Login

API Calls

User Profile

Logout

TODO

Future Improvements

darinmclain left a comment

Choose a reason for hiding this comment

darinmclain Jan 20, 2023

Choose a reason for hiding this comment

markphelps Jan 20, 2023

Choose a reason for hiding this comment

GeorgeMac Jan 20, 2023

Choose a reason for hiding this comment

GeorgeMac Jan 20, 2023

Choose a reason for hiding this comment

GeorgeMac commented Jan 20, 2023

GeorgeMac Jan 20, 2023

Choose a reason for hiding this comment

GeorgeMac left a comment

Choose a reason for hiding this comment

markphelps commented Jan 19, 2023 •

edited

Loading