Detangle Cognito, sources, & group logic on authn #74
Conversation
1768ed8 to
06daa63
Compare
| @@ -9,7 +9,7 @@ const UserGroups = (props) => { | |||
|
|
|||
| const colors = [...theme.titleColors]; | |||
|
|
|||
| const groupCards = props.user.groups.map((group) => { | |||
| const groupCards = props.user.visibleSources.map((group) => { | |||
There was a problem hiding this comment.
how do we feel about calling the mapping variable group still?
There was a problem hiding this comment.
Just commented on that separately before I saw your note here. :-)
| @@ -9,7 +9,7 @@ const UserGroups = (props) => { | |||
|
|
|||
| const colors = [...theme.titleColors]; | |||
|
|
|||
| const groupCards = props.user.groups.map((group) => { | |||
| const groupCards = props.user.visibleSources.map((group) => { | |||
There was a problem hiding this comment.
There's a conflict of terms here: the array is one of sources, but the map function calls them groups.
It may be that my off-the-cuff suggestion of visibleSources was a poor one and visibleGroups would be better.
| user.visibleSources = Array.from(sources.values()) | ||
| .filter(source => source.visibleToUser(user)) | ||
| .map(source => source._name) | ||
| .filter(source => groups.includes(source)); |
There was a problem hiding this comment.
Is the purpose of this last line to filter out the non-group sources (e.g. CoreSource, CoreStagingSource, and CommunitySource)?
If so, then I think there are more future-proof ways of doing this which delegates that decision to the source itself and avoids deepening the existing hardcoded assumption/conflation that the group name matches the source name exactly.
I don't think ☝️ needs to block merge, but I'll think about it some more and maybe push a commit up.
There was a problem hiding this comment.
Yes, it's to remove the groups/tiles that appear as core, staging, and community on nextstrain.org.
There was a problem hiding this comment.
I'm curious to know what a better solution would be.
There was a problem hiding this comment.
I'm curious to know what a better solution would be.
Not entirely sure! :-) The equating of group names with source names in this part of the code raises red flags for me, but I think avoiding that here would require broader changes than I think are worth implementing right now. So I would merge this as-is and we can revisit later during other work on Groups if it arises again.
Centralize the computation of the list of sources visible to the current user, but using a different property that doesn't conflate Cognito groups with our internal sources. Additionally, centralize the authz decisions to stay within the Source object. This change was originally [requested by Thomas on a recently closed PR](1a65e10#commitcomment-36703817).
06daa63 to
958d9d3
Compare
|
@tsibley I renamed |
|
Ah, sorry, I was doing one last test and ran into an issue: |
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.
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.
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.
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.
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.
Centralize the computation of the list of sources visible to the current
user, but using a different property that doesn't conflate Cognito
groups with our internal sources.
Additionally, centralize the authz decisions to stay within the Source
object.
This change was originally requested by Thomas on a recently closed
PR.