-
Notifications
You must be signed in to change notification settings - Fork 5
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
[ new ] Adding Basic and Bearer Authorization #13
[ new ] Adding Basic and Bearer Authorization #13
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.
Lgtm
But no tests though for it |
And cant start workflow since im not maintainer role |
Let me look into adding a simple test or two. |
@jarlah This should be ready to look at again. |
tests/src/AuthorizationTest.idr
Outdated
basicauth' = base64DecodeString basicauth | ||
in case basicauth' of | ||
Nothing => | ||
idris_crash "couldn't base64 decode string." |
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.
First, good work 👊
But, I guess this is the one that fails ? Can you add different error messages for the two tests? So its easier to see what test fails? And i dont think using idris crash is how the other tests are designed to fail. I could be wrong of course, but i dont remember seing a test crash like this before.
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.
There was a small oversight on my part here (easy fix):
basicauth
here is Basic [base64encodedstring]
, but I was accidentally treating it as [base64encodedstring]
.
Thus I needed to break
basicauth
apart, and call base64DecodeString
only on the [base64encodedstring]
part.
This should be fixed via 24e30c8
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.
Hmm, it failed again, let me take a look.
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.
In a test, code like this is fine. In normal code i would have asked to create a method to keep my brain from trying to decode it. 🤣
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.
Are you running tests locally btw. I can try running the tests locally to see if its mismatch between workflow and local, when i get time. Not at computer right now
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.
no hacks in user config. You could try pack update-db? i destroyed my setup now and its bootstrapping pack .. taking ages
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.
if rebuilding you should get these two when running tests
[ warning ] Package base64 uses custom build hooks. Continue (yes/*no)?
yes
[ warning ] Package tls uses custom build hooks. Continue (yes/*no)?
yes
if not prompt is disabled which it is in github workflow
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.
@Matthew-Mosior after full rebuild of pack and with default contents in .pack/user/pack.toml, i can still run the tests. Maybe remove stuff from .pack/.cache/tls? Or maybe i should just reinstall pack completely and see if its an issue with clean start.
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 will test the stuff in repl later today ;) you could also use pack repl and test the individual operations
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've fixed this function, turned out to be a really silly oversight:
It had to do with the break
function.
break
wasn't stripping the leading white space in Basic YWxhZGRpbjpvcGVuc2VzYW1l
, causing base64DecodeString
to be called on completely erroneous input (should have been called on YWxhZGRpbjpvcGVuc2VzYW1l
, not YWxhZGRpbjpvcGVuc2VzYW1l
).
Using split
accomplished what needed to be done here.
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.
Some whitespace but otherwise green as a bean 🫘
"Fix whitespace or not fix whitespace. Its your choice." 😂 |
@Matthew-Mosior agree? ☝️ |
Thank you for fixing the white space, looks great! 👍 |
This PR focuses on adding
Authorization
header functionality:Basic authentication
andBearer authentication
parsing functionality.base64
encode ausername
andpassword
to create a properBasic authentication
Header
.Bearer authentication
Header
.