feat(redirectTo): added UI part of redirectTo logic for CLOUD users to redirectTo original links after logging in #18214

asalem1 · 2020-05-26T15:09:34Z

Problem

Users navigating to CLOUD would lose the original URL reference once they completed logging in.

Solution

This is part one of a dual-change that will address the issue described above. Namely, the UI will set the redirectTo as the query parameter for the API. In this PR, we have:

Set feature flags for this feature
Sets the RedirectTo as a queryParameter when signing in
Sets the redirectTo in localStorage to then set as a queryParameter when fetching the clientConfig data
Removes the redirectTo from localStorage when it exists & the user has successfully logged in (there hasn't been an error here: https://github.com/influxdata/influxdb/compare/feat/redirectTo?expand=1#diff-61767b19972976d43393eefd74d91cacR77)

CHANGELOG.md updated with a link to the PR (not the Issue)
Rebased/mergeable

…directTo as a queryParameter

…ed correctly on IDPE Co-authored-by: Greg Linton<greg@influxdata.com>

…ctices & feature flag Co-authored-by: Greg Linton<greg@influxdata.com>

…is signed in

drdelambre · 2020-05-26T15:38:16Z

flags.yml

@@ -40,3 +40,11 @@
  default: false
  contact: Lyon Hill
  expose: true
+
+- name: RedirectTo Path After Login


is there a corresponding idpe PR that matches this one?

no PR up yet. I'm glad you commented on this. When we talked to @gavincabbage about it on Friday it seemed like the UI yml file would generate the flag for the API too, is that right? Right now we want to write up some tests for the API to make sure everything is smooth before submitting a PR for cloud

it will show up on api/v2/flags if that's what you're referring to as the API (we need to figure out better words for all this), but only for people who are hosting this locally, which seems like the opposite of the intent of this PR? If you want the flag to only show up on a production deployment, it needs to be made on IDPE. You should probably even remove the flag from this repo, as local overrides would work for development and the flag isn't meant for OSS instances that handle their own auth.

Gotcha. So I'll remove the local flag and set it in IDPE

Some clarifying details: The flag only needs to be in one place, either influxdb/flags.yml or idpe/flags.yml. The former for flags used by OSS or both OSS and IDPE. The latter for flags used by only IDPE. Since the UI lives in OSS afaiu, most or all UI related flags will probably end up here.

I made the above comment without refreshing this page, and missed @drdelambre's latest comment.

If the code is resilient to the flag only being present in IDPE's api/v2/flags response and not in OSS's, then moving the flag to IDPE also works.

drdelambre · 2020-05-26T16:33:56Z

ui/src/Signin.tsx

+            window.location.href
+          }`
+        )
+        window.localStorage.setItem('redirectTo', window.location.href)


note to self: this is because sessions are handled weird on the backend and don't exist until a user is authenticated instead of elevating an anonymous session that is created on first touch. We can't follow a request chain from the unauthenticated user to an authenticated user. this'll become a problem when we start sharing dashboards as we'll introduce non-authenticated user state into the mix.

hoorayimhelping · 2020-05-26T15:50:51Z

kit/feature/list.go

@@ -58,16 +58,32 @@ func SessionService() BoolFlag {
 	return sessionService
 }

+var redirectto = MakeBoolFlag(


Suggested change

var redirectto = MakeBoolFlag(

var redirectTo = MakeBoolFlag(

looks like the examples here are all camelCased. Best to stick with local conventions

hoorayimhelping · 2020-05-26T16:10:15Z

ui/src/Signin.tsx

@@ -74,13 +75,28 @@ export class Signin extends PureComponent<Props, State> {
  private checkForLogin = async () => {
    try {
      await client.users.me()
+      const redirectIsSet = !!window.localStorage.getItem('redirectTo')


do we have a way to access storage without hitting localStorage directly? maybe some kind of module that handles fallbacks in case localStorage isn't enabled?

It seems like we're setting/getting state items here:

https://github.com/influxdata/influxdb/blob/master/ui/src/localStorage.ts#L41-L51

But they make use of the same underlying functionality. Is it beneficial to abstract some of the logic to a generalized setter in that file? I'm curious to know

yeah, we should use the abstractions that exist in the system, even if they're doing the same thing. if we needed to add an extra step (like let's say we have to add a timestamp to all writes to local storage), we'd only have to update the code to do that in one place, if everything made use of that function. it handles error messaging in a consistent way as well

Sounds good. I'll go ahead and write up some utility functions and send up a commit for that

hoorayimhelping · 2020-05-26T16:12:00Z

ui/src/Signin.tsx

+            window.location.href
+          }`
+        )
+        window.localStorage.setItem('redirectTo', window.location.href)


why set the redirectTo to the current url instead of just removing it from localStorage since we're done with it?

Great question - so this feature is actually handling redirectTo logic for three separate environments conditions:

Social Sign On is not enabled (tools cluster), and the user logs in via the universal login page

Social Sign On is enabled and the user is authenticated in Auth0 but not IDPE

Social Sign On *is enabled and the user is NOT authenticated in Auth0

By setting the query parameter directly to the initial api/v2/signin request, we handle the first two cases (where a user is being navigated - whether directly or silently - to the universal login page).

The reason we are setting the redirectTo onto localStorage is due to the way that authentication occurs in cloud. Basically, by the time we get navigated to the /login page, we will have no reference or session with the original redirectTo logic (and the browser will have been redirected about 4 times)

and the browser will have been redirected about 4 times

😬 😱

Tell me about it. Things are in need of a good unification

hoorayimhelping · 2020-05-26T16:13:26Z

ui/src/Signin.tsx

+        )
+        window.localStorage.setItem('redirectTo', window.location.href)
+        window.location.href = url.href
+        throw error


why throw the error if we're redirecting away from this page?

The original implementation set this so I went based off of that. I think the understanding is that if there's an issue with the redirect, we will not want the user to continue on (since that'll navigate them to /signin which is reserved for OSS)

hoorayimhelping · 2020-05-26T16:19:41Z

ui/src/authorizations/apis/index.ts

+export const getAuth0Config = async (
+  redirectTo: string
+): Promise<Auth0Config> => {
  try {
-    const response = await fetch(
-      `${getAPIBasepath()}/api/v2private/oauth/clientConfig`
-    )
+    let url = `${getAPIBasepath()}/api/v2private/oauth/clientConfig`
+    if (isFlagEnabled('redirectto')) {
+      url = `${getAPIBasepath()}/api/v2private/oauth/clientConfig?redirectTo=${redirectTo}`
+    }
+    const response = await fetch(url)


i think this flag checking logic makes more sense in the component. the authorizations api shouldn't really know about feature flags if it doesn't have to. it's just making a request to a url and getting some data back.

i think it would be clearer to have redirectTo be an optional parameter that gets added as a query param if it exists. then you can move the isFlagEnabled one layer up to the code that calls this, LoginPageContents.

I think I see what you're saying

hoorayimhelping · 2020-05-26T16:33:54Z

ui/src/onboarding/containers/LoginPageContents.tsx

@@ -61,7 +61,8 @@ class LoginPageContents extends PureComponent<DispatchProps> {

  public async componentDidMount() {
    try {
-      const config = await getAuth0Config()
+      const redirectTo = window.localStorage.getItem('redirectTo') || '/'


the feature flag makes more sense here i think - here it can control what gets sent to getAuth0Config so that that layer doesn't even need to be aware of the flag. something like:

let config if (isFlagEnabled('redirectto')) { const redirectTo = window.localStorage.getItem('redirectTo') || '/' config = await getAuth0Config(redirectTo) } else { config = await getAuth0Config() }

doing it this way (not sending an empty string will leave the url a little cleaner - it won't have ?redirectTo= with no value in it - in other words, only append ?redirectTo=${actualURL} if there's something to redirect to.

That makes sense

drdelambre · 2020-05-26T16:34:42Z

ui/src/authorizations/apis/index.ts

-    )
+    let url = `${getAPIBasepath()}/api/v2private/oauth/clientConfig`
+    if (isFlagEnabled('redirectto')) {
+      url = `${getAPIBasepath()}/api/v2private/oauth/clientConfig?redirectTo=${redirectTo}`


why not just url += ?redirectTo= here?

No reason not to. Thought about it but figured this was a bit more explicit and would require less manipulation once the feature flag is removed

drdelambre · 2020-05-26T16:36:10Z

kit/feature/list.go

@@ -58,16 +58,32 @@ func SessionService() BoolFlag {
 	return sessionService
 }

+var redirectto = MakeBoolFlag(


is this autogenerated code from make flags that you forgot to remove?

it is autogenerated code that I didn't realize was autogenerated ;)

drdelambre

remove the autogenerated code changes, but the rest looks great given the weird session setup we have in the system.

…gestions

…ctions

asalem1 added 6 commits May 22, 2020 12:19

feat(redirectTo): featureFlagged the work for stagging and set the re…

505b0cd

…directTo as a queryParameter

feat(redirectTo): Added path to signin and validated it is being pars…

066aba3

…ed correctly on IDPE Co-authored-by: Greg Linton<greg@influxdata.com>

feat(redirectTo): POC is working, still need to refactor for best pra…

cdcc9c8

…ctices & feature flag Co-authored-by: Greg Linton<greg@influxdata.com>

feat(redirectTo): removed redirectTo from localStorage when the user …

6254db2

…is signed in

feat(redirectTo): set featureFlag check correctly

5e29591

feat(redirectTo): set default to false

462b16f

asalem1 requested review from hoorayimhelping and drdelambre May 26, 2020 15:09

asalem1 changed the title ~~Feat/redirect to~~ feat(redirectTo): added UI part of redirectTo logic for CLOUD users to redirectTo original links after logging in May 26, 2020

drdelambre reviewed May 26, 2020

View reviewed changes

asalem1 added 2 commits May 26, 2020 08:41

feat(flagClientConfig): added UI featureFlag for clientConfig

d79178d

feat(flag): removing the featureFlag since it needs to exist in the API

ce177f9

drdelambre reviewed May 26, 2020

View reviewed changes

hoorayimhelping suggested changes May 26, 2020

View reviewed changes

drdelambre reviewed May 26, 2020

View reviewed changes

drdelambre approved these changes May 26, 2020

View reviewed changes

asalem1 added 2 commits May 26, 2020 09:43

feat(list.go): removed the autogenerated flag data

82d5ee1

chore(PRchanges): colocated some of the featureFlagging as per PR sug…

5701a6f

…gestions

asalem1 requested a review from hoorayimhelping May 26, 2020 17:14

hoorayimhelping approved these changes May 26, 2020

View reviewed changes

asalem1 added 2 commits May 26, 2020 13:05

chore(utility): transferred the localStorage methods into utility fun…

fdb811b

…ctions

chore(LoginPageContents): refactored to use utility method

aff535d

asalem1 merged commit 0770046 into master May 27, 2020

asalem1 deleted the feat/redirectTo branch May 27, 2020 12:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(redirectTo): added UI part of redirectTo logic for CLOUD users to redirectTo original links after logging in #18214

feat(redirectTo): added UI part of redirectTo logic for CLOUD users to redirectTo original links after logging in #18214

asalem1 commented May 26, 2020 •

edited

Loading

drdelambre May 26, 2020

asalem1 May 26, 2020

drdelambre May 26, 2020 •

edited

Loading

asalem1 May 26, 2020

gavincabbage May 26, 2020

gavincabbage May 26, 2020

drdelambre May 26, 2020

hoorayimhelping May 26, 2020

hoorayimhelping May 26, 2020

asalem1 May 26, 2020

hoorayimhelping May 26, 2020

asalem1 May 26, 2020

hoorayimhelping May 26, 2020

asalem1 May 26, 2020

hoorayimhelping May 26, 2020

asalem1 May 26, 2020

hoorayimhelping May 26, 2020

asalem1 May 26, 2020 •

edited

Loading

hoorayimhelping May 26, 2020

asalem1 May 26, 2020

hoorayimhelping May 26, 2020

asalem1 May 26, 2020

drdelambre May 26, 2020

asalem1 May 26, 2020

drdelambre May 26, 2020

asalem1 May 26, 2020

drdelambre left a comment

	var redirectto = MakeBoolFlag(
	var redirectTo = MakeBoolFlag(

feat(redirectTo): added UI part of redirectTo logic for CLOUD users to redirectTo original links after logging in #18214

feat(redirectTo): added UI part of redirectTo logic for CLOUD users to redirectTo original links after logging in #18214

Conversation

asalem1 commented May 26, 2020 • edited Loading

Problem

Solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drdelambre May 26, 2020 • edited Loading

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

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

asalem1 May 26, 2020 • edited Loading

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

drdelambre left a comment

Choose a reason for hiding this comment

asalem1 commented May 26, 2020 •

edited

Loading

drdelambre May 26, 2020 •

edited

Loading

asalem1 May 26, 2020 •

edited

Loading