Fix CORS and add implicit grant

What does this MR do?

Adds missing CORS header and enables implicit oauth grant flow.

Why was this MR needed?

Web clients cannot login with oauth or password and are not allowed to use pagination X-Headers.

Does this MR meet the acceptance criteria?

• CHANGELOG entry added
• Tests
  • All builds are passing
• Conform by the style guides
• Branch has no merge conflicts with master (if you do - rebase it please)
• Squashed related commits together

What are the relevant issue numbers?

Fixes #19470 (closed), #20628 (closed), #2716 (closed)

You're right link header isn't used. I will remove it. What do you mean with other steps?

Thanks @medokin! I realise the mistake in my comment above now, but I don't think this is enough. In a web browser, this should use the implicit grant, which we don't currently support: http://docs.gitlab.com/ce/api/oauth2.html#web-application-flow / https://gitlab.com/gitlab-org/gitlab-ce/issues/20628. That is what we should be supporting, unless there's something I'm missing?

Without that, the JS application will have to leak its own client secret.

@smcgivern seems like a one liner: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/config/initializers/doorkeeper.rb#L81

Do we need docs for that?

Resolved all discussions

@medokin it seems that way. Do you have an actual client app that can be used to test this? I understand that it isn't really possible to write specs for this, but I'd like something we can test with and perhaps add to gitlab-qa. /cc @grzesiek

Also, without a test app, we can't really verify that this only works for the cases we want it to!

@smcgivern Yes I have an app I will provide when this MR is ready.

Added 1 commit:

• f2eb206b - Enable implicit grant flow

Added 1 commit:

• e50b401e - Updated CORS exposed headers

Changed title: WIP: Allow CORS on oauth login → WIP: Fix CORS and add implicit grant

Added 5 commits:

• 0066cbf9 - Prevent .form-actions element from leaking out of the main container
• b080f358 - Updated CHANGELOG
• d96e8959 - Fix due date example in slash commands documentation
• 59f39776 - Add angle brackets around due date in slash cmds docs
• cad99a2b - Add Changelog

Added 10 commits:

• cad99a2b...64366209 - 6 commits from branch gitlab-org:master
• 044d852a - Allow CORS on oauth login
• 1e5ca5cb - Enable implicit grant flow
• fe6a5c11 - Updated CORS exposed headers
• 330b10a1 - Add Changelog

Added 1 commit:

• c70a96be - Allow CORS on oauth login

Marked the task CHANGELOG entry added as completed

Marked the task Squashed related commits together as completed

Marked the task Branch has no merge conflicts with master (if you do - rebase it please) as completed

Marked the task Conform by the style guides as completed

Marked the task All builds are passing as completed

Marked the task Branch has no merge conflicts with master (if you do - rebase it please) as incomplete

Added 27 commits:

• c70a96be...2778dec1 - 26 commits from branch gitlab-org:master
• d9aee0ad - Allow CORS on oauth login

Unmarked this merge request as a Work In Progress

Marked the task Branch has no merge conflicts with master (if you do - rebase it please) as completed

@smcgivern I have an Emberjs app ready to test with. What will be the next step?

I've added checking CORS to gitlab-qa todos - gitlab-qa#3.

@medokin can you push your test app to a public repo somewhere? If not, it looks like we can use something like https://adodson.com/hello.js

Added ~164274 label

/cc @nick.thomas (since you've touched the CORS stuff recently)

@rymai this looks sensible to me. It's three different things:

• Fix missing CORS header on /oauth/token
• Expose pagination headers on /api/*
• Enable implicit grant auth flow in doorkeeper.

The first two are easy 👍 but the third is new to me 😊. I've scanned the RFC segments on the matter and it looks like we could enable it, but I'm lacking context on why we didn't in the first place.

Looks like implicit was disabled here: 984f8077

https://github.com/gitlabhq/gitlabhq/pull/9083

Doorkeeper disabled password and implicit flows in jan 2015: https://github.com/doorkeeper-gem/doorkeeper/commit/2115a5243b4663090b49c872a1837f6dda6b0b91 - v2.1.0. So when we upgraded to that version in 1c49c301, the comment became outdated and the implicit and resource owner password flows were disabled.

I don't think there should be a problem with re-enabling it, based on what I know so far, but that comment needs fixing and we have merge conflicts too (and more on the way).

@nick.thomas Thanks for investigating!

Added 1530 commits:

• d9aee0ad...166c6cd8 - 1524 commits from branch gitlab-org:master
• 443ac8a6 - Add organization field to user profile
• e34d04ec - Dispaly organization name at user page
• 206311d0 - Add User#organization to users api
• f6c9ed8c - Improvements to user organization field feature after code review
• dd2ce02d - Add extra check for api users spec
• 8cd6aa1e - Allow CORS on oauth login

Changed title: Fix CORS and add implicit grant → conflictsFix CORS and add implicit grant

Changed title: conflictsFix CORS and add implicit grant → Fix CORS and add implicit grant

Added 23 commits:

• 8cd6aa1e...52711b53 - 22 commits from branch gitlab-org:master
• a960414b - Allow CORS on oauth login

Fixed that comment. But I dont get why it still shows merge conflicts.

@medokin The merge conflict is most probably here. You can move your CHANGELOG entry to a random place among the 8.13 entries to avoid potential conflicts. ;)

Reassigned to @medokin

From @medokin:

Currently on vacation. I could get to it in March. The changes are minimal so I guess you can take over.

added ~480950 ~13884 oauth labels

mentioned in merge request !8606 (merged)

mentioned in issue #20628 (closed)

It seems we cannot see the diff anymore... I'm closing this for now. @medokin Feel free to reopen and update it, thanks!

closed

Hi @medokin, any chance you'll follow up on this M/R? If not I'll create a new one cherry picking your changes.

@empe feel free to create a new one. Currently I cannot fit it into my schedule.

mentioned in merge request !12384 (merged)