-
Notifications
You must be signed in to change notification settings - Fork 43
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
Added ClaimsPrincipal binding on http object, debug tests VS Code task. #183
Conversation
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.
A few comments but mostly looks good!
You may also want to consider adding a property to Interfaces.ts that is specific to the HttpRequest from the ClaimsPrincipal binding. It could be an optional param that includes documentation that says "when the App Setting WEBSITE_AUTH_ENABLED
is set, this will appear. Otherwise, undefined." You could also make an interface that extends off of HttpRequest that is some sort of AuthEnabledHttpRequest
instead.
Adding to a type to Interfaces.ts and doing npm run build will make corresponding changes in Interfaces.d.ts that we can publish with @azure/functions
azure-functions-language-worker-protobuf/src/proto/FunctionRpc.proto
Outdated
Show resolved
Hide resolved
azure-functions-language-worker-protobuf/src/proto/identity/ClaimsIdentityRpc.proto
Outdated
Show resolved
Hide resolved
...o-end/Azure.Functions.NodejsWorker.E2E/Azure.Functions.NodejsWorker.E2E/HttpEndToEndTests.cs
Outdated
Show resolved
Hide resolved
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.
I think it looks good, just want some clarification before approving.
azure-functions-language-worker-protobuf/src/proto/FunctionRpc.proto
Outdated
Show resolved
Hide resolved
azure-functions-language-worker-protobuf/src/proto/FunctionRpc.proto
Outdated
Show resolved
Hide resolved
...o-end/Azure.Functions.NodejsWorker.E2E/Azure.Functions.NodejsWorker.E2E/HttpEndToEndTests.cs
Show resolved
Hide resolved
azure-functions-language-worker-protobuf/src/proto/FunctionRpc.proto
Outdated
Show resolved
Hide resolved
...o-end/Azure.Functions.NodejsWorker.E2E/Azure.Functions.NodejsWorker.E2E/HttpEndToEndTests.cs
Outdated
Show resolved
Hide resolved
d5d08f5
to
1212ebd
Compare
Waiting to merge until new CLI build incorporating this host build change goes out. |
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.
One nit, but otherwise looks great! Thanks!
1212ebd
to
bfccc0c
Compare
@@ -48,7 +51,6 @@ protected virtual void Dispose(bool disposing) | |||
if (_funcProcess != null) | |||
{ | |||
_logger.LogInformation($"Shutting down functions host for {Constants.FunctionAppCollectionName}"); | |||
_funcProcess.Kill(); |
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.
I think this should be brought back? I think it's causing test executions to run for ~1 hr after tests have run!
I want to use Claims in m Node Function. Can I have hope this gets in the near future? |
…e-worker-protobuf. Branch: dev. Commit: 6933a72aec5389c00b50ca32671bac5720dee8c8
…e-worker-protobuf. Branch: dev. Commit: 2e9cf4b3ac3864e3f5db07dffefa751d38baa368
…erfaces to expose new properties. Updated protobuf compilation scripts.
…p>: string | undefined;
eaa24d3
to
baf659b
Compare
@kashimiz Is this something we still want to work on or can we close it? |
This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment. |
[pack]