Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Added ClaimsPrincipal binding on http object, debug tests VS Code task. #183

Closed
wants to merge 8 commits into from
Closed

Added ClaimsPrincipal binding on http object, debug tests VS Code task. #183

wants to merge 8 commits into from

Conversation

kashimiz
Copy link
Contributor

@kashimiz kashimiz commented Mar 1, 2019

[pack]

@kashimiz kashimiz closed this Mar 1, 2019
@kashimiz kashimiz reopened this Mar 1, 2019
@kashimiz kashimiz changed the title Added ClaimsPrincipal binding on http object, debug tests VS Code task. [pack] Added ClaimsPrincipal binding on http object, debug tests VS Code task. Mar 1, 2019
@kashimiz kashimiz changed the title [pack] Added ClaimsPrincipal binding on http object, debug tests VS Code task. Added ClaimsPrincipal binding on http object, debug tests VS Code task. Mar 2, 2019
mhoeger
mhoeger reviewed Mar 4, 2019
View reviewed changes
Copy link
Contributor

@mhoeger mhoeger left a comment •
edited

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
src/Converters.ts 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
@kashimiz kashimiz mentioned this pull request Mar 5, 2019
Closed
@mhoeger mhoeger mentioned this pull request Mar 7, 2019
Closed
ConnorMcMahon
ConnorMcMahon reviewed Mar 8, 2019
View reviewed changes
Copy link

@ConnorMcMahon ConnorMcMahon left a 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.

.vscode/tasks.json Show resolved Hide resolved
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
src/Converters.ts Outdated Show resolved Hide resolved
src/public/Interfaces.ts Outdated Show resolved Hide resolved
...o-end/Azure.Functions.NodejsWorker.E2E/Azure.Functions.NodejsWorker.E2E/HttpEndToEndTests.cs Show resolved Hide resolved
mhoeger
mhoeger reviewed Mar 8, 2019
View reviewed changes
src/public/Interfaces.ts Outdated Show resolved Hide resolved
mhoeger
mhoeger reviewed Mar 8, 2019
View reviewed changes
src/public/Interfaces.ts Outdated Show resolved Hide resolved
mhoeger
mhoeger reviewed Mar 8, 2019
View reviewed changes
src/public/Interfaces.ts Show resolved Hide resolved
.vscode/tasks.json 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
src/public/Interfaces.ts Outdated Show resolved Hide resolved
scripts/generateProtos.js Outdated Show resolved Hide resolved
test/ConvertersTests.ts Show resolved Hide resolved
@kashimiz
Copy link
Contributor Author

Waiting to merge until new CLI build incorporating this host build change goes out.

mhoeger
mhoeger approved these changes Mar 14, 2019
View reviewed changes
Copy link
Contributor

@mhoeger mhoeger left a 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!

src/public/Interfaces.ts Outdated Show resolved Hide resolved
mhoeger
mhoeger reviewed Mar 27, 2019
View reviewed changes
...e.Functions.NodejsWorker.E2E/Azure.Functions.NodejsWorker.E2E/Fixtures/FunctionAppFixture.cs Outdated
@@ -48,7 +51,6 @@ protected virtual void Dispose(bool disposing)
if (_funcProcess != null)
{
_logger.LogInformation($"Shutting down functions host for {Constants.FunctionAppCollectionName}");
_funcProcess.Kill();
Copy link
Contributor

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!

@mhoeger mhoeger mentioned this pull request Apr 3, 2019
Merged
@StefanSchoof StefanSchoof mentioned this pull request May 12, 2019
Closed
@StefanSchoof
Copy link

I want to use Claims in m Node Function. Can I have hope this gets in the near future?

@liliankasem
Copy link
Member

@kashimiz Is this something we still want to work on or can we close it?

@anthonychu
Copy link
Member

I created #421 which sounds like could be similar. /cc @ejizba

@ghost ghost added the no recent activity label Nov 30, 2021
@ghost
Copy link

ghost commented Nov 30, 2021

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.

@ghost ghost closed this Dec 7, 2021
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhoeger mhoeger mhoeger approved these changes

@ConnorMcMahon ConnorMcMahon ConnorMcMahon left review comments

@pragnagopa pragnagopa Awaiting requested review from pragnagopa

Assignees
No one assigned
Labels
Needs: Author Feedback no recent activity
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@kashimiz @StefanSchoof @liliankasem @anthonychu @pragnagopa @mhoeger @ConnorMcMahon