- 
                Notifications
    You must be signed in to change notification settings 
- Fork 242
fix: redirect unauthenticated users from My Channels to public catalog #5488
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
base: unstable
Are you sure you want to change the base?
fix: redirect unauthenticated users from My Channels to public catalog #5488
Conversation
af6594d    to
    aacae10      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @Abhishek-Punhani for tackling this. I've left you a specific comment for requested changes to the authentication check
| ) { | ||
| this.$router.replace({ name: RouteNames.CATALOG_ITEMS }); | ||
| return; | ||
| } | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is written correctly to address the issue. However, with the existing behavior in mind that was described in the issue, adding this code would mean there are two locations that are performing an authentication check and 'redirect' like this. In other words, based off the existing behavior, there's another location that should be updated rather than adding this new code.
8f49e7d    to
    1650ace      
    Compare
  
    | @bjester , I have modified the route watcher to check for user authentication, and it's working. Please let me know if these changes are satisfactory. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please take another look at my previous comment, and the code I linked. Only a very small change to that code is needed to handle the replace task.
…-channels now redirects the unauthorised users Signed-off-by: Abhishek-Punhani <[email protected]>
…n My Channels Signed-off-by: Abhishek-Punhani <[email protected]>
Signed-off-by: Abhishek-Punhani <[email protected]>
1650ace    to
    d694e55      
    Compare
  
    | @bjester, thanks for the feedback. You're right that the existing created() logic handles unauthenticated users, but it only runs on component mount—not on route changes, such as manual URL navigation to /my-channels. To prevent the spinner in those cases, we need the $route watcher to check and redirect dynamically. This keeps the logic centralised without duplication. Let me know if this works. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bjester, thanks for the feedback. You're right that the existing created() logic handles unauthenticated users, but it only runs on component mount—not on route changes, such as manual URL navigation to /my-channels. To prevent the spinner in those cases, we need the $route watcher to check and redirect dynamically. This keeps the logic centralised without duplication. Let me know if this works.
That's solid reasoning for the motive, but the changes didn't entirely reflect that. The first reason the changes don't reflect that is because the route watcher isn't executed on mount. It's easy to see this if you had tested it because your code still does not satisfy the scope of the issue: to replace the route in the history stack when navigating directly to a /channels route that requires authentication, like the link on the login form does. Loading /en/channels directly in a fresh tab, the browser's back button activates, which indicates the route wasn't replaced. It's easy to see this adding some console.log statements, and that your modification only prevents it from rendering when back is pressed. Notice page title, too:
| Initial | After back is pressed | 
|---|---|
| CREATED👀 | WATCH👀 | 
|  |  | 
As you mentioned, the only way the user would encounter the conditions satisfied by your change is by manually modifying the URL. If the user signs out in a separate browser tab, Studio is already reactive enough to reload the sign-in page.
The issue described a situation where we explicitly linked to a page that then redirected (pushed) to a new route, which is an easily navigable pathway for the user, but isn't accomplished by your change. The intent of the existing redirect also matched the navigable nature, and thus the expected changes are still accommodating for that intent.
In such a case as the user manually modifying the URL after opening a route under /channels, that is a significantly different user intent. Since the user's intent in that case is probably different than the existing intent, the solution is out of scope of the issue. While I admire your proactive consolidation, it should be something you proactively communicate with maintainers before making changes. Now, reflecting on this different intent, I think it would be better served by a redirect to the login page.
Lastly, you seem to have lost one of the expected changes that was satisfactory during my prior review.
Summary
/channelsto/channels/#/publicto directly load the public channel catalogue for guests.Updated tests in channelList.spec.js to remove the session mock since authentication is now handled at the index level.
References
#5482
Reviewer guidance
Manually navigate to /my-channels (or use the URL) – it should redirect to /public instantly, replacing the history.