Skip to content
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

[Slim] Add Basic authentication middleware #606

Merged
merged 7 commits into from
Jul 31, 2018

Conversation

ybelenko
Copy link
Contributor

@ybelenko ybelenko commented Jul 20, 2018

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: master, 3.1.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Basic auth middleware verifies token from Authorization header.
If token not verified it returns 401 http status code and quits script execution.
User needs to implement verifyCredentials method in AuthBasic.php. By default this method always returns false to avoid security holes on production. My code swapped with tuupola/slim-basic-auth package.
For a quick start, user needs to implement own authenticator in SlimRouter class:

$basicAuth = new HttpBasicAuthentication([
    "secure" => false,
    "authenticator" => function ($arguments) {
        $user = $arguments["user"];
        $password = $arguments["password"];
        // write your own code here, compare credentials with database etc.
        return false;
    }
]);

Test endpoint with Basic auth:

$ curl -X POST "http://petstore.swagger.io:80/v2/fake" \
    -H "accept: application/json" \
    -H "Content-Type: application/x-www-form-urlencoded" \
    -d "number=33&double=68.9&pattern_without_delimiter=FooBar&byte=fdfdfdf" -i

Output:

HTTP/1.1 401 Unauthorized
Date: Tue, 24 Jul 2018 08:54:42 GMT
Server: Apache/2.4.23 (Win64) PHP/7.2.7
X-Powered-By: PHP/7.2.7
WWW-Authenticate: Basic realm="Protected"
Content-Length: 0

cc @jebentier @dkarlovi @mandrean @jfastnacht @ackintosh

@ybelenko ybelenko changed the title [Slim] Add Basic authentication middleware [WIP][Slim] Add Basic authentication middleware Jul 20, 2018
@ybelenko
Copy link
Contributor Author

ybelenko commented Jul 20, 2018

At the last moment I've found slim-basic-auth package.
Obviously this package more stable than my script, but I've noticed few disadvantages:

  1. Big dependency list
  2. Hardcoded 401 response when credentials doesn't match
  3. Too many settings
  4. Plugin allows unsecure connection for only relaxed hosts
  5. We need two more middlewares(ApiKey, OAuth) and I don't think that existen plugins will follow same code patterns as described one.

@ybelenko
Copy link
Contributor Author

@wing328 @ackintosh One more thing. If we use 3rd party package then user have only settings without access to middleware source files. I'm still thinking what approach I should use.

@wing328
Copy link
Member

wing328 commented Jul 23, 2018

If we use 3rd party package then user have only settings without access to middleware source files

Normally users shouldn't need to modify the source code of slim-basic-auth (or other PHP libs). If slim-basic-auth fails to meet their requirement somehow, I would suggest opening an issue instead as slim-basic-auth is pretty active and likely the owners of slim-basic-auth can address that.

Here are my feedback (although I've not played with slim-basic-auth before):

  • Big dependency list (If we're doing it ourselves, we may end up having a big dependency list as we cater more and more use cases. Anyway, please open an issue ot see if the dependency list can be further trimmed down)
  • Hardcoded 401 response when credentials doesn't match (To me 401 seems a reasonable default but please share with slim-basic-auth team if there are other response code you want to use by providing the details of your use case)
  • Too many settings (depends on how we look at it - users may find these settings useful in their use/edge cases)
  • Plugin allows unsecure connection for only relaxed hosts (please open an issue to share your feedback)
  • We need two more middlewares(ApiKey, OAuth) and I don't think that existen plugins will follow same code patterns as described one (I'm ok with that as I've not seen any API that needs both HTTP basic auth and APIKey/OAuth at the same time)

User needs to add own implementation to verifyCredentials method in AuthBasic.php.
I'm not sure about `middlewareSrcPath` variable. I'll fix it in following PRs
if path is broken.
Hope that notice in README catches attention and most of users will read it.
Package "tuupola/slim-basic-auth" 3.1.0 requires PHP 7, that's why I
set it's version to ^3.0.0 in Composer. Minimum version will be
3.0.0-rc.1 which supports PHP 5.5. I've tested build with PHP 7, it
would be nice to check build with PHP 5.5 someday.
Not sure about forward slash in path to SlimRouter class. I will fix it
in upcoming PRs if necessary.
@ybelenko ybelenko changed the title [WIP][Slim] Add Basic authentication middleware [Slim] Add Basic authentication middleware Jul 24, 2018
@ybelenko
Copy link
Contributor Author

I can remove revert commits and wipe off custom implementation from history if needed.

@wing328 wing328 removed the WIP Work in Progress label Jul 27, 2018
<directory suffix=".php">./lib/Api</directory>
<directory suffix=".php">./lib/Model</directory>
<directory suffix=".php">./lib//Api</directory>
<directory suffix=".php">./lib//Model</directory>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ybelenko I believe the double forward slash won't cause any issue. Please correct me if I'm wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, and what are benefits from double forward slash, may I ask? 😄

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I do not know. I spotted the double forward slash and just asking to make sure phpunit still works fine with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checked on Win8.1, phpunit works fine. These slash changes is result of #610 . I just use {{apiSrcPath}} and {{modelSrcPath}} which defined in AbstractPhpCodegen.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌

@wing328 wing328 merged commit 58e0946 into OpenAPITools:master Jul 31, 2018
@wing328 wing328 added this to the 3.2.0 milestone Jul 31, 2018
@ybelenko ybelenko deleted the slim_basic_authentication branch July 31, 2018 16:56
@jmini
Copy link
Member

jmini commented Aug 1, 2018

This PR breaks the Shippable CI build => ensure-up-to-date

@ybelenko
Copy link
Contributor Author

ybelenko commented Aug 1, 2018

@jmini Sad to hear it. Any ideas what's wrong and how to fix it?

@jmini
Copy link
Member

jmini commented Aug 1, 2018

@ybelenko thank you for asking, no problem.

I have run bin/utils/ensure-up-to-date and pushed corresponding commit: d8ea28e

It is now OK on master.

A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
* [Slim] Add Basic Authentication Middleware

User needs to add own implementation to verifyCredentials method in AuthBasic.php.

* [Slim] Update README template

I'm not sure about `middlewareSrcPath` variable. I'll fix it in following PRs
if path is broken.
Hope that notice in README catches attention and most of users will read it.

* Revert "[Slim] Update README template"

This reverts commit 204ee02.

* Revert "[Slim] Add Basic Authentication Middleware"

This reverts commit 6a8e030.

* [Slim] Add "tuupola/slim-basic-auth" package

Package "tuupola/slim-basic-auth" 3.1.0 requires PHP 7, that's why I
set it's version to ^3.0.0 in Composer. Minimum version will be
3.0.0-rc.1 which supports PHP 5.5. I've tested build with PHP 7, it
would be nice to check build with PHP 5.5 someday.

* [Slim] Update README template

Not sure about forward slash in path to SlimRouter class. I will fix it
in upcoming PRs if necessary.

* [Slim] Refresh samples
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants