-
Notifications
You must be signed in to change notification settings - Fork 106
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
feat: add new local auth UX #54
Conversation
Wow. This is way nicer than I imagined. 😍 I wonder if the provider dropdown is redundant. Perhaps it should just use what's in the URL. Also keep in mind that it's possible that the URL can be rewritten by a routing rule. There'll also be more providers supported in the future via OpenID Connect integration (they can use any name). |
I am wondering how can we get the provider if the route is being rewritten to something like:
If we can't infer the provider from the URL:
WDYT? |
I don't think I know enough how the routing will work. The way I picture it in my head is that the routing code runs like a piece of middleware in the proxy's request pipeline. That rule would cause it to rewrite the URL to |
So maybe it would be useful to let the CLI knows what was the original route, when calling the routes engine. Here is how I see it:
When user requests
|
e4ee844
to
423a6ca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome. Thanks!
Is returning the info from /.auth/me
and adding the x-ms-claims-principal
header for function app part of this PR or will it be a separate one?
I'm planning on migrating However, when requesting click to show logs
|
|
Oh! That's something we already do. See: https://github.com/Azure/static-web-apps-cli/blob/main/src/proxy.ts#L142 Is this working as expected or is it broken? |
It was missing when I tried it yesterday with this PR. I'm using this app that has a function that dumps out the headers: https://github.com/anthonychu/swa-cli-js-sample |
I am tracking this in issue #63 |
Next up - found myself with an
Note the location header is |
Yep, can confirm that like @anthonychu I'm not seeing it on this branch Edit: actually, it's now working. I think it might be a bit flakey on whether it adds the header or not. |
Co-authored-by: Aaron Powell <me@aaron-powell.com>
Co-authored-by: Aaron Powell <me@aaron-powell.com>
@aaronpowell the issue that Anthony reported should have been fixed by 0bafc54 |
@anthonychu @aaronpowell I am planning to merge this PR today. |
Sorry haven't had a chance to test the latest changes. Yes please merge! Thanks. |
(work in progress)
Closes #47
cc @anthonychu @annaji-msft