Skip to content

Remove OAuth1.0 and basic authentication #12

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

Merged
merged 8 commits into from
May 28, 2025
Merged

Remove OAuth1.0 and basic authentication #12

merged 8 commits into from
May 28, 2025

Conversation

HarelM
Copy link
Collaborator

@HarelM HarelM commented Jan 12, 2025

@HarelM HarelM requested a review from blackboxlogic January 12, 2025 07:58
@HarelM
Copy link
Collaborator Author

HarelM commented May 28, 2025

I think the auth hierarchy can be simplified as well as there is only one auth client...

@HarelM
Copy link
Collaborator Author

HarelM commented May 28, 2025

Maybe even unite the clients and in case no token is passed it will fail for the APIs that require authentication?
It's a bit radical but world probably reduce a lot of code complexity that currently exists...

@HarelM HarelM changed the title Remove OAuth1.0 Remove OAuth1.0 and basic authentication May 28, 2025
@HarelM
Copy link
Collaborator Author

HarelM commented May 28, 2025

I've simplified the hierarchy, not sure I want to unite the clients though.

@blackboxlogic
Copy link
Collaborator

I hope that's not a breaking change? Will people need to refactor their calling code when they update?

@HarelM
Copy link
Collaborator Author

HarelM commented May 28, 2025

It's a breaking change since we removed oauth1 and basic auth clients.
But I haven't changed any of the interfaces, so if someone is using this library as intended by using the factory and not doing any casting it should continue work as it did before.

@blackboxlogic
Copy link
Collaborator

Good. I'm fixing the tests to use oauth2.

@HarelM
Copy link
Collaborator Author

HarelM commented May 28, 2025

I ended up unifying the clients code to a single class as the non auth class had some authentication in it which was odd anyway, so now everything sits in a single class now and the exposed methods are defined by the relevant interfaces.
I've also moved most of the doc comments to the non-auth interface and used inheridoc in the implementation.
It's good to go from my side, although there's probably a need to manually test some of the auth client stuff as I haven't tested it...

Add test results to ignore.
@blackboxlogic
Copy link
Collaborator

I'm not setup to run the auth tests. You're free to merge when you're ready.

@HarelM
Copy link
Collaborator Author

HarelM commented May 28, 2025

Ok, I've used my OSM token to test a trace upload and then delete.
It worked as expected, I'm expecting other stuff to work as well, if they won't we can always fix what's needed.

@HarelM HarelM merged commit 796c370 into main May 28, 2025
1 check passed
@HarelM HarelM deleted the remove-auth1 branch May 28, 2025 21:07
@HarelM
Copy link
Collaborator Author

HarelM commented May 28, 2025

Seems like the Nuget token has expired and I don't think I have the relevant credentials.
@xivk any chance you can take care of this?

@blackboxlogic
Copy link
Collaborator

I noticed that the nuget secret had been recently updated (thanks @xivk) so I triggered the build/publish job. Nuget.org shows 1.0.5 now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove OAuth 1 and Basic auth
2 participants