Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set current namespace when checking authz #1205

Conversation

absoludity
Copy link
Contributor

@absoludity absoludity commented Oct 4, 2019

This diff is an alternative to #1201, which I think is caused by #1194. So instead, this PR reverts #1194 (well, most of it - there is one small change we want to keep) and provides an alternative.

So the issue mentioned on #1194 was that:

Again I found more issues (mostly related to my changes, sorry). The issue is that current.namespace was now always set to default because it was read at the very beginning when the token is not yet introduced.

I think this meant that if you are not authenticated, we were setting the current namespace to default when the initialState was created for the reducer, and it was never overwritten from that point. The solution in #1194 was both to not set it in the initialState and add an action to update it when not set, but I think this is not the correct solution (and leads to the current issue mentioned in #1201).

So this PR provides an alternative: that we explicitly set the current namespace when a user authenticates (setAuthenticated is only ever called at that one point). As mentioned on #1201, I don't think it should ever be empty and the only cases where we set it are:

  • When you login it should be set from the default namespace,
  • When you are already authd and reload a page it should be set from the url (if present), otherwise defaulting to getNamespaceFromToken,
  • When you switch namespaces (and the location changes) it should update
  • When you logout it should be reset to the initialState of the namespace reducer.

Let me know what you think.

@absoludity absoludity requested a review from andresmgot October 4, 2019 07:42
@absoludity absoludity self-assigned this Oct 4, 2019
Copy link
Contributor

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also works for me. So far I don't see any issue, let's see after testing it.

case LOCATION_CHANGE:
const pathname = action.payload.location.pathname;
// looks for /ns/:namespace in URL
const matches = pathname.match(/\/ns\/([^/]*)/);
if (matches) {
return { ...state, current: matches[1] };
}
case getType(actions.auth.setAuthenticated):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

regarding your Typescript error. The problem is that LOCATION_CHANGE doesn't have a break; at the end of the case so this one is also executed after the previous one if it matches. That's why you are seeing the error, the payload here can be a location change.

To fix it, either add a break in the case LOCATION_CHANGE or move this case above (but you also need to add a break;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent, thanks.

@andresmgot andresmgot mentioned this pull request Oct 4, 2019
@absoludity
Copy link
Contributor Author

This also works for me. So far I don't see any issue, let's see after testing it.

Great, thanks. QA'd locally and verified that:

  • When I first authenticate the default namespace is updated from the token (ie. that it fixes the issue which the reverted Revamp default namespace setup #1194 solved in a different way), and
  • If I click on catalog, then reload the page, the default namespace is set immediately from the token as the reducers initial state (ie. fixing what Fix missing namespace #1201 wanted to fix).

@absoludity absoludity requested a review from andresmgot October 8, 2019 01:08
Copy link
Contributor

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks for checking.

@absoludity absoludity merged commit 122f4aa into vmware-tanzu:master Oct 8, 2019
@absoludity absoludity deleted the set-current-namespace-when-checking-authz branch October 8, 2019 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants