loginUrl and loginText are hardcoded in the UI Gerrit UI ignored the settings in the auth section of the configuration [1]. In particular auth.loginUrl [2] should be used if present and auth.type is set to HTTP or HTTP_LDAP. Also auth.loginText [3] should be used if auth.loginUrl is set. [1]: https://gerrit-review.googlesource.com/Documentation/config-gerrit.html#auth [2]: https://gerrit-review.googlesource.com/Documentation/config-gerrit.html#auth.loginUrl [3]: https://gerrit-review.googlesource.com/Documentation/config-gerrit.html#auth.loginText Bug: Issue 290225204 Release-Notes: Fix loginUrl and loginText hardcoded in the UI Change-Id: I405259438edbfe582feef677a928d2cce07d2c28
diff --git a/polygerrit-ui/app/elements/core/gr-error-dialog/gr-error-dialog.ts b/polygerrit-ui/app/elements/core/gr-error-dialog/gr-error-dialog.ts index 5510aa3..ca0480c9 100644 --- a/polygerrit-ui/app/elements/core/gr-error-dialog/gr-error-dialog.ts +++ b/polygerrit-ui/app/elements/core/gr-error-dialog/gr-error-dialog.ts
@@ -33,6 +33,9 @@ @property({type: String}) loginUrl = '/login'; + @property({type: String}) + loginText = 'Sign in'; + @property({type: Boolean}) showSignInButton = false; @@ -82,7 +85,7 @@ return html` <gr-button id="signIn" class="signInLink" link="" slot="footer"> - <a class="signInLink" href=${this.loginUrl}>Sign in</a> + <a class="signInLink" href=${this.loginUrl}>${this.loginText}</a> </gr-button> `; }
diff --git a/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager.ts b/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager.ts index 570d393..c8a1c39 100644 --- a/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager.ts +++ b/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager.ts
@@ -113,6 +113,9 @@ @property({type: String}) loginUrl = '/login'; + @property({type: String}) + loginText = 'Sign in'; + private readonly reporting = getAppContext().reportingService; private readonly getAuthService = resolve(this, authServiceToken); @@ -164,6 +167,7 @@ id="errorDialog" @dismiss=${() => this.errorModal.close()} .loginUrl=${this.loginUrl} + .loginText=${this.loginText} ></gr-error-dialog> </dialog> <dialog
diff --git a/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.ts b/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.ts index 41cf920..4fee395 100644 --- a/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.ts +++ b/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.ts
@@ -116,6 +116,9 @@ @property({type: String}) loginUrl = '/login'; + @property({type: String}) + loginText = 'Sign in'; + @property({type: Boolean}) mobileSearchHidden = false; @@ -445,7 +448,7 @@ ></gr-icon> </div> ${this.renderRegister()} - <a class="loginButton" href=${this.loginUrl}>Sign in</a> + <a class="loginButton" href=${this.loginUrl}>${this.loginText}</a> <a class="settingsButton" href="${getBaseUrl()}/settings/"
diff --git a/polygerrit-ui/app/elements/gr-app-element.ts b/polygerrit-ui/app/elements/gr-app-element.ts index 759a9cb..a36f17a 100644 --- a/polygerrit-ui/app/elements/gr-app-element.ts +++ b/polygerrit-ui/app/elements/gr-app-element.ts
@@ -28,11 +28,11 @@ import './settings/gr-registration-dialog/gr-registration-dialog'; import './settings/gr-settings-view/gr-settings-view'; import './core/gr-notifications-prompt/gr-notifications-prompt'; -import {getBaseUrl} from '../utils/url-util'; +import {loginUrl} from '../utils/url-util'; import {navigationToken} from './core/gr-navigation/gr-navigation'; import {getAppContext} from '../services/app-context'; import {routerToken} from './core/gr-router/gr-router'; -import {AccountDetailInfo, NumericChangeId} from '../types/common'; +import {AccountDetailInfo, NumericChangeId, ServerInfo} from '../types/common'; import { constructServerErrorMsg, GrErrorManager, @@ -75,6 +75,7 @@ import {modalStyles} from '../styles/gr-modal-styles'; import {AdminChildView, createAdminUrl} from '../models/views/admin'; import {ChangeChildView, changeViewModelToken} from '../models/views/change'; +import {configModelToken} from '../models/config/config-model'; interface ErrorInfo { text: string; @@ -135,8 +136,6 @@ @state() private mobileSearch = false; - @state() private loginUrl = '/login'; - @state() private loadRegistrationDialog = false; @state() private loadKeyboardShortcutsDialog = false; @@ -153,6 +152,9 @@ @state() private theme = AppTheme.AUTO; + @state() + serverConfig?: ServerInfo; + readonly getRouter = resolve(this, routerToken); private readonly getNavigation = resolve(this, navigationToken); @@ -171,6 +173,8 @@ private readonly getChangeViewModel = resolve(this, changeViewModelToken); + private readonly getConfigModel = resolve(this, configModelToken); + constructor() { super(); @@ -183,9 +187,7 @@ this.addEventListener('dialog-change', e => { this.handleDialogChange(e as CustomEvent<DialogChangeEventDetail>); }); - document.addEventListener('location-change', () => - this.handleLocationChange() - ); + document.addEventListener('location-change', () => this.requestUpdate()); document.addEventListener('gr-rpc-log', e => this.handleRpcLog(e)); this.shortcuts.addAbstract(Shortcut.OPEN_SHORTCUT_HELP_DIALOG, () => this.showKeyboardShortcuts() @@ -220,6 +222,14 @@ subscribe( this, + () => this.getConfigModel().serverConfig$, + config => { + this.serverConfig = config; + } + ); + + subscribe( + this, () => this.getUserModel().preferenceTheme$, theme => { this.theme = theme; @@ -259,7 +269,6 @@ const resizeObserver = this.getBrowserModel().observeWidth(); resizeObserver.observe(this); - this.updateLoginUrl(); this.reporting.appStarted(); this.getRouter().start(); @@ -399,7 +408,8 @@ <gr-endpoint-decorator name="plugin-overlay"></gr-endpoint-decorator> <gr-error-manager id="errorManager" - .loginUrl=${this.loginUrl} + .loginUrl=${loginUrl(this.serverConfig?.auth)} + .loginText=${this.serverConfig?.auth.login_text ?? 'Sign in'} ></gr-error-manager> <gr-plugin-host id="plugins"></gr-plugin-host> `; @@ -414,7 +424,8 @@ @mobile-search=${this.mobileSearchToggle} @show-keyboard-shortcuts=${this.showKeyboardShortcuts} .mobileSearchHidden=${!this.mobileSearch} - .loginUrl=${this.loginUrl} + .loginUrl=${loginUrl(this.serverConfig?.auth)} + .loginText=${this.serverConfig?.auth.login_text ?? 'Sign in'} ?aria-hidden=${this.footerHeaderAriaHidden} > </gr-main-header> @@ -688,35 +699,6 @@ } } - private handleLocationChange() { - this.updateLoginUrl(); - } - - private updateLoginUrl() { - const baseUrl = getBaseUrl(); - if (baseUrl) { - // Strip the canonical path from the path since needing canonical in - // the path is unneeded and breaks the url. - this.loginUrl = - baseUrl + - '/login/' + - encodeURIComponent( - '/' + - window.location.pathname.substring(baseUrl.length) + - window.location.search + - window.location.hash - ); - } else { - this.loginUrl = - '/login/' + - encodeURIComponent( - window.location.pathname + - window.location.search + - window.location.hash - ); - } - } - // private but used in test paramsChanged() { const viewsToCheck = [GerritView.SEARCH, GerritView.DASHBOARD];
diff --git a/polygerrit-ui/app/elements/gr-app_test.ts b/polygerrit-ui/app/elements/gr-app_test.ts index 05ee9ed..6b97c85 100644 --- a/polygerrit-ui/app/elements/gr-app_test.ts +++ b/polygerrit-ui/app/elements/gr-app_test.ts
@@ -19,11 +19,12 @@ import {GrRouter, routerToken} from './core/gr-router/gr-router'; import {resolve} from '../models/dependency'; import {removeRequestDependencyListener} from '../test/common-test-setup'; +import {ReactiveElement} from 'lit'; suite('gr-app callback tests', () => { - const handleLocationChangeSpy = sinon.spy( - GrAppElement.prototype, - <any>'handleLocationChange' + const requestUpdateStub = sinon.stub( + ReactiveElement.prototype, + 'requestUpdate' ); const dispatchLocationChangeEventSpy = sinon.spy( GrRouter.prototype, @@ -34,9 +35,9 @@ await fixture<GrApp>(html`<gr-app id="app"></gr-app>`); }); - test("handleLocationChange in gr-app-element is called after dispatching 'location-change' event in gr-router", () => { + test("requestUpdate in reactive-element is called after dispatching 'location-change' event in gr-router", () => { dispatchLocationChangeEventSpy(); - assert.isTrue(handleLocationChangeSpy.calledOnce); + assert.isTrue(requestUpdateStub.calledOnce); }); });
diff --git a/polygerrit-ui/app/utils/url-util.ts b/polygerrit-ui/app/utils/url-util.ts index 4fa0e63..8267638 100644 --- a/polygerrit-ui/app/utils/url-util.ts +++ b/polygerrit-ui/app/utils/url-util.ts
@@ -3,7 +3,13 @@ * Copyright 2020 Google LLC * SPDX-License-Identifier: Apache-2.0 */ -import {BasePatchSetNum, PARENT, RevisionPatchSetNum} from '../types/common'; +import { + AuthInfo, + BasePatchSetNum, + PARENT, + RevisionPatchSetNum, +} from '../types/common'; +import {AuthType} from '../api/rest-api'; export function getBaseUrl(): string { // window is not defined in service worker, therefore no CANONICAL_PATH @@ -11,6 +17,37 @@ return self.CANONICAL_PATH || ''; } +/** + * Return the url to use for login. If the server configuration + * contains the `loginUrl` in the `auth` section then that custom url + * will be used, defaults to `/login` otherwise. + * + * @param authConfig the auth section of gerrit configuration if defined + */ +export function loginUrl(authConfig: AuthInfo | undefined): string { + const baseUrl = getBaseUrl(); + const customLoginUrl = authConfig?.login_url; + const authType = authConfig?.auth_type; + if ( + customLoginUrl && + (authType === AuthType.HTTP || authType === AuthType.HTTP_LDAP) + ) { + return customLoginUrl.startsWith('http') + ? customLoginUrl + : baseUrl + customLoginUrl; + } else { + // Strip the canonical path from the path since needing canonical in + // the path is unneeded and breaks the url. + const defaultUrl = `${baseUrl}/login/`; + const postFix = encodeURIComponent( + window.location.pathname.substring(baseUrl.length) + + window.location.search + + window.location.hash + ); + return defaultUrl + postFix; + } +} + export interface PatchRangeParams { patchNum?: RevisionPatchSetNum; basePatchNum?: BasePatchSetNum;
diff --git a/polygerrit-ui/app/utils/url-util_test.ts b/polygerrit-ui/app/utils/url-util_test.ts index c82cd70..e36719d 100644 --- a/polygerrit-ui/app/utils/url-util_test.ts +++ b/polygerrit-ui/app/utils/url-util_test.ts
@@ -3,19 +3,21 @@ * Copyright 2020 Google LLC * SPDX-License-Identifier: Apache-2.0 */ -import {BasePatchSetNum, RevisionPatchSetNum} from '../api/rest-api'; +import {AuthType, BasePatchSetNum, RevisionPatchSetNum} from '../api/rest-api'; import '../test/common-test-setup'; import { - getBaseUrl, encodeURL, + getBaseUrl, + getPatchRangeExpression, + loginUrl, + PatchRangeParams, singleDecodeURL, toPath, toPathname, toSearchParams, - getPatchRangeExpression, - PatchRangeParams, } from './url-util'; import {assert} from '@open-wc/testing'; +import {createAuth} from '../test/test-data-generators'; suite('url-util tests', () => { suite('getBaseUrl tests', () => { @@ -35,6 +37,47 @@ }); }); + suite('loginUrl tests', () => { + const authConfig = createAuth(); + + test('default url if auth.loginUrl is not defined', () => { + const current = encodeURIComponent( + window.location.pathname + window.location.search + window.location.hash + ); + assert.deepEqual(loginUrl(undefined), '/login/' + current); + assert.deepEqual(loginUrl(authConfig), '/login/' + current); + }); + + test('default url if auth type is not HTTP or HTTP_LDAP', () => { + const defaultUrl = + '/login/' + + encodeURIComponent( + window.location.pathname + + window.location.search + + window.location.hash + ); + const customLoginUrl = '/custom'; + authConfig.login_url = customLoginUrl; + + authConfig.auth_type = AuthType.LDAP; + assert.deepEqual(loginUrl(authConfig), defaultUrl); + authConfig.auth_type = AuthType.OPENID_SSO; + assert.deepEqual(loginUrl(authConfig), defaultUrl); + authConfig.auth_type = AuthType.OAUTH; + assert.deepEqual(loginUrl(authConfig), defaultUrl); + }); + + test('use auth.loginUrl when defined', () => { + const customLoginUrl = '/custom'; + authConfig.login_url = customLoginUrl; + + authConfig.auth_type = AuthType.HTTP; + assert.deepEqual(loginUrl(authConfig), customLoginUrl); + authConfig.auth_type = AuthType.HTTP_LDAP; + assert.deepEqual(loginUrl(authConfig), customLoginUrl); + }); + }); + suite('url encoding and decoding tests', () => { suite('encodeURL', () => { test('does not encode alphanumeric chars', () => {