mcgilman commented on code in PR #11868:
URL: https://github.com/apache/nifi/pull/11868#discussion_r4416441019
##########
nifi-frontend/src/main/frontend/apps/nifi/src/app/pages/login/state/access/access.effects.ts:
##########
@@ -59,7 +61,13 @@ export class AccessEffects {
this.actions$.pipe(
ofType(AccessActions.loginSuccess),
tap(() => {
- this.router.navigate(['/']);
+ const returnUrl = this.storageService.getReturnUrl();
Review Comment:
`loginSuccess$` only runs after the username/password form succeeds.
External authentication redirects through `loginUri` and never dispatches
`AccessActions.loginSuccess`; after returning from the identity provider, the
authentication guard instead dispatches `loadCurrentUserSuccess`.
Could we leave this effect responsible only for navigating to the
authenticated root route?
```typescript
this.router.navigate(['/']);
```
The stored URL can then be restored from a `loadCurrentUserSuccess` effect,
which covers both form and external authentication. It also avoids removing the
URL before the subsequent current-user request has succeeded.
##########
nifi-frontend/src/main/frontend/apps/nifi/src/app/state/current-user/current-user.effects.ts:
##########
@@ -75,6 +77,10 @@ export class CurrentUserEffects {
this.actions$.pipe(
ofType(UserActions.navigateToLogIn),
tap(() => {
+ const currentUrl = window.location.hash.substring(1);
+ if (currentUrl.startsWith('/') && !['/login',
'/logout-complete'].includes(currentUrl)) {
Review Comment:
This compares the entire hash and only excludes two routes. Consequently,
values such as `/login?returnTo=/settings`, `/error`, and `/404` can be stored
and restored after login.
Could we centralize the exact public-route check while preserving the
original URL, including its query string? For example, in `state/index.ts`:
```typescript
export const RETURN_URL = 'return-url';
const UNAUTHENTICATED_ROUTES: ReadonlySet<string> = new Set(['/login',
'/logout-complete', '/error', '/404']);
export function isAuthenticatedRoute(url: string): boolean {
const pathname = url.split('?')[0];
return !UNAUTHENTICATED_ROUTES.has(pathname);
}
```
Then capture it here, keeping the useful leading-slash validation:
```typescript
const currentUrl = window.location.hash.substring(1);
if (currentUrl.startsWith('/') && isAuthenticatedRoute(currentUrl)) {
this.sessionStorage.setItem<string>(RETURN_URL, currentUrl);
}
this.router.navigate(['/login']);
```
The corresponding root-level restore effect can handle both login methods
and remove the key before navigation so later polling successes are no-ops:
```typescript
restoreReturnUrl$ = createEffect(
() =>
this.actions$.pipe(
ofType(UserActions.loadCurrentUserSuccess),
tap(() => {
const returnUrl =
this.sessionStorage.getItem<string>(RETURN_URL);
if (returnUrl) {
this.sessionStorage.removeItem(RETURN_URL);
this.router.navigateByUrl(returnUrl);
}
})
),
{ dispatch: false }
);
```
##########
nifi-frontend/src/main/frontend/apps/nifi/src/app/service/storage.service.ts:
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { Injectable, inject } from '@angular/core';
+import { SessionStorageService } from '@nifi/shared';
+
+@Injectable({
+ providedIn: 'root'
+})
+export class StorageService {
Review Comment:
This class only forwards `setItem`, `getItem`, and `removeItem` to the
existing `SessionStorageService`. Could we use `SessionStorageService` directly
from `CurrentUserEffects` and remove this wrapper?
That would also let the tests focus on the behavior that matters: exact
exclusion of all four public routes (including query strings), preservation of
authenticated-route query strings, capture during `navigateToLogIn$`, restore
and removal during `loadCurrentUserSuccess`, and no navigation when a return
URL is absent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
