SF-3117 Add option to remove branding from auth0 page #2937

RaymondLuong3 · 2025-01-08T21:54:23Z

This PR calls the Auth0 login API with the additional useBranding and logo parameters that will configure Auth0 to hide the SF name and SIL logo when using a site other than localhost or scriptureforge.org.
I marked this as testing not required since it won't be testable until the non-branded site is live. A developer can test this by setting useBranding to false.

This change is

Nateowami · 2025-01-08T22:27:56Z

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/auth.service.ts

@@ -243,11 +245,15 @@
    const state: AuthState = { returnUrl };
    const language: string = getAspCultureCookieLanguage(this.cookieService.get(ASP_CULTURE_COOKIE_NAME));
    const ui_locales: string = language;
+    const useBranding: boolean =
+      this.locationService.origin.includes('scriptureforge.org') || this.locationService.origin.includes('localhost');


To fix the problem, we need to parse the URL and check its host value against a whitelist of allowed hosts. This ensures that the check is not bypassed by embedding the allowed host in an unexpected location within the URL.

Parse the URL to extract the host.

Check if the host is in a predefined list of allowed hosts.

Replace the substring check with this more secure approach.

We will need to import the URL module to parse the URL and define a list of allowed hosts.

This alert is correct. We should be making an exact check for location.host or location.hostname (the difference being that host includes the port, so it would be localhost:5000 locally).

Thanks, I have it working as per the suggestion here.

codecov · 2025-01-08T22:03:02Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.96%. Comparing base (c433724) to head (cea0ea1).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2937   +/-   ##
=======================================
  Coverage   81.96%   81.96%           
=======================================
  Files         545      545           
  Lines       31536    31536           
  Branches     5090     5112   +22     
=======================================
  Hits        25850    25850           
+ Misses       4939     4927   -12     
- Partials      747      759   +12

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

marksvc

I tested locally and it is working for me.

I observe that the SF name is being used rather than "Log In", when at a useBranding===true site. If a change for that was supposed to be included here.

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Github-advanced-security[bot] and @RaymondLuong3)

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/auth.service.spec.ts line 436 at r3 (raw file):

  }));

  it('should login with branding', fakeAsync(() => {

I think you mean 'without' ?

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/auth.service.ts line 257 at r3 (raw file):

      login_hint: ui_locales,
      useBranding,
      logo: 'https://auth0.languagetechnology.org/assets/sf.svg'

From just this code file, it seems a little odd that this logo isn't used, when useBranding===false.

marksvc · 2025-01-09T20:58:25Z

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/auth.service.ts line 257 at r3 (raw file):

Previously, marksvc wrote…

From just this code file, it seems a little odd that this logo isn't used, when useBranding===false.

A possible improvement would be for the login page to always use the logo, if specified.
An alternate possible improvement would be to put a comment in interface xForgeAuth0Parameters on logo, saying something like "Log in/Sign up page logo to use when useBranding===false".
It's also not the end of the world.

Nateowami · 2025-01-09T21:42:31Z

@RaymondLuong3 This part of the Jira issue doesn't appear to have been addressed:

Also, there's an idiosyncrasy in that the login page says "Scripture Forge" at the top, but signup page says "Sign Up" at the top. This would be a good time to address that.

I also don't really see a value in having two different appearances.

marksvc · 2025-01-09T21:56:05Z

two different appearances

As in one display with the SIL logo, and one display with the SF logo?

marksvc

I approve. Tho you may still want to address some of these above comments in the same PR.

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Github-advanced-security[bot] and @RaymondLuong3)

RaymondLuong3

Thanks for the suggestions. I have made the update to the auth0 config repo to use the logo when it is provided. I've also updated the code there so that the Login and Signup tabs and titles match the widget title.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Github-advanced-security[bot])

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/auth.service.ts line 257 at r3 (raw file):

Previously, marksvc wrote…

A possible improvement would be for the login page to always use the logo, if specified.
An alternate possible improvement would be to put a comment in interface xForgeAuth0Parameters on logo, saying something like "Log in/Sign up page logo to use when useBranding===false".
It's also not the end of the world.

Thanks. That makes sense. When you try it now the logo provided will always be used.

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/auth.service.spec.ts line 436 at r3 (raw file):

Previously, marksvc wrote…

I think you mean 'without' ?

Good catch!

marksvc

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Github-advanced-security[bot])

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/auth.service.ts line 257 at r3 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

Thanks. That makes sense. When you try it now the logo provided will always be used.

I confirm that the SF logo is used in both the useBranding or not-useBranding cases.
Since in both cases the same logo and words are shown, I wonder if SF-3117 should be a SF fix after all? Oh I guess for other applications using the auth0 tennant, they might still want the SIL logo.
But perhaps now the useBranding might not be meaningful?

RaymondLuong3

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @Github-advanced-security[bot] and @marksvc)

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/auth.service.ts line 257 at r3 (raw file):

Previously, marksvc wrote…

I confirm that the SF logo is used in both the useBranding or not-useBranding cases.
Since in both cases the same logo and words are shown, I wonder if SF-3117 should be a SF fix after all? Oh I guess for other applications using the auth0 tennant, they might still want the SIL logo.
But perhaps now the useBranding might not be meaningful?

Aha, yes I see. By just providing the logo we do not need to specify the useBranding parameter. I've removed it.

marksvc

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Github-advanced-security[bot])

Nateowami · 2025-01-17T15:37:03Z

@RaymondLuong3 Is this PR title still accurate?

RaymondLuong3 · 2025-01-17T15:54:54Z

Yes, essentially still accurate on the user facing level.

Nateowami · 2025-01-18T02:15:04Z

I couldn't really understand how this is "adding an option;" it seems more so to be just "remove branding." Maybe an option was added on the Auth0 side? I see an option on the Auth0 side that's an unmerged PR, meaning this PR isn't really ready to merge, if I'm understanding things correctly. I almost merged this this morning but the code changes and the title didn't seem to match, which is the only reason I held off.

RaymondLuong3 added the testing not required label Jan 8, 2025

github-advanced-security bot found potential problems Jan 8, 2025

View reviewed changes

marksvc self-assigned this Jan 9, 2025

marksvc requested changes Jan 9, 2025

View reviewed changes

RaymondLuong3 force-pushed the feature/no-branding branch from 519eb72 to 5be3217 Compare January 9, 2025 22:11

marksvc approved these changes Jan 9, 2025

View reviewed changes

RaymondLuong3 commented Jan 9, 2025

View reviewed changes

marksvc approved these changes Jan 9, 2025

View reviewed changes

RaymondLuong3 force-pushed the feature/no-branding branch from 5be3217 to 9a9d98d Compare January 9, 2025 23:35

RaymondLuong3 commented Jan 9, 2025

View reviewed changes

RaymondLuong3 requested a review from marksvc January 13, 2025 22:57

SF-3117 Add option to remove branding from auth0 page

cea0ea1

RaymondLuong3 force-pushed the feature/no-branding branch from 74a063f to cea0ea1 Compare January 15, 2025 21:13

marksvc approved these changes Jan 15, 2025

View reviewed changes

@@ -1,2 +1,3 @@
             import { Injectable } from '@angular/core';
+            import { URL } from 'url';
             import { Router } from '@angular/router';
@@ -247,4 +248,5 @@
                 const ui_locales: string = language;
-                const useBranding: boolean =
-                  this.locationService.origin.includes('scriptureforge.org') || this.locationService.origin.includes('localhost');
+                const allowedHosts = ['scriptureforge.org', 'localhost'];
+                const url = new URL(this.locationService.origin);
+                const useBranding: boolean = allowedHosts.includes(url.host);
                 const auth0Parameters: xForgeAuth0Parameters = {

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SF-3117 Add option to remove branding from auth0 page #2937

SF-3117 Add option to remove branding from auth0 page #2937

RaymondLuong3 commented Jan 8, 2025 •

edited by Nateowami

Loading

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Nateowami Jan 8, 2025

RaymondLuong3 Jan 8, 2025

codecov bot commented Jan 8, 2025 •

edited

Loading

marksvc left a comment

marksvc commented Jan 9, 2025

Nateowami commented Jan 9, 2025

marksvc commented Jan 9, 2025

marksvc left a comment

RaymondLuong3 left a comment

marksvc left a comment

RaymondLuong3 left a comment

marksvc left a comment

Nateowami commented Jan 17, 2025

RaymondLuong3 commented Jan 17, 2025

Nateowami commented Jan 18, 2025

SF-3117 Add option to remove branding from auth0 page #2937

Are you sure you want to change the base?

SF-3117 Add option to remove branding from auth0 page #2937

Conversation

RaymondLuong3 commented Jan 8, 2025 • edited by Nateowami Loading

Nateowami Jan 8, 2025

Choose a reason for hiding this comment

RaymondLuong3 Jan 8, 2025

Choose a reason for hiding this comment

codecov bot commented Jan 8, 2025 • edited Loading

Codecov Report

marksvc left a comment

Choose a reason for hiding this comment

marksvc commented Jan 9, 2025

Nateowami commented Jan 9, 2025

marksvc commented Jan 9, 2025

marksvc left a comment

Choose a reason for hiding this comment

RaymondLuong3 left a comment

Choose a reason for hiding this comment

marksvc left a comment

Choose a reason for hiding this comment

RaymondLuong3 left a comment

Choose a reason for hiding this comment

marksvc left a comment

Choose a reason for hiding this comment

Nateowami commented Jan 17, 2025

RaymondLuong3 commented Jan 17, 2025

Nateowami commented Jan 18, 2025

RaymondLuong3 commented Jan 8, 2025 •

edited by Nateowami

Loading

codecov bot commented Jan 8, 2025 •

edited

Loading