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 ApiKey and OAuth authentication middleware #1207

Merged
merged 8 commits into from
Jan 5, 2019

Conversation

ybelenko
Copy link
Contributor

@ybelenko ybelenko commented Oct 9, 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.4.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

First of all, that feature is authentication only. It contains token/apiKey parsing and it validation. It doesn't contain token signing and all tasks related to authorization yet.

I've checked all secured endpoints with fake petstore spec. It turns out that some server ignores http headers with underscores, so header api_key doesn't work, while api-key/apikey works.

There are official list of Slim middlewares

Slim OAuth middleware looks like overkill to me, so I've ended up with my fork of Slim token authentication.

✔️ Maybe we should deprecate Slim Basic Authentication package and do all the job with Slim token authentication, to make implementation more consistent and reduce dependency list.

✔️ I've decided to move so called authenticators into external PHP classes. Three classes BasicAuthenticator, ApiKeyAuthenticator and OAuthAuthenticator should extend AbstractAuthenticator. Don't know if it's breaking changes or not. Probably breaking changes with fallback.

cc @jebentier @dkarlovi @mandrean @jfastnacht @ackintosh

@ybelenko
Copy link
Contributor Author

ybelenko commented Oct 9, 2018

#1092 related issue
Maybe @Newspaperman57 will reconsider and come back after this PR merge. 😄

@ybelenko
Copy link
Contributor Author

Major question to PHP committee, can I drop Basic authentication middleware which has been presented in previous releases and use Token authentication middleware for all kind of tokens.

@wing328
Copy link
Member

wing328 commented Nov 4, 2018

Major question to PHP committee, can I drop Basic authentication middleware which has been presented in previous releases and use Token authentication middleware for all kind of tokens.

Sure but let's target 4.0.x branch, which will be released in Dec.

@ackintosh
Copy link
Contributor

That sounds good to me. 👍

@wing328 wing328 modified the milestones: 3.4.0, 4.0.0 Nov 15, 2018
@ybelenko ybelenko force-pushed the slim_token_authentication branch 2 times, most recently from ddeccdb to 5da2d83 Compare December 26, 2018 01:39
@ybelenko ybelenko removed the WIP Work in Progress label Dec 26, 2018
@ybelenko ybelenko force-pushed the slim_token_authentication branch from 5da2d83 to e570200 Compare December 27, 2018 02:59
This commit will be dropped, when official repo approves submitted PRs.
Right now it's for test purposes only.
Considered to use dyorg/slim-token-authentication for all authentication
schemes. User needs to decode and parse Basic token himself, but it's
pretty simple task and there are many code examples in
the web. Most of time solution is two lines of code.
I've changed PHP version to 7 and updated comments to follow  main
recommendations. Used PHPCodesniffer rules are Generic.Commenting,
Squiz.Commenting, PEAR.Commenting. Of course I applied only reasonable
sniffs from this standards.

@category tag has been deleted as deprecated accordingly to
phpDocumentor offical docs.

Ref: http://docs.phpdoc.org/references/phpdoc/tags/category.html
@ybelenko ybelenko force-pushed the slim_token_authentication branch from e570200 to 58b7bd8 Compare December 30, 2018 02:44
@ybelenko
Copy link
Contributor Author

ybelenko commented Jan 3, 2019

Ref to related PR in Slim Token Authentication repo:
Add middleware to route or group of routes

Copy link
Contributor

@ackintosh ackintosh left a comment

Choose a reason for hiding this comment

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

b5a60c6

This commit will be dropped, when official repo approves submitted PRs.
Right now it's for test purposes only.

Thanks for the details. Looks good to me. 👍

@ackintosh ackintosh merged commit fa9bd1f into OpenAPITools:master Jan 5, 2019
@ybelenko ybelenko deleted the slim_token_authentication branch January 5, 2019 07:07
A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
)

* [Slim] Add fork of token middleware

This commit will be dropped, when official repo approves submitted PRs.
Right now it's for test purposes only.

* [Slim] Adds token middleware to template

* [Slim] Move auth implementation to external classes

* [Slim] Update readme

* [Slim] Add config example

* [Slim] Remove deprecated package

Considered to use dyorg/slim-token-authentication for all authentication
schemes. User needs to decode and parse Basic token himself, but it's
pretty simple task and there are many code examples in
the web. Most of time solution is two lines of code.

* [Slim] Format phpdoc comments

I've changed PHP version to 7 and updated comments to follow  main
recommendations. Used PHPCodesniffer rules are Generic.Commenting,
Squiz.Commenting, PEAR.Commenting. Of course I applied only reasonable
sniffs from this standards.

@category tag has been deleted as deprecated accordingly to
phpDocumentor offical docs.

Ref: http://docs.phpdoc.org/references/phpdoc/tags/category.html

* [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.

3 participants