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

[API] JWT Authentication #11174

Merged
merged 7 commits into from
Mar 5, 2020
Merged

[API] JWT Authentication #11174

merged 7 commits into from
Mar 5, 2020

Conversation

Zales0123
Copy link
Member

Q A
Branch? api
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Related tickets
License MIT

@Zales0123 Zales0123 added Feature New feature proposals. API APIs related issues and PRs. labels Mar 4, 2020
@Zales0123 Zales0123 requested a review from a team as a code owner March 4, 2020 09:44
print_header "Setting up JWT for API" "Sylius"
run_command "source .env.test"
run_command "openssl genrsa -out config/jwt/private-test.pem 4096 -algorithm rsa -passout env:JWT_PASSPHRAS rsa_keygen_bits:4096"
run_command "openssl pkey -in config/jwt/private-test.pem -out config/jwt/public-test.pem -pubout"
Copy link
Member

Choose a reason for hiding this comment

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

Should we expose this in our docs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably 👌

Copy link
Member Author

Choose a reason for hiding this comment

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

But as we don't have any docs for the new API right now (instead of swagger which, maybe, is enough 💃) I would add it later 🚀

Copy link
Member

Choose a reason for hiding this comment

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

That's one of the problems that people may have issues with running and testing it :( Let's move with this PR, but we need to provide this note in the installation guide.

Copy link
Member

Choose a reason for hiding this comment

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

Both private-test.pem and public-test.pem (for test environment) could be included in the repository
on .gitignore
!config/jwt/*-test.pem

Copy link
Member

Choose a reason for hiding this comment

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

Hey Loic, could you open a PR with fix?

Copy link
Member

Choose a reason for hiding this comment

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

@lchrusciel Yes here #11198 🥂

@Zales0123 Zales0123 changed the title [WIP][API] JWT Authentication [API] JWT Authentication Mar 4, 2020
);
$defaultHeaders = ['HTTP_ACCEPT' => 'application/ld+json'];
if ($this->sharedStorage->has('token')) {
$defaultHeaders['HTTP_Authorization'] = 'Bearer ' . $this->sharedStorage->get('token');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$defaultHeaders['HTTP_Authorization'] = 'Bearer ' . $this->sharedStorage->get('token');
$defaultHeaders['HTTP_Authorization'] = 'Bearer ' . $this->sharedStorage->get('securiity_token');

Copy link
Member Author

Choose a reason for hiding this comment

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

securiiiiiiiiity 💃

Copy link
Member

Choose a reason for hiding this comment

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

XD

@lchrusciel lchrusciel merged commit 522c179 into Sylius:api Mar 5, 2020
@lchrusciel
Copy link
Member

Thanks, Mateusz! 🥇

@lchrusciel
Copy link
Member

Part of #11250

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API APIs related issues and PRs. Feature New feature proposals.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants