senaduka/old-new-wiaas
1
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

Enabled Json Web Token authorisation on backend, and frontend #4

Merged
nedimu merged 2 commits from login into master 2018-07-26 14:19:13 +02:00
Conversation 14 Commits 2 Files Changed 39 +7554 -2636
nedimu commented 2018-07-24 17:31:05 +02:00 (Migrated from gitlab.com)
Copy Link

jwt-authentication-for-wp-rest-api - is third party code, so it is not subjected to this code review

Smoke test:
Deploy app with docker (view README)

  1. Go to http://localhost:8081/wp-admin/plugins.php and activate JWT plugin
  2. Open new wiass frontend page
  3. type admin user and pass (wpUser) and log in

Expected : 1) Login request to backend will return 200 and token
2) App will try to open dashboard page and display a bunch of errors (other endpoints need to be fetched too in order to display this page prorperly)

jwt-authentication-for-wp-rest-api - is third party code, so it is not subjected to this code review Smoke test: Deploy app with docker (view README) 1) Go to http://localhost:8081/wp-admin/plugins.php and activate JWT plugin 1) Open new wiass frontend page 2) type admin user and pass (wpUser) and log in Expected : 1) Login request to backend will return 200 and token 2) App will try to open dashboard page and display a bunch of errors (other endpoints need to be fetched too in order to display this page prorperly)
nedimu commented 2018-07-24 17:35:36 +02:00 (Migrated from gitlab.com)
Copy Link

changed the description

changed the description
nedimu commented 2018-07-24 17:36:46 +02:00 (Migrated from gitlab.com)
Copy Link

changed the description

changed the description
akrdzic commented 2018-07-25 23:57:53 +02:00 (Migrated from gitlab.com)
Copy Link

Please use ${API_SERVER} for this url and not localhost.

But since it seems there my be bug in docker .env file regarding API_URL and REACT_APP_DEV_URL variables. (REACT_APP_DEV_URL has value http://localhost:8081/$%7BAPI_URL%7D for local enviroment and http://next.wiaas.saburly.com/$%7BAPI_URL%7D for test env) fix should also be done there.

Also we could update config.js to remove API_VERSION from API_SERVER since it is of no use for us. Then we can use ${API_SERVER} here.

Please use `${API_SERVER}` for this url and not localhost. But since it seems there my be bug in docker .env file regarding `API_URL` and `REACT_APP_DEV_URL` variables. (`REACT_APP_DEV_URL` has value `http://localhost:8081/$%7BAPI_URL%7D` for local enviroment and `http://next.wiaas.saburly.com/$%7BAPI_URL%7D` for test env) fix should also be done there. Also we could update `config.js` to remove `API_VERSION` from `API_SERVER` since it is of no use for us. Then we can use `${API_SERVER}` here.
akrdzic commented 2018-07-25 23:58:59 +02:00 (Migrated from gitlab.com)
Copy Link

The same for http://localhost//wp-json/jwt-auth/v1/token/validate regarding usage of ${API_SERVER}.

The same for `http://localhost//wp-json/jwt-auth/v1/token/validate` regarding usage of ${API_SERVER}.
akrdzic commented 2018-07-26 00:01:33 +02:00 (Migrated from gitlab.com)
Copy Link

Seems like response.data.data.status === 200 is the path here? The code inside never executed for me until this change.

Seems like `response.data.data.status === 200` is the path here? The code inside never executed for me until this change.
akrdzic commented 2018-07-26 00:16:25 +02:00 (Migrated from gitlab.com)
Copy Link

Only after activating, updates to .htaccess and wp-config.php as noted in plugin page (https://wordpress.org/plugins/jwt-authentication-for-wp-rest-api/) I could use the plugin on frontend.
Can we have the changes we need in .htaccess and wp-config.php commited (since automatic deployment will start after merge)?

Only after activating, updates to .htaccess and wp-config.php as noted in plugin page (https://wordpress.org/plugins/jwt-authentication-for-wp-rest-api/) I could use the plugin on frontend. Can we have the changes we need in .htaccess and wp-config.php commited (since automatic deployment will start after merge)?
nedimu commented 2018-07-26 10:25:48 +02:00 (Migrated from gitlab.com)
Copy Link

@akrdzic Thx for review, next time there is no need for you to go trough the whole thing and get it to work , if the smoke test fails. Just say it failed and where, and it is up to that person that submitted the code to find out. :)

The reason why everything worked for me locally is cause docker container frontend as you saw did point out to localhost (with port 80 that is my development copy of wordpress, that had all those thnigs like httaccess setup.) and not the wordpress in container. I did change the localhost to {API_SERVER} but forgot to save :D (lame excuse, it is cause I switch between two editors, one that autosaves, and one that doesn't )

Aniways I will fix everything, and rewrite smoke test

@akrdzic Thx for review, next time there is no need for you to go trough the whole thing and get it to work , if the smoke test fails. Just say it failed and where, and it is up to that person that submitted the code to find out. :) The reason why everything worked for me locally is cause docker container frontend as you saw did point out to localhost (with port 80 that is my development copy of wordpress, that had all those thnigs like httaccess setup.) and not the wordpress in container. I did change the localhost to `{API_SERVER}` but forgot to save :D (lame excuse, it is cause I switch between two editors, one that autosaves, and one that doesn't ) Aniways I will fix everything, and rewrite smoke test
nedimu commented 2018-07-26 13:48:17 +02:00 (Migrated from gitlab.com)
Copy Link

changed this line in version 2 of the diff

changed this line in [version 2 of the diff](https://gitlab.com/saburly/wiaas/new-wiaas/merge_requests/4/diffs?diff_id=20301909&start_sha=1f6b1043a44900be635dc2373a3dfa7c9e51a755#f7333065de75abf9238f5562fde05ccb693c2ad1_41_41)
nedimu commented 2018-07-26 13:48:18 +02:00 (Migrated from gitlab.com)
Copy Link

changed this line in version 2 of the diff

changed this line in [version 2 of the diff](https://gitlab.com/saburly/wiaas/new-wiaas/merge_requests/4/diffs?diff_id=20301909&start_sha=1f6b1043a44900be635dc2373a3dfa7c9e51a755#f7333065de75abf9238f5562fde05ccb693c2ad1_80_77)
nedimu commented 2018-07-26 13:48:19 +02:00 (Migrated from gitlab.com)
Copy Link

changed this line in version 2 of the diff

changed this line in [version 2 of the diff](https://gitlab.com/saburly/wiaas/new-wiaas/merge_requests/4/diffs?diff_id=20301909&start_sha=1f6b1043a44900be635dc2373a3dfa7c9e51a755#f7333065de75abf9238f5562fde05ccb693c2ad1_45_45)
nedimu commented 2018-07-26 13:48:19 +02:00 (Migrated from gitlab.com)
Copy Link

added 1 commit

  • 5c07b371 - Fixed docker env bugs, and issues from PR

Compare with previous version

added 1 commit <ul><li>5c07b371 - Fixed docker env bugs, and issues from PR</li></ul> [Compare with previous version](https://gitlab.com/saburly/wiaas/new-wiaas/merge_requests/4/diffs?diff_id=20301909&start_sha=1f6b1043a44900be635dc2373a3dfa7c9e51a755)
nedimu commented 2018-07-26 13:50:12 +02:00 (Migrated from gitlab.com)
Copy Link

changed the description

changed the description
akrdzic commented 2018-07-26 14:19:13 +02:00 (Migrated from gitlab.com)
Copy Link

merged

merged
akrdzic commented 2018-07-26 14:19:14 +02:00 (Migrated from gitlab.com)
Copy Link

mentioned in commit 63cce6100c

mentioned in commit 63cce6100c7133b8812687abf0effbb40058d297
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
No items
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
senaduka
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: senaduka/old-new-wiaas#4
Delete Branch "login"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?