- They are not included automatically since `API::Users` does not inherit from
`API::API`, as I initially assumed.
- Scopes declared in `API::API` are considered global (to the API), and need to
be included in all cases.
- Scope declarations of the form:
allow_access_with_scope :read_user, if: -> (request) { request.get? }
will only apply for `GET` requests
- Add a negative test to a `POST` endpoint in the `users` API to test this. Also
test for this case in the `AccessTokenValidationService` unit tests.
- Declaring an endpoint's scopes in a `before` block has proved to be
unreliable. For example, if we're accessing the `API::Users` endpoint - code
in a `before` block in `API::API` wouldn't be able to see the scopes set in
`API::Users` since the `API::API` `before` block runs first.
- This commit moves these declarations to the class level, since they don't need
to change once set.
The ProjectsFinder and GroupFinder both support the same set of params. And the
`/api/v4/projects` and `/api/v4/group/:id/projects` also support the same set of
params. But they do not match the Finder params. So use a helper method to
transform them.
- Currently, (for example) admins can't delete snippets for blocked users, which
is an unexpected limitation.
- We modify `authenticate!` to conduct the `access_api` policy check against the
`initial_current_user`, instead of the user being impersonated.
- Update CHANGELOG for !10842
In API V4 all endpoints were changed so Merge Requests and Issues
should be referred by iid, instead of id. Except the /notes endpoint
was forgotten. So change the endpoints from:
- /projects/:id/issues/:issue_id/notes
- /projects/:id/merge_requests/:merge_request_id/notes
To:
- /projects/:id/issues/:issue_iid/notes
- /projects/:id/merge_requests/:merge_request_iid/notes
For Project Snippets nothing changes.
- As opposed to the `id` that was previously being used.
- This brings the API routes closer to the web interface's routes.
- This is specific to API v4.
- As opposed to the issue `id` that was previously being used.
- This brings the API routes closer to the web interface's routes.
- This is specific to API v4.
It consolidates these endpoints:
- /projects
- /projects/owned
- /projects/visible
- /projects/starred
- /projects/all
Into the /projects endpoint using query parameters.
New endpoints are:
POST :project_id/(issues|merge_requests)/(:issue_id|:merge_request_id)/time_estimate"
POST :project_id/(issues|merge_requests)/(:issue_id|:merge_request_id)/reset_time_estimate"
POST :project_id/(issues|merge_requests)/(:issue_id|:merge_request_id)/add_spent_time"
POST :project_id/(issues|merge_requests)/(:issue_id|:merge_request_id)/reset_spent_time"
GET :project_id/(issues|merge_requests)/(:issue_id|:merge_request_id)/time_stats"
* We realized that headers were not set whenever we give 204
because `render_api_error!` doesn't preserve the headers.
* We also realized that `update_runner_info` would be called in
POST /builds/register every time therefore runner is updated
every time, ticking the queue, making this last_update didn't
work very well, and the test would be failing due to that.
This adds counters for build artifacts and LFS objects, and moves
the preexisting repository_size and commit_count from the projects
table into a new project_statistics table.
The counters are displayed in the administration area for projects
and groups, and also available through the API for admins (on */all)
and normal users (on */owned)
The statistics are updated through ProjectCacheWorker, which can now
do more granular updates with the new :statistics argument.
* master: (367 commits)
Set “Remove branch” button to default size
remove unused helper method
reduce common code even further to satisfy rake flay
remove button class size alteration from revert and cherry pick links
factor out common code to satisfy rake flay
homogenize revert and cherry-pick button styles generated by commits_helper
apply margin on alert banners only when there is one or more alerts
Rename MattermostNotificationService back to MattermostService
Rename SlackNotificationService back to SlackService
Fix stage and pipeline specs and rubocop offenses
Added QueryRecorder to test N+1 fix on Milestone#show
Use gitlab-workhorse 1.2.1
Make 'unmarked as WIP' message more consistent
Improve specs for Files API
Allow unauthenticated access to Repositories Files API GET endpoints
Add isolated view spec for pipeline stage partial
Move test for HTML stage endpoint to controller specs
Fix sizing of avatar circles; add border
Fix broken test
Fix broken test Changes after review
...
Conflicts:
app/assets/stylesheets/pages/pipelines.scss
app/controllers/projects/pipelines_controller.rb
app/views/projects/pipelines/index.html.haml
spec/features/projects/pipelines/pipelines_spec.rb
- Move the `Oauth2::AccessTokenValidationService` class to
`AccessTokenValidationService`, since it is now being used for
personal access token validation as well.
- Each API endpoint declares the scopes it accepts (if any). Currently,
the top level API module declares the `api` scope, and the `Users` API
module declares the `read_user` scope (for GET requests).
- Move the `find_user_by_private_token` from the API `Helpers` module to
the `APIGuard` module, to avoid littering `Helpers` with more
auth-related methods to support `find_user_by_private_token`
The issue was arising when `#current_user` was called a second time
after a user was impersonated: the `User#is_admin?` check would be
performed on it and it would fail.
Signed-off-by: Rémy Coutable <remy@rymai.me>
* master: (76 commits)
Update "Installation from source" guide for 8.15.0
Group links spec update
Updates the font weight of button styles because of the change to system fonts
Refactor SSH keys docs
Improvements to setting up ssh
Do not reload diff for merge request made from fork when target branch in fork is updated
Add 8.12.10, 8.12.11, and 8.12.12 CHANGELOG.md items
Changes after review
Fix broken test
Adds CHANGELOG entry
Adds tests
Uniformize props name format
Replace commit icon svg logic
Replace play icon svg logic
Updated JS based on review Fixed group links dropdown to match
Update docs to reflect new defaults on omnibus
Merge branch 'jej-23867-use-mr-finder-instead-of-access-check' into 'security'
Merge branch 'html-safe-diff-line-content' into 'security'
Merge branch 'rs-filter-authentication_token' into 'security'
Merge branch 'destroy-session' into 'security'
...
Conflicts:
app/models/ci/pipeline.rb
app/models/commit_status.rb
app/views/projects/ci/pipelines/_pipeline.html.haml
app/views/projects/commit/_pipeline.html.haml
app/views/projects/pipelines/_with_tabs.html.haml
app/views/projects/pipelines/index.html.haml
lib/api/helpers.rb
* add parent_id field to namespaces table to store relation with nested groups
* create routes table to keep information about full path of every group and project
* project/group lookup by full path from routes table
Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
Replace issue access checks with use of IssuableFinder
Split from !2024 to partially solve https://gitlab.com/gitlab-org/gitlab-ce/issues/23867
## Which fixes are in this MR?
⚠️ - Potentially untested
💣 - No test coverage
🚥 - Test coverage of some sort exists (a test failed when error raised)
🚦 - Test coverage of return value (a test failed when nil used)
✅ - Permissions check tested
### Issue lookup with access check
Using `visible_to_user` likely makes these security issues too. See [Code smells](#code-smells).
- [x] 🚦 app/finders/notes_finder.rb:15 [`visible_to_user`]
- [x] 🚥 app/views/layouts/nav/_project.html.haml:73 [`visible_to_user`] [`.count`]
- [x] ✅ app/services/merge_requests/build_service.rb:84 [`issue.try(:confidential?)`]
- [x] ✅ lib/api/issues.rb:112 [`visible_to_user`]
- CHANGELOG: Prevented API returning issues set to 'Only team members' to everyone
- [x] ✅ lib/api/helpers.rb:126 [`can?(current_user, :read_issue, issue)`] Maybe here too?
- [x] ✅ lib/gitlab/search_results.rb:53 [`visible_to_user`]
### Previous discussions
- [ ] https://dev.gitlab.org/gitlab/gitlabhq/merge_requests/2024/diffs#b2ff264eddf9819d7693c14ae213d941494fe2b3_128_126
- [ ] https://dev.gitlab.org/gitlab/gitlabhq/merge_requests/2024/diffs#7b6375270d22f880bdcb085e47b519b426a5c6c7_87_87
See merge request !2031
Defer saving project services to the database if there are no user changes
## What does this MR do?
It defers saving project services to the database as long as it is possible. It creates a project service when creating a project only if this project service has an active template. After that project services are saved on the first edit.
## Are there points in the code the reviewer needs to double check?
- tests that used `build_missing_services` before the change
- number of queries executed
## Why was this MR needed?
Motivation in #22281
## Does this MR meet the acceptance criteria?
- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- Tests
- [x] Added for this feature/bug
- [x] All builds are passing
- [x] Conform by the [merge request performance guides](http://docs.gitlab.com/ce/development/merge_request_performance_guidelines.html)
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if it does - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)
## What are the relevant issue numbers?
Fixes#22281
See merge request !6958
Memoize Github::Shell's secret token
## What does this MR do?
`API::Helpers#secret_token` was reading the secret file on every invocation. This MR reads the file in the `gitlab_shell_secret_token.rb` initializer and saves it as a class variable at `Gitlab::Shell.secret_token`
## Are there points in the code the reviewer needs to double check?
- I'm not sure if the use of `cattr_accessor` is the best approach, or if should be moved into the `class << self` block?
- Should `API::Helpers#secret_token` be removed in favor of using `Gitlab::Shell.secret_token`?
## Why was this MR needed?
Performance optimization.
Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/22510
See merge request !6599
The practical effect of this commit is to make the API check the Rails session
cookie for authentication details. If the cookie is present and valid, it will
be used to authenticate.
The API now has several authentication options for users. They follow in this
order of precedence:
* Authentication token
* Personal access token
* OAuth2 Bearer token (Doorkeeper - application access)
* Rails session cookie
Also, mutualize AccessRequests and Members endpoints for Group &
Project.
New API documentation for the AccessRequests endpoints.
Signed-off-by: Rémy Coutable <remy@rymai.me>
1. Don't use case statements for dispatch anymore. This leads to a lot
of duplication, and makes the logic harder to follow.
2. Remove duplicated logic.
- For example, the `can_push_to_branch?` exists, but we also have a
different way of checking the same condition within `change_access_check`.
- This kind of duplication is removed, and the `can_push_to_branch?`
method is used in both places.
3. Move checks returning true/false to `UserAccess`.
- All public methods in `GitAccess` now return an instance of
`GitAccessStatus`. Previously, some methods would return
true/false as well, which was confusing.
- It makes sense for these kinds of checks to be at the level of a
user, so the `UserAccess` class was repurposed for this. The prior
`UserAccess.allowed?` classmethod is converted into an instance
method.
- All external uses of these checks have been migrated to use the
`UserAccess` class
4. Move the "change_access_check" into a separate class.
- Create the `GitAccess::ChangeAccessCheck` class to run these
checks, which are quite substantial.
- `ChangeAccessCheck` returns an instance of `GitAccessStatus` as
well.
5. Break out the boolean logic in `ChangeAccessCheck` into `if/else`
chains - this seems more readable.
6. I can understand that this might look like overkill for !4892, but I
think this is a good opportunity to clean it up.
- http://martinfowler.com/bliki/OpportunisticRefactoring.html
- Extract a duplicated `redirect_to`
- Fix a typo: "token", not "certificate"
- Have the "Expires at" datepicker be attached to a text field, not inline
- Have both private tokens and personal access tokens verified in a
single "authenticate_from_private_token" method, both in the
application and API. Move relevant logic to
`User#find_by_personal_access_token`
- Remove unnecessary constants relating to API auth. We don't need a
separate constant for personal access tokens since the param is the
same as for private tokens.
API: Star and unstar a project
Add two new endpoints `POST /projects/:id/star` and `POST /projects/:id/unstar` to star and unstar a project.
* Closes#12739
See merge request !3578
Due to broken implementation of attribute_for_keys the runner information was not updated correctly.
This MR adds test to check that such scenario will never happen again.
- Offloads uploading to GitLab Workhorse
- Use /authorize request for fast uploading
- Added backup recipes for artifacts
- Support download acceleration using X-Sendfile
Ruby str_equal uses memcmp internally to compare String.
Memcmp is vunerable to timing attacks because it returns early
on mismatch (on most x32 platforms memcmp uses a bytewise comparision).
Devise.secure_compare implements a constant time comparision instead.
* users (#6878, #3526, #4209): Validation error messages are now exposed through 400 responses, 409 response are sent in case of duplicate email or username
* MRs (#5335): 409 responses are sent in case of duplicate merge request (source/target branches), 422 responses are sent when submiting MR fo/from unrelated forks
* issues
* labels
* projects
Also, is_admin and can_create_group are exposed in the user information.
Fixed attributes_for_keys to process properly keys with boolean values (since false.present? is false).
-Specifying a header of SUDO or adding a :sudo with either user id, or username of the user will set the current_user to be that user if your identifying private_token/PRIVATE_TOKEN is an administrator token
Added a helper method to check if required parameters are given in an API call. Can be used
to return a `400 Bad Request` return code if a required attribute is missing.
Code clean up and fixed tests.
Extracted a method for 400 error (Bad request) and adjusted code accordingly. The name of
the missing attribute is used to show which one was missing from the request. It is used to
give an appropriate message in the json response.
Douwe Maan
Travis Miller
Alejandro Rodríguez
Rémy Coutable
Lin Jen-Shin
Ruben Davila
Grzegorz Bizon
blackst0ne
Grzegorz Bizon
Sean McGivern
Robert Schilling
Zeger-Jan van de Weg
Felipe Artur
vanadium23
Timothy Andrew
Kamil Trzcinski
Toon Claes
Robert Speicher
Bob Van Landuyt
http://jneen.net/
Pawel Chojnacki
Adam Niedzielski
Oswaldo Ferreira
Mark Fletcher
Sean McGivern
Adam Pahlevi
Regis
Markus Koller
Douwe Maan
Dmitriy Zaporozhets
tiagonbotelho
Douglas Barbosa Alexandre
Justin DiPierro
Nick Thomas
Tomasz Maczukin
Stan Hu
Robert Speicher
Gabriel Mazetto
Ahmad Sherif
Paco Guzman
Zeger-Jan van de Weg
Michi302
Josh Frye
Jason Lee
Guilherme Garnier
Valery Sizov
Kirilll Zaitsev
Jakub Jirutka
jubianchi
Vinnie Okada
Jörg Thalheim
Marin Jankovski
Valery Sizov
Ciro Santilli
Sasha Joseph
Jacob Vosmaer
skv
Nihad Abbasov
Angus MacArthur
Boyan Tabakov
Izaak Alpert
Izaak Alpert
Sebastian Ziebell
Dmitriy Zaporozhets
Alex Denisov