Token signatures and issuers #564
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
cthulhu-rider
commented
Mar 4, 2024
cthulhu-rider
commented
- based on and blocked by Support bearer token issuer neofs-api-go#446
cthulhu-rider
force-pushed
the
feature/bearer-issuer
branch
2 times, most recently
from
c006761
to
bb870dc
Compare
cthulhu-rider
requested review from
roman-khimov,
smallhive and
carpawell
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #564 +/- ##
==========================================
+ Coverage 68.15% 68.16% +0.01%
==========================================
Files 122 122
Lines 9965 9987 +22
==========================================
+ Hits 6792 6808 +16
- Misses 2799 2804 +5
- Partials 374 375 +1 ☔ View full report in Codecov by Sentry. |
carpawell
reviewed
Mar 5, 2024
bearer/bearer.go
Outdated
| // SignByIssuer combines [Token.Sign] and [Token.SetIssuer] in one call. Use | ||
| // this method for stable authorization in the system. SignByUser should not be | ||
| // mixed with the mentioned methods. | ||
| func (b *Token) SignByIssuer(signer user.Signer) error { |
There was a problem hiding this comment.
you do this one way and the next commit does it another way, why? why not just Sign that sets issuer?
There was a problem hiding this comment.
There was a problem hiding this comment.
I'd vote for Sign(signer user.Signer) unification, but I fear @amlwwalker will hate us for changing the interface again.
There was a problem hiding this comment.
made same interface
There was a problem hiding this comment.
I don't hate anything you guys are doing @roman-khimov just have to keep up! 😂
roman-khimov
reviewed
Mar 5, 2024
cthulhu-rider
force-pushed
the
feature/bearer-issuer
branch
5 times, most recently
from
b3cd074
to
4123c78
Compare
Dedicated field for the bearer token issuer was recently added to the protocol nspcc-dev/neofs-api#266. Now SDK provides getter and setter for it. Previously, `Token` type accepted `neofscrypto.Signer` parameter in `Sign` method to calculate and set signature of the bearer token. Obviously, the method did not set nonexistent issuer field. The only way to access the issuer was `ResolveIssuer` method resolving user ID from the public key. Now `Sign` method accepts parameter of `user.Signer` type to additionally set issuer field. This is a breaking change overall, but still needed for stable system authorization and library usage. `ResolveIssuer` method now dual: it starts like `Issuer` and falls back to the old behavior when field is missing. Signed-off-by: Leonard Lyubich <[email protected]>
Previously, `Sign` method set session token's issuer only if it had not been set yet. This could lead to the unexpected behavior on signing formed (completely or partially) token. Although this scenario is not common in NeoFS - the token is created once and then only read - this behavior does not make sense, so it's worth to be changed. Closes #546. Signed-off-by: Leonard Lyubich <[email protected]>
`Sign` method sets both issuer and signature of the token. There could be a need to set signature only, e.g. for testing. Now signature could be set via new method `SetSignature` also used by `Sign` itself. Refs #546. Signed-off-by: Leonard Lyubich <[email protected]>
cthulhu-rider
force-pushed
the
feature/bearer-issuer
branch
from
4123c78
to
bd31d70
Compare
roman-khimov
approved these changes
Mar 25, 2024
cthulhu-rider
force-pushed
the
feature/bearer-issuer
branch
from
d4f0257
to
bd31d70
Compare
|
integration test failures aint related to these changes. #572 |
carpawell
approved these changes
Mar 26, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.