Use chromedash-login-required-page for all components that require signin (#3070)

* Use chromedash-login-required-page for all components that require signin

client-src/elements/chromedash-app.js

@@ -192,10 +192,10 @@ class ChromedashApp extends LitElement {
   }
 
   // Maybe set up new page, or if the URL is the same, we stay.
-  // If signin is required and a backup component is passed in signinRequiredComponentName,
+  // If signin is required 'chromedash-login-required-page' is rendered.,
   // we render that component instead page.
   // Returns true if we are proceeding to the new page, false otherwise.
-  setupNewPage(ctx, componentName, signinRequiredComponentName) {
+  setupNewPage(ctx, componentName) {
     // If current page is ctx.path and a ctx.hash exists,
     // don't create a new element but instead
     // just scroll to the element identified by the hash.
@@ -223,8 +223,8 @@ class ChromedashApp extends LitElement {
     const signinRequired = ctx.querystring.search('loginStatus=False') > -1;
 
     // Loading new page.
-    this.pageComponent = document.createElement(signinRequired && signinRequiredComponentName ?
-      signinRequiredComponentName :
+    this.pageComponent = document.createElement(signinRequired ?
+      'chromedash-login-required-page' :
       componentName);
     this.setUnsavedChanges(false);
     this.removeBeforeUnloadHandler();
@@ -306,15 +306,16 @@ class ChromedashApp extends LitElement {
     });
     page('/guide/new', (ctx) => {
       if (!this.setupNewPage(ctx, 'chromedash-guide-new-page')) return;
-      this.pageComponent.userEmail = this.user.email;
+      if (ctx.querystring.search('loginStatus=False') == -1) {
+        this.pageComponent.userEmail = this.user.email;
+      }
       this.currentPage = ctx.path;
       this.hideSidebar();
     });
     page('/guide/enterprise/new', (ctx) => {
       if (!this.setupNewPage(
         ctx,
-        'chromedash-guide-new-page',
-        'chromedash-login-required-page')) return;
+        'chromedash-guide-new-page')) return;
 
       if (ctx.querystring.search('loginStatus=False') == -1) {
         this.pageComponent.userEmail = this.user.email;
@@ -414,8 +415,7 @@ class ChromedashApp extends LitElement {
     page('/enterprise/releasenotes', (ctx) => {
       if (!this.setupNewPage(
         ctx,
-        'chromedash-enterprise-release-notes-page',
-        'chromedash-login-required-page')) return;
+        'chromedash-enterprise-release-notes-page')) return;
       this.pageComponent.user = this.user;
       this.contextLink = ctx.path;
       this.currentPage = ctx.path;

framework/basehandlers.py

@@ -527,6 +527,8 @@ class ConstHandler(FlaskHandler):
   def get_template_data(self, **defaults):
     """Render a template, or return a JSON constant."""
     if defaults.get('require_signin') and not self.get_current_user():
+      if 'loginStatus=False' in self.get_common_data()['current_path']:
+        return {}
       return flask.redirect(settings.LOGIN_PAGE_URL), self.get_headers()
     if 'template_path' in defaults:
       template_path = defaults['template_path']
@@ -562,11 +564,9 @@ def get_spa_template_data(handler_obj, defaults):
   """Check permissions then let spa.html do its thing."""
   # Check if the page requires user to sign in
   if defaults.get('require_signin') and not handler_obj.get_current_user():
-    if defaults.get('is_enterprise_page'):
-      common_data = handler_obj.get_common_data()
-      if 'loginStatus=False' in common_data['current_path']:
-        return {}
-      return flask.redirect('?loginStatus=False'), handler_obj.get_headers()
+    common_data = handler_obj.get_common_data()
+    if 'loginStatus=False' in common_data['current_path']:
+      return {}
     return flask.redirect(settings.LOGIN_PAGE_URL), handler_obj.get_headers()
 
   # Check if the page requires create feature permission

framework/basehandlers_test.py

@@ -1142,23 +1142,11 @@ class GetSPATemplateDataTests(testing_config.CustomTestCase):
     self.assertEqual(
         settings.LOGIN_PAGE_URL, actual_redirect.headers['location'])
 
-  def test_get_spa_template_data__signin_missing_enterprise(self):
-    """This page requires sign in, but user is anon."""
-    testing_config.sign_out()
-    with test_app.test_request_context('/must_be_signed_in'):
-      defaults = {'require_signin': True, 'is_enterprise_page':True}
-      actual_redirect, actual_headers = basehandlers.get_spa_template_data(
-          self.handler, defaults)
-
-    self.assertEqual(302, actual_redirect.status_code)
-    self.assertEqual(
-        '?loginStatus=False', actual_redirect.headers['location'])
-
-  def test_get_spa_template_data__signin_missing_enterprise_after_redirect(self):
+  def test_get_spa_template_data__signin_missing_after_redirect(self):
     """This page requires sign in, but user is anon."""
     testing_config.sign_out()
     with test_app.test_request_context('/must_be_signed_in?loginStatus=False'):
-      defaults = {'require_signin': True, 'is_enterprise_page':True}
+      defaults = {'require_signin': True}
       actual = basehandlers.get_spa_template_data(self.handler, defaults)
 
     self.assertEqual({}, actual)

framework/permissions.py

@@ -137,6 +137,13 @@ def can_approve_feature(user: User, feature: FeatureEntry, approvers) -> bool:
   return is_approver
 
 
+def _maybe_redirect_to_login(handler_obj):
+  common_data = handler_obj.get_common_data()
+  if 'current_path' in common_data and 'loginStatus=False' in common_data['current_path']:
+    return {}
+  return handler_obj.redirect(settings.LOGIN_PAGE_URL)
+    
+
 def _reject_or_proceed(
     handler_obj, handler_method, handler_args, handler_kwargs,
     perm_function):
@@ -146,7 +153,7 @@ def _reject_or_proceed(
 
   # Give the user a chance to sign in
   if not user and req.method == 'GET':
-    return handler_obj.redirect(settings.LOGIN_PAGE_URL)
+    return _maybe_redirect_to_login(handler_obj)
 
   if not perm_function(user):
     handler_obj.abort(403)
@@ -189,7 +196,7 @@ def validate_feature_create_permission(handler_obj):
 
   # Give the user a chance to sign in
   if not user and req.method == 'GET':
-    return handler_obj.redirect(settings.LOGIN_PAGE_URL)
+    return _maybe_redirect_to_login(handler_obj)
 
   # Redirect to 403 if user does not have create permission for feature.
   if not can_create_feature(user):
@@ -203,7 +210,7 @@ def validate_feature_edit_permission(handler_obj, feature_id: int):
 
   # Give the user a chance to sign in
   if not user and req.method == 'GET':
-    return handler_obj.redirect(settings.LOGIN_PAGE_URL)
+    return _maybe_redirect_to_login(handler_obj)
 
   # Redirect to 404 if feature is not found.
   # Load feature directly from NDB so as to never get a stale cached copy.

framework/permissions_test.py

@@ -31,6 +31,10 @@ class MockHandler(basehandlers.BaseHandler):
 
   def __init__(self):
     self.called_with = None
+    self.common_data = {}
+
+  def get_common_data(self):
+    return self.common_data
 
   @permissions.require_admin_site
   def do_get(self, *args):

settings.py

@@ -64,7 +64,7 @@ GOOGLE_SIGN_IN_CLIENT_ID = (
 
 # This is where the an anon user is redirected if they try to access a
 # page that requires being signed in.
-LOGIN_PAGE_URL = '/features?loginStatus=False'
+LOGIN_PAGE_URL = '?loginStatus=False'
 
 INBOUND_EMAIL_ADDR = 'chromestatus@cr-status-staging.appspotmail.com'