Fix visibleGroups is undefined error #77

kairstenfay · 2020-01-10T19:29:39Z

In further testing of #74, we found an error when a user is logged-out
that prevented the splash page of nextstrain.org from loading.

Fix the error by checking if visibleGroups is defined in addition to
user before rendering the <UserGroups> component.

kairstenfay · 2020-01-10T19:30:17Z

static-site/src/components/splash/index.jsx

@@ -52,7 +52,7 @@ class Splash extends React.Component {

        <HugeSpacer/>

-        {this.props.user && <UserGroups user={this.props.user}/>}
+        {this.props.user && this.props.user.visibleGroups && <UserGroups user={this.props.user}/>}


I'm a little confused here how/why this.props.user was defined when there was no logged-in user.

user is getting passed to the Splash component via the UserDataWrapper component, which by default is undefined.

Ah, so looking closer, what's happening is that I have an existing login session (like most of our users will) but it lacks the visibleGroups attribute because that's only added by your previous PR. In order to get the new attribute with the current code, a person has to logout and log back in again. We can either guard against the missing attribute everywhere (a little ugly), or could put the computation somewhere else so it happens every time, not only in the login flow.

Ah, right. @jameshadfield initially pointed that out as a consequence of moving the visibleGroups logic inside the setup of OAuth2Strategy.

I now recall that was the error I encountered in testing and fixed by logging in and out again.

Is there no third solution where, now that the splash page is no longer broken, we can tell logged-in users to log in and out again? 😄

edit: I realize now that we will not want to make users log in and out every time they get added to a new group. So this is not a real solution.

edit: I realize now that we will not want to make users log in and out every time they get added to a new group. So this is not a real solution.

The Cognito groups for a user are (as I understand the code) only fetched when a user logs in, so I don't think it's possible (as currently implemented) to see a new cognito group without logging in again

The Cognito groups for a user are (as I understand the code) only fetched when a user logs in, so I don't think it's possible (as currently implemented) to see a new cognito group without logging in again

Correct. We can improve this in the future by refreshing user details from Cognito on ~every request (or, ideally, with some short-lived cache).

tsibley

I describe the issue a bit more in my reply to your comment.

Separately, the /users/trs page is also broken for me when I have an existing login session.

tsibley · 2020-01-10T19:38:18Z

static-site/src/components/splash/index.jsx

@@ -52,7 +52,7 @@ class Splash extends React.Component {

        <HugeSpacer/>

-        {this.props.user && <UserGroups user={this.props.user}/>}
+        {this.props.user && this.props.user.visibleGroups && <UserGroups user={this.props.user}/>}


Ah, so looking closer, what's happening is that I have an existing login session (like most of our users will) but it lacks the visibleGroups attribute because that's only added by your previous PR. In order to get the new attribute with the current code, a person has to logout and log back in again. We can either guard against the missing attribute everywhere (a little ugly), or could put the computation somewhere else so it happens every time, not only in the login flow.

kairstenfay · 2020-01-10T23:30:33Z

I moved the logic for determining visibleGroups back to its original place in authn.js under the /whoami endpoint.

kairstenfay · 2020-01-10T23:33:09Z

I'm still getting a console error when logged out:

TypeError: Cannot read property 'groups' of undefined
    at Object.json (/home/kairsten/nextstrain/nextstrain.org/authn.js:229:33)
    at ServerResponse.res.format (/home/kairsten/nextstrain/nextstrain.org/node_modules/express/lib/response.js:675:13)
    at app.route.get (/home/kairsten/nextstrain/nextstrain.org/authn.js:220:16)
    at Layer.handle [as handle_request] (/home/kairsten/nextstrain/nextstrain.org/node_modules/express/lib/router/layer.js:95:5)
    at next (/home/kairsten/nextstrain/nextstrain.org/node_modules/express/lib/router/route.js:137:13)
    at Route.dispatch (/home/kairsten/nextstrain/nextstrain.org/node_modules/express/lib/router/route.js:112:3)
    at Layer.handle [as handle_request] (/home/kairsten/nextstrain/nextstrain.org/node_modules/express/lib/router/layer.js:95:5)
    at /home/kairsten/nextstrain/nextstrain.org/node_modules/express/lib/router/index.js:281:22
    at Function.process_params (/home/kairsten/nextstrain/nextstrain.org/node_modules/express/lib/router/index.js:335:12)
    at next (/home/kairsten/nextstrain/nextstrain.org/node_modules/express/lib/router/index.js:275:10)
TypeError: Cannot read property 'groups' of undefined
    at Object.json (/home/kairsten/nextstrain/nextstrain.org/authn.js:229:33)
    at ServerResponse.res.format (/home/kairsten/nextstrain/nextstrain.org/node_modules/express/lib/response.js:675:13)
    at app.route.get (/home/kairsten/nextstrain/nextstrain.org/authn.js:220:16)
    at Layer.handle [as handle_request] (/home/kairsten/nextstrain/nextstrain.org/node_modules/express/lib/router/layer.js:95:5)
    at next (/home/kairsten/nextstrain/nextstrain.org/node_modules/express/lib/router/route.js:137:13)
    at Route.dispatch (/home/kairsten/nextstrain/nextstrain.org/node_modules/express/lib/router/route.js:112:3)
    at Layer.handle [as handle_request] (/home/kairsten/nextstrain/nextstrain.org/node_modules/express/lib/router/layer.js:95:5)
    at /home/kairsten/nextstrain/nextstrain.org/node_modules/express/lib/router/index.js:281:22
    at Function.process_params (/home/kairsten/nextstrain/nextstrain.org/node_modules/express/lib/router/index.js:335:12)
    at next (/home/kairsten/nextstrain/nextstrain.org/node_modules/express/lib/router/index.js:275:10)
TypeError: Cannot read property 'groups' of undefined
    at Object.json (/home/kairsten/nextstrain/nextstrain.org/authn.js:229:33)
    at ServerResponse.res.format (/home/kairsten/nextstrain/nextstrain.org/node_modules/express/lib/response.js:675:13)
    at app.route.get (/home/kairsten/nextstrain/nextstrain.org/authn.js:220:16)
    at Layer.handle [as handle_request] (/home/kairsten/nextstrain/nextstrain.org/node_modules/express/lib/router/layer.js:95:5)
    at next (/home/kairsten/nextstrain/nextstrain.org/node_modules/express/lib/router/route.js:137:13)
    at Route.dispatch (/home/kairsten/nextstrain/nextstrain.org/node_modules/express/lib/router/route.js:112:3)
    at Layer.handle [as handle_request] (/home/kairsten/nextstrain/nextstrain.org/node_modules/express/lib/router/layer.js:95:5)
    at /home/kairsten/nextstrain/nextstrain.org/node_modules/express/lib/router/index.js:281:22
    at Function.process_params (/home/kairsten/nextstrain/nextstrain.org/node_modules/express/lib/router/index.js:335:12)
    at next (/home/kairsten/nextstrain/nextstrain.org/node_modules/express/lib/router/index.js:275:10)

I'm currently still investigating a working solution.

edit: fixed

In further testing of #74, we found an error when a user is logged-out that prevented the splash page of nextstrain.org from loading. Move the logic for determining which groups are visible to a user into the `/whoami` endpoint. Send the resulting `visibleGroups` as an additional key in the json load sent by this endpoint. Handle the additional `visibleGroups` object accordingly.

tsibley · 2020-01-13T17:36:28Z

Thanks for fixing this, @kairstenfay. Merged!

kairstenfay requested a review from tsibley January 10, 2020 19:29

tsibley temporarily deployed to nextstrain-s-fix-visibl-r6gdap January 10, 2020 19:29 Inactive

kairstenfay commented Jan 10, 2020

View reviewed changes

tsibley requested changes Jan 10, 2020

View reviewed changes

kairstenfay force-pushed the Fix-visibleGroups-undefined-error branch from e8169d2 to aae1c22 Compare January 10, 2020 23:22

tsibley temporarily deployed to nextstrain-s-fix-visibl-r6gdap January 10, 2020 23:23 Inactive

kairstenfay force-pushed the Fix-visibleGroups-undefined-error branch from aae1c22 to 393cae0 Compare January 10, 2020 23:29

tsibley temporarily deployed to nextstrain-s-fix-visibl-r6gdap January 10, 2020 23:29 Inactive

kairstenfay force-pushed the Fix-visibleGroups-undefined-error branch from 393cae0 to 33b9d60 Compare January 10, 2020 23:31

tsibley temporarily deployed to nextstrain-s-fix-visibl-r6gdap January 10, 2020 23:31 Inactive

kairstenfay force-pushed the Fix-visibleGroups-undefined-error branch from 33b9d60 to 2d1f3b6 Compare January 11, 2020 01:09

tsibley temporarily deployed to nextstrain-s-fix-visibl-r6gdap January 11, 2020 01:09 Inactive

tsibley merged commit 2d1f3b6 into master Jan 13, 2020

kairstenfay deleted the Fix-visibleGroups-undefined-error branch January 13, 2020 18:30

tsibley mentioned this pull request Jan 14, 2020

Sessions don't refresh group membership information without logout/login cycle #81

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix visibleGroups is undefined error #77

Fix visibleGroups is undefined error #77

kairstenfay commented Jan 10, 2020

kairstenfay Jan 10, 2020

kairstenfay Jan 10, 2020

tsibley Jan 10, 2020

kairstenfay Jan 10, 2020

kairstenfay Jan 10, 2020 •

edited

Loading

jameshadfield Jan 12, 2020

tsibley Jan 13, 2020

tsibley left a comment

tsibley Jan 10, 2020

kairstenfay commented Jan 10, 2020

kairstenfay commented Jan 10, 2020 •

edited

Loading

tsibley commented Jan 13, 2020

Fix visibleGroups is undefined error #77

Fix visibleGroups is undefined error #77

Conversation

kairstenfay commented Jan 10, 2020

kairstenfay Jan 10, 2020

Choose a reason for hiding this comment

kairstenfay Jan 10, 2020

Choose a reason for hiding this comment

tsibley Jan 10, 2020

Choose a reason for hiding this comment

kairstenfay Jan 10, 2020

Choose a reason for hiding this comment

kairstenfay Jan 10, 2020 • edited Loading

Choose a reason for hiding this comment

jameshadfield Jan 12, 2020

Choose a reason for hiding this comment

tsibley Jan 13, 2020

Choose a reason for hiding this comment

tsibley left a comment

Choose a reason for hiding this comment

tsibley Jan 10, 2020

Choose a reason for hiding this comment

kairstenfay commented Jan 10, 2020

kairstenfay commented Jan 10, 2020 • edited Loading

tsibley commented Jan 13, 2020

kairstenfay Jan 10, 2020 •

edited

Loading

kairstenfay commented Jan 10, 2020 •

edited

Loading