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.

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

.NET: Always send metadata; don't fail if auth token not present #829

Merged
merged 2 commits into from
Jul 5, 2022

Conversation

geel9
Copy link
Member

@geel9 geel9 commented Jun 30, 2022

This PR changes .NET to always send message metadata in every call (even if unnecessary). Additionally, BuildMetadata[Async] no longer throws if authToken is not present; instead, it simply sends an empty auth header.

All in accordance with #822

@geel9 geel9 added the dotnet Issue is related to the .NET sdk label Jun 30, 2022
Copy link
Member

@fundthmcalculus fundthmcalculus left a comment

Choose a reason for hiding this comment

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

@sethjback any changes needed on the server side for this?

@sethjback
Copy link
Contributor

No - the server looks for metadata if it is required, otherwise it is ignored.

Copy link
Member

@fundthmcalculus fundthmcalculus left a comment

Choose a reason for hiding this comment

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

In light of wanting to send version information in the header, I think that we should probably always send the metadata Authorization as empty.

@codecov-commenter
Copy link

Codecov Report

Merging #829 (369c928) into main (50ce309) will increase coverage by 46.37%.
The diff coverage is 75.00%.

@@              Coverage Diff              @@
##               main     #829       +/-   ##
=============================================
+ Coverage     15.18%   61.55%   +46.37%     
=============================================
  Files           214       14      -200     
  Lines         35382      489    -34893     
  Branches       4357        0     -4357     
=============================================
- Hits           5371      301     -5070     
+ Misses        29335      188    -29147     
+ Partials        676        0      -676     
Impacted Files Coverage Δ
dotnet/Trinsic/AccountService.cs 52.38% <50.00%> (ø)
dotnet/Trinsic/ServiceBase.cs 94.73% <87.50%> (-2.49%) ⬇️
dotnet/Trinsic/ProviderService.cs 50.00% <100.00%> (-2.95%) ⬇️
...va/trinsic/services/provider/v1/InviteRequest.java
...sic/services/provider/v1/GetOberonKeyResponse.java
...emplates/v1/SearchCredentialTemplatesResponse.java
...c/services/provider/v1/UpdateEcosystemRequest.java
...n/trinsic/proto/trinsic/services/event/__init__.py
...ces/trustregistry/v1/UnregisterMemberResponse.java
...va/src/main/java/trinsic/services/ServiceBase.java
... and 194 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 50ce309...369c928. Read the comment docs.

@geel9
Copy link
Member Author

geel9 commented Jul 5, 2022

In light of wanting to send version information in the header, I think that we should probably always send the metadata Authorization as empty.

I changed it to not send the header at all because, upon reviewing the backend codebase, functionality depends on the presence of the header whatsoever -- not whether it's empty or not.

Although it depends on whether or not an empty header is serialized and sent down the wire? Likely language-specific?

@fundthmcalculus
Copy link
Member

In light of wanting to send version information in the header, I think that we should probably always send the metadata Authorization as empty.

I changed it to not send the header at all because, upon reviewing the backend codebase, functionality depends on the presence of the header whatsoever -- not whether it's empty or not.

Although it depends on whether or not an empty header is serialized and sent down the wire? Likely language-specific?

Good points. @sethjback thoughts on backend behavior? I agree, we shouldn't rely on language-specific behavior here. I think if the metadata is required, it checks it. If it isn't required, it ignores it (automatically becomes effectively anonymous).

@fundthmcalculus fundthmcalculus merged commit da6609d into main Jul 5, 2022
@fundthmcalculus fundthmcalculus deleted the JC/dotnet-always-metadata branch July 5, 2022 18:25
@sethjback
Copy link
Contributor

If we add the sdk / okapi version as metadata we will always want to send those - so the way it's implemented here makes sense: always call the build metadata. Set the sdk/okapi versions, and if appropriate authorization.

As for an empty authorization header - it would be easy to add a NoResult() return if the header is empty as well as missing. If that would save time/effort on all the SDKs that is probably the best route to go.

@geel9
Copy link
Member Author

geel9 commented Jul 5, 2022

Seems good to me -- I've created a PR against server to handle empty Auth headers @sethjback

@tmarkovski
Copy link
Member

tmarkovski commented Jul 5, 2022

The unintended side effect of this is that if, for some reason, the authToken is present, but invalid/expired/different_eco, the user won't be able to login, because the server will always try to check the authorization if the header is present.
In general, Authorization should be sent if the server endpoint is decorated with Authorize. If it's AllowAnonymous it's best not to send Authorization to prevent side effects.

I'm sorry, I missed the convo on this.

@sethjback
Copy link
Contributor

@tmarkovski would it affect login? If the authorization header is internally invalid it would, but if it checks out from a signature/timestamp perspective login should succeed since anonymous endpoints don't do additional authz.

It almost seems better as well to fail if there is an internally inconsistent auth header set, even on login, to give feedback to the sdk user.

It is not strictly correct to send an authorization header to anonymous endpoints. That being said, the server should handle the situation correctly.

Maybe I'm not fully appreciating the scenario here though...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dotnet Issue is related to the .NET sdk
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants