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

Implementation "Embed" Field #304

Merged
merged 15 commits into from
Feb 11, 2019
Merged

Implementation "Embed" Field #304

merged 15 commits into from
Feb 11, 2019

Conversation

xiaohutai
Copy link
Member

@xiaohutai xiaohutai commented Feb 1, 2019

Fixes #124 till a certain degree.

I made a simple endpoint for getting the oembed information.
Not sure if we want everything that's from Bolt 3 too?

(This may need improvements with regards to UX. Though that may be a thing for a separate issue)

Implemented CSRF check, but maybe needs to be set in a re-usable before interceptor/trait? 🤔

@xiaohutai xiaohutai changed the title [WIP] Implementation "Embed" Field Implementation "Embed" Field Feb 1, 2019
src/Controller/Async/Embed.php Outdated Show resolved Hide resolved
Copy link
Member

@bobdenotter bobdenotter left a comment

Choose a reason for hiding this comment

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

Looks good to me. Only thing i'm not too happy about is /embed as endpoint. I've opened #305 to fix this for all async endpoints.

@JarJak
Copy link
Member

JarJak commented Feb 1, 2019

@xiaohutai CSRF check doesn't need to be implemented, just use what Symfony gives us (https://symfony.com/doc/current/security/csrf.html) (for ajax requests you can also use Nelmio cors bundle)

src/Controller/Async/Embed.php Outdated Show resolved Hide resolved
@xiaohutai
Copy link
Member Author

xiaohutai commented Feb 1, 2019

@bobdenotter I've made it so, that you only need to change the base URL once (in the Route annotation).

@JarJak I'm not sure what I have to do here. Could you show me how? Maybe it's something we have to do properly for ALL (ajax) requests.
Also: CORS does not prevent CSRF, right? So they're different, no?

Edit: https://security.stackexchange.com/questions/97825/is-cors-helping-in-anyway-against-cross-site-forgery

src/Controller/Async/Embed.php Outdated Show resolved Hide resolved
@JarJak
Copy link
Member

JarJak commented Feb 1, 2019

@xiaohutai Sorry you are right, CORS does not prevent CSRF, that's another topic.

Could you show me how?

I'd recommend start using Symfony Forms.

Maybe it's something we have to do properly for ALL (ajax) requests.

Yes, definitely.

Is this route behind the firewall?

src/Controller/Async/Embed.php Outdated Show resolved Hide resolved
@xiaohutai
Copy link
Member Author

I did a 400 on Invalid URL, but I'm not sure whether it should be 500.

src/Controller/Async/Embed.php Outdated Show resolved Hide resolved
src/Controller/Async/Embed.php Outdated Show resolved Hide resolved
@JarJak
Copy link
Member

JarJak commented Feb 1, 2019

needs tests

@xiaohutai
Copy link
Member Author

xiaohutai commented Feb 1, 2019

I need to more stuff for @Security to work.

Using:

@Security("has_role('ROLE_ADMIN')")

@xiaohutai xiaohutai force-pushed the feature/embed branch 2 times, most recently from bb238b2 to a5c03bc Compare February 1, 2019 16:33
@xiaohutai
Copy link
Member Author

@bobdenotter @JarJak
I've added a simple Kakunin test. But it doesn't work for me on the server here on work:

npm run kakunin -- --tags @wip
> cross-env NODE_ENV=prod kakunin "--tags" "@wip"

All reports have been deleted!
[14:36:48] I/launcher - Running 1 instances of WebDriver
[14:36:48] I/local - Starting selenium standalone server...
[14:36:48] I/local - Selenium standalone server started at http://192.168.3.3:33293/wd/hub
[14:36:49] E/launcher - unknown error: cannot find Chrome binary
  (Driver info: chromedriver=2.46.628388 (4a34a70827ac54148e092aafb70504c4ea7ae926),platform=Linux 4.9.0-8-amd64 x86_64) (WARNING: The server did not provide any stacktrace information)
Command duration or timeout: 28 milliseconds
Build info: version: '3.141.59', revision: 'e82be7d358', time: '2018-11-14T08:25:53'
System info: host: 'kwabbernoot', ip: '127.0.1.1', os.name: 'Linux', os.arch: 'amd64', os.version: '4.9.0-8-amd64', java.version: '1.8.0_181'
Driver info: driver.version: unknown
[14:36:49] E/launcher - WebDriverError: unknown error: cannot find Chrome binary
  (Driver info: chromedriver=2.46.628388 (4a34a70827ac54148e092aafb70504c4ea7ae926),platform=Linux 4.9.0-8-amd64 x86_64) (WARNING: The server did not provide any stacktrace information)
Command duration or timeout: 28 milliseconds
Build info: version: '3.141.59', revision: 'e82be7d358', time: '2018-11-14T08:25:53'
System info: host: 'kwabbernoot', ip: '127.0.1.1', os.name: 'Linux', os.arch: 'amd64', os.version: '4.9.0-8-amd64', java.version: '1.8.0_181'
Driver info: driver.version: unknown
    at Object.checkLegacyResponse (/path/to/bolt4/tests/e2e/node_modules/selenium-webdriver/lib/error.js:546:15)
    at parseHttpResponse (/path/to/bolt4/tests/e2e/node_modules/selenium-webdriver/lib/http.js:509:13)
    at doSend.then.response (/path/to/bolt4/tests/e2e/node_modules/selenium-webdriver/lib/http.js:441:30)
    at process._tickCallback (internal/process/next_tick.js:68:7)
From: Task: WebDriver.createSession()
    at Function.createSession (/path/to/bolt4/tests/e2e/node_modules/selenium-webdriver/lib/webdriver.js:769:24)
    at Function.createSession (/path/to/bolt4/tests/e2e/node_modules/selenium-webdriver/chrome.js:761:15)
    at createDriver (/path/to/bolt4/tests/e2e/node_modules/selenium-webdriver/index.js:170:33)
    at Builder.build (/path/to/bolt4/tests/e2e/node_modules/selenium-webdriver/index.js:626:16)
    at Local.getNewDriver (/path/to/bolt4/tests/e2e/node_modules/protractor/built/driverProviders/driverProvider.js:53:33)
    at Runner.createBrowser (/path/to/bolt4/tests/e2e/node_modules/protractor/built/runner.js:195:43)
    at q.then.then (/path/to/bolt4/tests/e2e/node_modules/protractor/built/runner.js:339:29)
    at _fulfilled (/path/to/bolt4/tests/e2e/node_modules/q/q.js:834:54)
    at /path/to/bolt4/tests/e2e/node_modules/q/q.js:863:30
    at Promise.promise.promiseDispatch (/path/to/bolt4/tests/e2e/node_modules/q/q.js:796:13)
[14:36:49] E/launcher - Process exited with error code 199
Protractor has finished
npm ERR! code ELIFECYCLE
npm ERR! errno 199
npm ERR! e2e@ kakunin: `cross-env NODE_ENV=prod kakunin "--tags" "@wip"`
npm ERR! Exit status 199
npm ERR!
npm ERR! Failed at the e2e@ kakunin script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/xiao/.npm/_logs/2019-02-04T13_36_49_200Z-debug.log

@JarJak
Copy link
Member

JarJak commented Feb 4, 2019

@xiaohutai do you have Chrome installed?
You can try setting up Kakunin to run on Firefox too, but I've never tried it out :)

@xiaohutai
Copy link
Member Author

I work on a server (without any browsers). I'm not working locally as I'm on Windows and that always had some issues with WAMP and variants.

@xiaohutai
Copy link
Member Author

Rebased with conflicts fixed.

@JarJak
Copy link
Member

JarJak commented Feb 4, 2019

@xiaohutai you can install chrome on server, kakunin runs it in headless mode

@bobdenotter
Copy link
Member

I ninja-fixed the Kakunin tests

@JarJak
Copy link
Member

JarJak commented Feb 6, 2019

@marcingajda could you have a look on Vue things here?

src/Controller/Async/Embed.php Outdated Show resolved Hide resolved
@xiaohutai xiaohutai merged commit 5626e1e into bolt:master Feb 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants