🐛 token authenticate #621
🐛 token authenticate #621
Conversation
roceb
force-pushed
the
bugfix/tokenAuthenticate
branch
from
1ed3563
to
e8af612
Compare
auth/builtin.go
Outdated
| if splitToken[0] != "Bearer" { | ||
| return "", errors.New("authentication header not of type bearer") | ||
| } | ||
| token := strings.TrimSpace(splitToken[1]) |
There was a problem hiding this comment.
This will result in a SEGV when requestToken="Bearer". no token.
parseToken() should return NotValid which will result in the request http=403 (forbidden) instead of 500.
auth/builtin.go
Outdated
|
|
||
| // Parse Token out of a string | ||
| func ParseToken(requestToken string) (string, error) { | ||
| splitToken := strings.Split(requestToken, " ") |
There was a problem hiding this comment.
Probably should:
requestToken = strings.TrimSpace(requestToken)
so that " Bearer " will be valid.
Also the "Bearer" header should be optional or, the binding.Client will be broken. Or, the binding.Client.send() needs to add the "Bearer".
I don't know if it's optional in JWT (or not).
There was a problem hiding this comment.
Bearer is for the type of schema and JWT is the encoding. JWT encodes to the Bearer schema. So unless there are plans to support different schemas, then Bearer should be included.
As far as:
requestToken = strings.TrimSpace(requestToken)
Is there a use case for " Bearer "? This seams like a bad formatted request and shouldn't be allowed.
There was a problem hiding this comment.
Bearer is for the type of schema and JWT is the encoding. JWT encodes to the Bearer schema. So unless there are plans to support different schemas, then Bearer should be included.
Yes, I understand the difference.
Is there a use case for " Bearer "? This seams like a bad formatted request and shouldn't be allowed.
Just suggesting that API could be tolerant of curl users trying to do the right thing.
That said, I suppose the standard is not that hard to follow.
|
We all have to start somewhere. |
roceb
force-pushed
the
bugfix/tokenAuthenticate
branch
from
651c0ba
to
62865af
Compare
Signed-off-by: Ross <ross@roceb.xyz>
Signed-off-by: Ross <ross@roceb.xyz>
- Used code style for return and formatting - Replaced string.split with string.field to avoid panic Signed-off-by: Ross <ross@roceb.xyz>
roceb
force-pushed
the
bugfix/tokenAuthenticate
branch
from
62865af
to
46a9ee4
Compare
|
Thanks for the PR and requested changes @roceb. We still need to change the Client.send() method like: Else, the binding will no longer work with our API. Unfortunately, I don't think our CI does not run with auth enabled and won't catch this. |
Signed-off-by: Ross <ross@roceb.xyz>
|
Thank you for your time and patience @jortel . I have updated Client.send() method. Let me know if there is anything else? |
Fix auth unit tests broken by: #621. Signed-off-by: Jeff Ortel <jortel@redhat.com>
fix for issue / #619
Missing Bearer token return error instead of panicking.
Also updated Client.send() method to work with bearer tokens.