We need to do two things to support this:
1. Simplify the regex capture in the routing for the CommitsController
to not exclude the '.atom' suffix. That's a perfectly valid git
branch name, so we shouldn't blow up if we get it.
2. Because Rails now can't automatically detect the request format, add
some code to do so in `ExtractPath` when there is no path. This means
that, given branches 'foo' and 'foo.atom', the Atom feed for the
former is unroutable. To fix this: don't do that! Give the branches
different names!
Remove NamespacesController
* removes unnecessary NamespacesController. The main purpose of this controller was redirect to group or user page when URL like https://gitlab.com/gitlab-org was used. Now this functionality is handled by constrainers (like this https://gitlab.com/gitlab-org/gitlab-ce/blob/master/config/routes/user.rb#L17-21) and take user to correct controller right from the start.
* serve non existing API routes like `/api/v3/whatever` with Grape instead of Rails. Before this change wrong API url was served by rails with not obvious 404, 405 & 500 errors
See merge request !6733
The main purpose of this controller was redirect to group or user page
when URL like https://gitlab.com/gitlab-org was used. Now this
functionality is handled by contrainers and take user to correct
controller right from the start
Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
Copy logic from `Devise::Models::Lockable#valid_for_authentication?`, as
our custom login flow with two pages doesn't call this method. This will
increment the failed login counter, and lock the user's account once
they exceed the number of failed attempts.
Also ensure that users who are locked can't continue to submit 2FA
codes.
Resolve "`Member.add_user`doesn't detect existing members that have requested access"
## What does this MR do?
This merge request handle the case when an access requester is added to a group or project (via the members page or the API).
In `Member.add_user`, if an access requester already exists, we simply accept their request (and set the `created_by`, `access_level` and `expires_at` attributes if given).
## Are there points in the code the reviewer needs to double check?
I've taken the opportunity to cleanup the whole `{Group,Project}Member.add_user*` methods since it was quite a mess.
## What are the relevant issue numbers?
Closes#21983
See merge request !6393
Changes include:
- Ensure Member.add_user is not called directly when not necessary
- New GroupMember.add_users_to_group to have the same abstraction level as for Project
- Refactor Member.add_user to take a source instead of an array of members
- Fix Rubocop offenses
- Always use Project#add_user instead of project.team.add_user
- Factorize users addition as members in Member.add_users_to_source
- Make access_level a keyword argument in GroupMember.add_users_to_group and ProjectMember.add_users_to_projects
- Destroy any requester before adding them as a member
- Improve the way we handle access requesters in Member.add_user
Instead of removing the requester and creating a new member,
we now simply accepts their access request. This way, they will
receive a "access request granted" email.
- Fix error that was previously silently ignored
- Stop raising when access level is invalid in Member, let Rails validation do their work
Signed-off-by: Rémy Coutable <remy@rymai.me>
Projects::ForkService delegates to this service almost entirely, but needed
one small change so it would propagate create errors correctly.
CreateService#execute needs significant refactoring; it is now right at the
complexity limit set by Rubocop. I avoided doing so in this commit to keep the
diff as small as possible.
Several tests depend on the insecure behaviour of ForkService, so fi them up at
the same time.
And Snippets get awards
## What does this MR do?
Makes snippets more awesome, by making them awardables
## Why was this MR needed?
Because Snippets were left behind.
## What are the relevant issue numbers?
Closes#17878
See merge request !4456
* Created a force=true param that will continue with the previous
behaviour of the unsubscribe method
* Created a filter for not-logged users so they see a unsubsribe
confirmation page
* Added the List-Unsubscribe header on emails so the email client can
display it on top
Pass the remember_me option into the u2f form and support it while authenticating
## What does this MR do?
Adds remember me support in the u2f authentication, and makes sure the flag gets passed from the login form to the u2f form.
Based on the changes for the same thing done for regular 2fa: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4369
## Why was this MR needed?
The remember me option wasn't working for u2f devices (yubikey)
## What are the relevant issue numbers?
Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/18103
See merge request !5918
Refactor ability.rb into Policies
## What does this MR do?
Factors out `ability.rb` into a new abstraction - the "policy" (stored in `app/policies`). A policy is a class named `#{class_name}Policy` (looked up automatically as needed) that implements `rules` as follows:
``` ruby
class ThingPolicy < BasePolicy
def rules
@user # this is a user to determine abilities for, optionally nil in the anonymous case
@subject # this is the subject of the ability, guaranteed to be an instance of `Thing`
can! :some_ability # grant the :some_ability permission
cannot! :some_ability # ensure that :some_ability is not allowed. this overrides any `can!` that is called before or after
delegate! @subject.other_thing # merge the abilities (can!) and prohibitions (cannot!) from `@subject.other_thing`
can? :some_ability # test whether, so far, :some_ability is allowed
end
def anonymous_rules
# optional. if not implemented `rules` is called where `@user` is nil. otherwise this method is called when `@user` is nil.
end
end
```
See merge request !5796
Add test for closed MR without fork
Add view test visibility of Reopen and Close buttons
Fix controller tests and validation method
Fix missing space
Remove unused variables from test
closed_without_fork? method refactoring
Add information about missing fork
When closed MR without fork can't edit target branch
Tests for closed MR edit view
Fix indentation and rebase, refactoring
a70431f874 modified the behavior to link to the external issue tracker
issues URL instead of the project path URL. This restores the
previous behavior.
Closes#21252, #21402
Use just SQL to check is a user can admin_issue on a project
Using offset pagination instead pages to avoid a count query
Tradeoff
- we duplicate how we check admin_issue in a SQL relation in the Ability class
- Removed unnecessary column from `SpamLog`
- Moved creation of SpamLogs out of its own service and into SpamCheckService
- Simplified code in SpamCheckService.
- Moved move spam related code into Spammable concern
- New concern `AkismetSubmittable` to allow issues and other `Spammable` models to be submitted to Akismet.
- New model `UserAgentDetail` to store information needed for Akismet.
- Services needed for their creation and tests.
There is a race condition in DestroyGroupService now that projects are deleted asynchronously:
1. User attempts to delete group
2. DestroyGroupService iterates through all projects and schedules a Sidekiq job to delete each Project
3. DestroyGroupService destroys the Group, leaving all its projects without a namespace
4. Projects::DestroyService runs later but the can?(current_user,
:remove_project) is `false` because the user no longer has permission to
destroy projects with no namespace.
5. This leaves the project in pending_delete state with no namespace/group.
Projects without a namespace or group also adds another problem: it's not possible to destroy the container
registry tags, since container_registry_path_with_namespace is the wrong value.
The fix is to destroy the group asynchronously and to run execute directly on Projects::DestroyService.
Closes#17893
Prior, in order to display the correct link to "Issues" in the project
navigation, we were performing a check against the project to see if it
used an external issue tracker, and if so, we used that URL. This was
inefficient.
Now, we simply _always_ link to `namespace_project_issues_path`, and
then in the controller we redirect to the external tracker if it's
present.
This also removes the need for the url_for_issue helper. Bonus! 🎉
Submit new issues created via the WebUI or API to Akismet for spam check on public projects.
## What does this MR do?
Submit new issues created via the WebUI by non project members to Akismet for spam check.
## Why was this MR needed?
Support for Akismet was added only to the API with !2266. This MR builds on that functionality to also check issues submitted via the WebUI for spam.
## What are the relevant issue numbers?
Related to:
- #5573
- #5932
- gitlab-com/infrastructure#14
- gitlab-com/support#61
- !2266
cc @stanhu @MrChrisW
See merge request !5333
Pass an array of user IDs in the `skip_users` param to have them
excluded from the results (unless they are explicitly included through
the `current_user` or `author_id` params).
A release's tag reference is just the name of the tag, not the entire
tag object.
This also fixes the tags index if a tag's message contains non-UTF8 byte
sequences.
Add support for inline videos in issue, MR and notes (on issue, commit, MR, and MR diff)
## What does this MR do?
It adds support for inline videos in issue, MR and notes (on issue, commit, MR, and MR diff). Most of the work was done by @hayesr in !3508 but a few improvements were still missing.
## Why was this MR needed?
To be able to play uploaded videos in GitLab!
## What are the relevant issue numbers?
Closes#4142.
## Screenshots
### Video players
-----
-----
## Does this MR meet the acceptance criteria?
- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- [x] [Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)
- Tests
- [x] Test `VideoLinkFilter`
- [x] Test in `spec/features/markdown_spec.rb`
- [x] Improve `spec/uploaders/file_uploader_spec.rb`
- [x] All builds are passing
- [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 you do - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)
See merge request !5215
New diff notes
Fixes#12732, #14731, #19375, #14783
Builds on https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4110
To do:
- [x] Get it mostly working
- [x] Validate position validity
- [x] Fix: Don’t link to `#`
- [x] Fix: Base ref can be `nil`, potentially, when the MR has an oprhan source branch => Yep, doesn’t work. We need to store a `start_id`
- [x] Optimize: Fewer duplicate `git diff` compares
- [x] Optimize: Pass paths to `PositionTracer#diff` for faster diffs
- [x] Refactor: Use `head_id` in `MergeRequest`/`MergeRequestDiff` instead of `source_sha`
- [x] Refactor: Convert existing array-based diff refs to the DiffRefs model
- [x] Tweak: Use `note_type` in `Autosave` key
- [x] Tweak: Remove `line_code: note.line_code` from `link_to_reply_discussion`
- [x] Update: `SentNotifications` and reply-by-email receiver
- [x] Update: MR diff notification email
- [x] Update: API (MR, Commit note creation and entity)
- [x] Update: GitHub importer
- [x] Address any other TODO comments
- [x] Fix: Suppress "edited 4 minutes ago"
- [x] Write tests
- [x] `LineMapper`
- [x] `PositionTracer`
- [x] `Position`
- [x] `DiffPositionUpdateService`
- [x] `DiffNote`
- [x] `MergeRequests::RefreshService` / `MergeRequest#update_diff_notes_positions`
- [x] Make sure commits with diff notes don't get cleaned up, since this would prevent the diff notes from being rendered (https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5062)
Future improvements:
- Display unresolved comments on files outside the diff, if the comment was added when that file _was_ part of the diff
- Allow commenting on sections between hunks, when expanding the diff using `...`
- (We'd need to generate line code based on Position if we have it, even if it falls outside bounds of diff)
- `diff_hunk` on diff note API entity
- Show diff hunk in notification email
- Resolved line notes would have a boolean, and be inactive through `notes.any? { !active? || resolved? }`
- Multi line notes would store a number of positions, and do the right thing (™) in grouping and then rendering if the first item is multiline? => true
- Image diff notes could store x,y,width,height instead of old_line,new_line for similar grouping. Does it need a reference to say if it's on old or new? These can't have line_codes, clearly. Rendering would be interesting.
- Show commit line comments in the MR diff
- Comment on specific selected words
- Comment on file header
- Unfold top of discussion diff note
- New diff notes API for commits and MRs
/cc @rspeicher
See merge request !4101
- Look for a `change` in its size rather than asserting against an
actual size.
- This previously failed because another spec had an email in
`ActionMailer::Base.deliveries`, which failed this `be_nil` assertion.
Exclude requesters from Project#members, Group#members and User#members
## What does this MR do?
It excludes requesters from the `Project#members`, `Group#members` and `User#members` associations, and adds new `Project#requesters` and `Group#requesters` associations.
## Are there points in the code the reviewer needs to double check?
No.
## Why was this MR needed?
Without this, if you call `project.members`, requesters are included in the results! This is at best misleading, and at worst can lead to security issues. By excluding requesters from the `#members` associations, we avoid introducing security inadvertently since you have to call the `#requesters` association explicitly to get requesters.
## What are the relevant issue numbers?
This is something I realized while fixing the security issue #19102.
## Does this MR meet the acceptance criteria?
- [x] I don't think this needs a CHANGELOG since this is an internal change
- Tests
- [x] Added for this feature/bug
- [ ] All builds are passing
- [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 you do - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)
See merge request !4946
This stands as an alternative to using OAuth to access a user's Github
repositories. This is setup in such a way that it can be used without OAuth
configuration.
From a UI perspective, the how to import modal has been replaced by a full
page, which includes a form for posting a personal access token back to the
Import::GithubController.
If the user has logged in via GitHub, skip the Personal Access Token and go
directly to Github for an access token via OAuth.
The issue was that `MergeRequest#mergeable?` returns false when the CI
state is not success and project.only_allow_merge_if_build_succeeds is
true. In this case `Projects::MergeRequestsController#merge` would
return the `:failed` status when enabling `merge_when_build_succeeds`,
thus leading to a weird state and the MR never automatically merged.
The fix is to disable the CI state check in the controller safeguard
that early return the `:failed` status.
Signed-off-by: Rémy Coutable <remy@rymai.me>
Resolve "Destroying a project causes post_decline_request to be executed"
## What does this MR do?
Ensure we don't send "access request declined" to access requesters when a project is deleted.
## Are there points in the code the reviewer needs to double check?
I've created a service to decouple the notification sending from the AR model.
## Why was this MR needed?
Because there was an issue.
## What are the relevant issue numbers?
Fixes#18755, #18750.
## Does this MR meet the acceptance criteria?
- [x] No CHANGELOG needed.
- [x] Tests
- [x] Added for this feature/bug
- [x] All builds are passing
- [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 you do - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)
See merge request !4744
Implement custom notification level options
part of #12758
See merge request !4389
Disable the unlink feature for SAML connected accounts (social login).
This disables the ability to manually unlink your SAML account, if you have one connected. In certain scenarios, the only allowed login mechanism can be SAML, and if you unlink your account you will be locked out of the system (configuration dependent).
Fixes#18613
See merge request !4662
+ Move 'Edit Project/Group' out of membership-related partial
+ Show the access request buttons only to logged-in users
+ Put the request access buttons out of in a more visible button
+ Improve the copy in the #remove_member_message helper
Signed-off-by: Rémy Coutable <remy@rymai.me>
Here was the problem:
1. When determining whether a given blob is viewable text, gitlab_git reads the first 1024 bytes and checks with Linguist whether it is a text or binary file.
2. If the blob is text, GitLab will attempt to display it.
3. However, if the text has binary characters after the first 1024 bytes, then GitLab will attempt to load the entire contents, but the encoding will be ASCII-8BIT since there are binary characters.
4. The Error 500 results when GitLab attempts to display a mix UTF-8 and ASCII-8BIT.
To fix this, we load as much data as we are willing to display so that the detection will work properly. Requires
an update to gitlab_git: gitlab-org/gitlab_git!86
Closes#13826
Notes are awardables
## What does this MR do?
Makes sure we can :thunder_cloud_rain: comments/notes.
## What are the relevant issue numbers?
Follows up upon !2901, depends on !3785Closes#3655
## Screenshots (if relevant)
TODO
See merge request !4291
- Move the `TwoFactorAuthsController`'s `new` action to `show`, since
the page is not used to create a single "two factor auth" anymore. We
can have a single 2FA authenticator app, along with any number of U2F
devices, in any combination, so the page will be accessed after the
first "two factor auth" is created.
- Add the `u2f` javascript library, which provides an API to the
browser's U2F implementation.
- Add tests for the JS components
Resolve "SHA parameter in accept merge request API"
Add a `sha` parameter to the MR merge API, which must match the source SHA for the branch to be merged.
Also add the same parameter to the UI:
@DouweM and I discussed adding some smart feature to that, like updating the source SHA on navigating to the diff tab, but for now it will just require a refresh 😃Closes#14139.
See merge request !4414
Upgrade devise, devise-two-factor, and attr_encrypted
Devise 4 includes support for Rails 5, working towards #14286. devise-async doesn't support Devise 4.0 and in 4.1 the bug that was blocking using Devise's built-in ActiveJob integration was fixed. So devise-async is removed. devise-two-factor 3.0.0 is required for Devise 4 support.
attr_encrypted and encryptor are optional but recommended upgrades for devise-two-factor 3.0.0. The mode and algorithm will need to be changed in order to update to attr_encrypted 4.x in the future.
See merge request !4216
Set a `sha` parameter on the MR form. If this doesn't match the HEAD of
the source branch when the form is submitted, show a warning (like with
a merge conflict) and don't merge the branch.
Pass the "Remember me" value to the 2FA token form
Prior, if a user had 2FA enabled and checked the "Remember me" field,
the setting was ignored because the OTP input was on a new form and the
value was never passed.
Closes#18000
See merge request !4369
Prior, if a user had 2FA enabled and checked the "Remember me" field,
the setting was ignored because the OTP input was on a new form and the
value was never passed.
Closes#18000
- Use `TokenAuthenticatable` to generate the personal access token
- Remove a check for `authenticity_token` in application controller;
this should've been `authentication_token`, maybe, and doesn't make
any sense now.
- Have the datepicker appear inline
- Rename the `authenticate_user_from_token!` filter to
`authenticate_user_from_private_token!`
- Add a new `authenticate_user_from_personal_access_token!` filter
- Add tests for both.
These changes were pulled from GitLab EE to support configuring
an alternative API URL than the default https://api.github.com.
In addition, the `verify_ssl` flag allows users to disable SSL cert
checking.
One modification: add a default `args` option if it does not exist
to avoid breaking existing configurations.
Always read diff_view setting from the cookie
Prior, when the user had their view set to "parallel" and then visited a
merge request's changes tab _without_ passing the `view` parameter via
query string, the view would be parallel but the `Notes` class was
always instantiated with the default value from `diff_view` ("inline"),
resulting in broken markup when the form to add a line note was
dynamically inserted.
The cookie is set whenever the view is changed, so this value should
always be up-to-date.
Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/14557 and https://gitlab.com/gitlab-org/gitlab-ce/issues/15285
See merge request !3732
Sean McGivern
Linus G Thiel
Dmitriy Zaporozhets
James Edwards-Jones
Semyon Pupkov
Phil Hughes
Douglas Barbosa Alexandre
James
Rémy Coutable
barthc
Thomas Balthazar
Robert Speicher
James Lopez
Connor Shea
Nick Thomas
Douwe Maan
Maximiliano Perez Coto
Z.J. van de Weg
Katarzyna Kobierska
Airat Shigapov
DJ Mountney
Felipe Artur
Stan Hu
http://jneen.net/
Jacob Schatz
Paco Guzman
Douwe Maan
Patricio Cano
Kamil Trzcinski
tiagonbotelho
Robert Speicher
Alejandro Rodríguez
Achilleas Pipinellis
Alfredo Sumaran
Timothy Andrew
Grzegorz Bizon
Eric K Idema
Ruben Davila
Jacob Vosmaer (GitLab)
Fatih Acet
Long Nguyen