tomkuijsten / restup

Webserver for universal windows platform (UWP) apps
MIT License
114 stars 48 forks source link

implement basic authorization module #104

Closed kreuzhofer closed 7 years ago

kreuzhofer commented 7 years ago

@tomkuijsten I have implemented the basic authorization. To activate it, you add an IAuthorizationProvider to the constructor of RestRouteHandler and StaticFileRouteHandler. So you can define which paths have authorization and which not. I would also like to add this to the wiki if you would make me a contributor of your project. You have to pass an ICredentialValidator to the constructor of the BasicAuthorizationProvider. There you can implement username and password check. Next I would implement another ICredentialValidator with pass through authentication that checks the username/password against the local administrator account, which is also used to log in to the admin interface of the pi. Btw I did not yet write any unit tests but I tested locally and on the raspberry and it works fine.

kreuzhofer commented 7 years ago

Ok, this time it get errors. You will need to check the conflicts

kreuzhofer commented 7 years ago

@tomkuijsten I tried to resolve conflicts in github but every time I klick on the Button above I just get the spinning wheel an nothing happens. Strange.

tomkuijsten commented 7 years ago

Ill sort this out after the holidays, thanks for the PR!

Jark commented 7 years ago

I think we should have unit tests before we merge it back though.

We shouldn't add any features that are not at least tested in a basic manner, they become a pain to support in future when we want to make changes in the structure.

kreuzhofer commented 7 years ago

@tomkuijsten Yes, Launch.json is not needed. @Jark I agree, I will add some Basic unit Tests for my Code to help out

kreuzhofer commented 7 years ago

@Jark Could you please first do the merge so I can Switch over to the development branch for further changes. This will help to reduce the merge conflicts.

Jark commented 7 years ago

Hi @kreuzhofer,

I've created a new branch feature-implement-basic-authorization-module based off development and cherry-picked your change over, it's here -> https://github.com/tomkuijsten/restup/tree/feature-implement-basic-authorization-module

If you update your fork from the tomkuijsten repo and checkout that branch then you can add your unit tests and create a new pull request.

Alternatively you can try and rebase your change on top of development instead of master and the pull request should update as well.

Hope this helps.

Cheers,

Jark

tomkuijsten commented 7 years ago

I agree on the unit-tests, but I don't mind merging first. It's the development branch, so we won't release it to the public and still be able to remove it.

kreuzhofer commented 7 years ago

I have created a new pull request with both the changes and new unit tests