Skip to content

feat(auth-guard): add support for specifying a string to redirect to #2448

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

Merged

Conversation

EdricChan03
Copy link
Contributor

@EdricChan03 EdricChan03 commented May 9, 2020

Closes #2287
Closes #2144

Checklist

Description

This PR introduces the ability to specify a UrlTree or a string to the return value of an AuthPipe and includes this feature in the existing redirect*To methods.

Previously, as noted in #2287, it was not possible to specify additional extras that could be passed to Router#createUrlTree.

Code sample

  • With a string:
    export const redirectUnauthorizedToLogin = (afterLoginRedirect: string) => redirectUnauthorizedTo(`/login?redirectTo=${afterLoginRedirect}`);
    // ...
    const routes: Route[] = [
      {
        path: 'items',
        component: ItemsComponent
        ...canActivate(redirectUnauthorizedToLogin('/items'))
      }
    ]
  • With a UrlTree (not currently possible atm):
    // TODO

@EdricChan03
Copy link
Contributor Author

EdricChan03 commented May 9, 2020

Seems that I did not think through the UrlTree portion - how should this be implemented?

EDIT: I've temporarily removed support for specifying a UrlTree

@EdricChan03 EdricChan03 changed the title feat(auth-guard): add support for specifying a UrlTree or a string to redirect to feat(auth-guard): add support for specifying a string to redirect to May 9, 2020
@EdricChan03 EdricChan03 force-pushed the feat/auth-guard-urltree-2144-2287 branch 2 times, most recently from 867478f to e5cb663 Compare May 9, 2020 11:06
@EdricChan03 EdricChan03 force-pushed the feat/auth-guard-urltree-2144-2287 branch from c6e9fab to cd67dd3 Compare May 23, 2020 13:59
@EdricChan03
Copy link
Contributor Author

/cc @jamesdaniels Thoughts? It's been about a month now

@@ -40,21 +40,23 @@ export class AngularFireAuthGuard implements CanActivate {
return this.authState.pipe(
take(1),
authPipeFactory(next, state),
map(can => typeof can == "boolean" ? can : this.router.createUrlTree(<any[]>can))
map(can => typeof can === 'boolean' ? can :
Array.isArray(can) ? this.router.createUrlTree(can) : this.router.parseUrl(can)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have a test for this new functionality?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current tests don't seem to test much... I'll see what I can do.

@NothingEverHappens
Copy link
Contributor

Note: those suggestions would be relevant once @jamesdaniels confirms the feasibility of this approach

@EdricChan03
Copy link
Contributor Author

I can't seem to rebase the changes - was the file updated in a recent commit?

@NothingEverHappens
Copy link
Contributor

I can't seem to rebase the changes - was the file updated in a recent commit?
Yes, lots of style fixes across the codebase, may have to merge manually

@EdricChan03 EdricChan03 force-pushed the feat/auth-guard-urltree-2144-2287 branch from cd67dd3 to 9eeba6c Compare June 15, 2020 15:58
@EdricChan03
Copy link
Contributor Author

I can't seem to rebase the changes - was the file updated in a recent commit?
Yes, lots of style fixes across the codebase, may have to merge manually

I've just fixed the conflicts locally.

EdricChan03 added a commit to EdricChan03/studybuddy-web-angular that referenced this pull request Jun 16, 2020
Until angular/angularfire#2448 is merged, query parameters can't currently be specified as arguments for `redirect*To` methods from the AngularFire auth guard module.
@EdricChan03 EdricChan03 marked this pull request as ready for review June 20, 2020 15:32
@jamesdaniels
Copy link
Member

I'm good with this approach, UrlTree support would be great.

@jamesdaniels jamesdaniels added this to the 6.1 milestone Jun 23, 2020
@EdricChan03
Copy link
Contributor Author

I'm good with this approach, UrlTree support would be great.

I don't see any way for the user to pass in a UrlTree to the auth guard?

@jamesdaniels jamesdaniels merged commit fe31191 into angular:master Nov 11, 2020
@EdricChan03 EdricChan03 deleted the feat/auth-guard-urltree-2144-2287 branch November 12, 2020 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants