Skip to content

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

Merged
jamesdaniels merged 3 commits into
angular:masterfrom
EdricChan03:feat/auth-guard-urltree-2144-2287
Nov 11, 2020
Merged

feat(auth-guard): add support for specifying a string to redirect to#2448
jamesdaniels merged 3 commits into
angular:masterfrom
EdricChan03:feat/auth-guard-urltree-2144-2287

Conversation

@EdricChan03

@EdricChan03 EdricChan03 commented May 9, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

EdricChan03 commented May 9, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

EdricChan03 commented Jun 13, 2020

Copy link
Copy Markdown
Contributor Author

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

NothingEverHappens
NothingEverHappens suggested changes Jun 15, 2020
View reviewed changes
Comment thread src/auth-guard/auth-guard.ts Resolved
Comment thread src/auth-guard/auth-guard.ts Outdated
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)

@NothingEverHappens NothingEverHappens Jun 15, 2020

Copy link
Copy Markdown
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?

@EdricChan03 EdricChan03 Jun 15, 2020

Copy link
Copy Markdown
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

NothingEverHappens commented Jun 15, 2020

Copy link
Copy Markdown
Contributor

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

@EdricChan03

EdricChan03 commented Jun 15, 2020

Copy link
Copy Markdown
Contributor Author

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

@NothingEverHappens

NothingEverHappens commented Jun 15, 2020

Copy link
Copy Markdown
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

EdricChan03 commented Jun 15, 2020

Copy link
Copy Markdown
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
@EdricChan03
fix(app-routing): use custom auth guards to implement redirect URLs
8c2b1de 
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

jamesdaniels commented Jun 23, 2020

Copy link
Copy Markdown
Contributor

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

@jamesdaniels jamesdaniels added this to the 6.1 milestone Jun 23, 2020
@EdricChan03

EdricChan03 commented Jun 24, 2020

Copy link
Copy Markdown
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

Reviewers

@NothingEverHappens NothingEverHappens Awaiting requested review from NothingEverHappens

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

6.1

Development

Successfully merging this pull request may close these issues.

feat(redirectUnauthorizedTo): allow passing query parameters Feature Request: Add queryParams into return url from custom AuthGuardPipe

4 participants

@EdricChan03 @NothingEverHappens @jamesdaniels @googlebot