search

anaynayak / buildnotify

A system tray based build status notification app for cctray.xml feeds.
https://anaynayak.github.io/buildnotify/
Other
29 stars 7 forks source link

Add Bearer token authentication type #129

Closed Deuchnord closed 4 years ago

Deuchnord commented 4 years ago

Fix #121.

Add a way to authenticate with a Bearer token instead of the classic username/password.

Edit: the new configuration fields do not break BC. If I run the last release of BuildNotify (v2.0.0) with a server configured with the changes on this PR, only the servers that use Bearer token are affected and throw a 401 Client Error (which is logic). Restarting then with the changes on this PR will then restore said server without error.

Deuchnord commented 4 years ago

Hi, thanks for your review!

I tried running it locally as well. Noticed a minor issue where the username/password gets blanked out of you switch the dropdown value. I don't expect people to do this often so ok without that fix.

That is actually expected: since we are speaking about using two completely different types of authentication, keeping the "password" value has no really sense when switching between both types, so the password field is simply dropped. If you think my assertion is not good, feel free to tell me and I'll reverse this behavior :slightly_smiling_face:

I'm not too sure why it didn't run tests for your PR but two tests are still failing . This should be a quick fix by adding one more parameter to the MockConnection class.

Sorry, I'm a bit newbie on Python and don't know anything about testing on this language (I'm more familiar of PHP 😛)... Do you have any resources I can read about this, so I can run and fix them?

anaynayak commented 4 years ago

Hi, thanks for your review!

I tried running it locally as well. Noticed a minor issue where the username/password gets blanked out of you switch the dropdown value. I don't expect people to do this often so ok without that fix.

That is actually expected: since we are speaking about using two completely different types of authentication, keeping the "password" value has no really sense when switching between both types, so the password field is simply dropped. If you think my assertion is not good, feel free to tell me and I'll reverse this behavior 🙂

This is fine. I don't think we need to change it.

I'm not too sure why it didn't run tests for your PR but two tests are still failing . This should be a quick fix by adding one more parameter to the MockConnection class.

Sorry, I'm a bit newbie on Python and don't know anything about testing on this language (I'm more familiar of PHP 😛)... Do you have any resources I can read about this, so I can run and fix them?

Currently I'm using tox for running tests which is internally running them via pytest . If you run tox locally (pip install if it is missing), you will find test/projects_test.py failing. The reason for the same is that i'm using a MockHttpConnection class at https://github.com/Deuchnord/buildnotify/blob/feat/bearer-token-authentication/test/projects_test.py#L43 which now will need one more parameter in the connect method.

Adding that parameter should fix both tests for you.

Deuchnord commented 4 years ago

Oh, OK, I see! I'm looking at that tonight (UTC+2), thanks :)