Make GitLab pages support access control
What does this MR do?
Adds option to use GitLab access control in project pages #33422 (closed) and implement the necessary changes for pages config file generation.
This depends on the pages MR gitlab-pages!94 (merged).
The changes required for GitLab to automatically configure necessary pages client id, secret etc. are implemented in omnibus-gitlab!2583 (merged).
All feedback is appreciated!
Testing
At the moment this can be tested by creating the authentication id and secret manually and compiling GitLab pages from gitlab-pages branch.
CI
I haven't done any changes to gitlab-ee
so the ee_compat_check
job is failing at the moment.
Are there points in the code the reviewer needs to double check?
Not that I know of.
Why was this MR needed?
Closes #33422 (closed).
Screenshots (if relevant)
Does this MR meet the acceptance criteria?
-
Changelog entry added, if necessary -
Documentation created/updated -
API support added -
Tests added for this feature/bug - Review
-
Has been reviewed by UX -
Has been reviewed by Frontend -
Has been reviewed by Backend -
Has been reviewed by Database
-
-
Conform by the merge request performance guides -
Conform by the style guides -
Squashed related commits together -
Internationalization required/considered -
End-to-end tests pass ( package-and-qa
manual pipeline job)
What are the relevant issue numbers?
Merge request reports
Activity
mentioned in merge request gitlab-pages!94 (merged)
added Community contribution ~1267825 labels
mentioned in merge request omnibus-gitlab!2583 (merged)
added 3493 commits
-
54e2efa4...f032f8db - 3492 commits from branch
gitlab-org:master
- 53fdf503 - Add checkbox for GitLab Pages access control and implement changes for pages config
-
54e2efa4...f032f8db - 3492 commits from branch
marked the checklist item Changelog entry added, if necessary as completed
@rymai would it be possible to get some input on this and the related merge requests?
@nick.thomas Would you be able to review that one? Thanks!
changed milestone to %11.1
assigned to @nick.thomas
Thanks so much for working on this @tuomoa! I'll try to get a first review of all three MRs done this week, starting here.
- Resolved by Tuomo Ala-Vannesluoma
- Resolved by Tuomo Ala-Vannesluoma
- Resolved by Tuomo Ala-Vannesluoma
- Resolved by Tuomo Ala-Vannesluoma
Thanks (kiitos?) so much @tuomoa , I can see you've put huge amounts of work into this, and it's a feature we definitely want in GitLab. I've conducted an initial review "at 20,000 feet", and identified a few things that need attention, mostly to do with the security model.
I think we'll need a little discussion to work it all out, but I'm happy to work with you on this for as long as you're willing!
added 1 commit
- 5e2abe35 - Move conditions to own function to avoid complexity increasing
@nick.thomas I'm out now, but I'm happy to look on that :)
added 1 commit
- a01196ba - Add explanatory text for pages access control setting
- Resolved by Tuomo Ala-Vannesluoma
mentioned in issue #33422 (closed)
@tuomoa could I get you to resolve the merge conflicts before you pass it back for another review? I'd typically rebase against master, but merging master into your branch is also perfectly acceptable :)
@nick.thomas Yeah sure, I'll rebase against the master.
added 1073 commits
-
a01196ba...3cf68362 - 1068 commits from branch
gitlab-org:master
- 0411692b - Add checkbox for GitLab Pages access control and implement changes for pages config
- f601c65c - Add changelog entry
- 4867387d - Move conditions to own function to avoid complexity increasing
- 2cec6d67 - Add explanatory text for pages access control setting
- 839a4050 - Remove private boolean as we only need access control
Toggle commit list-
a01196ba...3cf68362 - 1068 commits from branch
- Resolved by Tuomo Ala-Vannesluoma
@tuomoa the process isn't particularly obvious, but if we want this to make %11.1, we need to complete it by the end of the 7th July. How do you feel about that timing? If we keep the access control mechanism that's in this MR, it might just about be manageable. I know we're still waiting on @ayufan to act as tie-breaker there.
@nick.thomas well I guess it depends. I don't have plenty of time before the end of 7th July, but I can always try. :)
I suggest we keep the on/off functionality for now, as it can be changed later on with the access levels support if necessary. I also think I've done all the necessary things that were pointed out. There are couple of things I haven't done yet: deduplication of
transport
package and the code still uses thesessions
package. But I would like to keep thesessions
package because the API is nicer than thesecurecookie
and if we swapped it tocookiejar
we would need to handle the encryption ourselves.Is there something that you think still needs to be done before this could be merged (if we decide to go with the on/off functionality) ?
- Resolved by Tuomo Ala-Vannesluoma
- Resolved by Tuomo Ala-Vannesluoma
@tuomoa let's not try to rush it; that way lies much suffering and disappointment! 11.2 is soon enough :)
Happy to stick with the
sessions
package in the Pages MR - it sounds like we're using some of its functionality.changed milestone to %11.2
@nick.thomas yeah sounds good to me considering the changes required :)
@nick.thomas @ayufan I've now changed this to be implemented with pages feature toggle & access level option. I also wrote test for the migration. There is still couple of issues:
- Some migration tests are failing, and I think the reason is what is described in gitlab-org/gitlab-ce#42090. (new field in the project factory)
- I'm not sure how to implement the public policy. It seems that when the project is not public and user is anonymous then
prevent_all
is called and that prevents the enabling ofread_pages_content
in the public pages case.
Do you have any ideas how to fix these issues?
I also made it to remove the pages when pages is disabled in the features. And when it is disabled they won't be deployed again.
I added the
access_control
hint to the gitlab config, so if that is not enabled then there is just simple toggle button available in the features. Also if there is no pages at all, then the whole toggle is hidden.Edited by Tuomo Ala-Vannesluomaadded 1108 commits
-
ede64bf2...be2410ab - 1107 commits from branch
gitlab-org:master
- 3247418f - Add access control setting and toggle for GitLab Pages and implement changes for…
-
ede64bf2...be2410ab - 1107 commits from branch
- Resolved by Tuomo Ala-Vannesluoma
- Resolved by Tuomo Ala-Vannesluoma
added 1 commit
- fc0065bb - Add access control setting and toggle for GitLab Pages and implement changes for…
added 1 commit
- 69431ce5 - Add access control setting and toggle for GitLab Pages and implement changes for…
Thanks @tuomoa, and sorry for the delay in getting back to this - I had a short holiday! I'm hopeful I can dedicate the time to give this another review tomorrow.
@nick.thomas no worries, I just pushed this few days ago anyways. :)
- Resolved by Tuomo Ala-Vannesluoma
- Resolved by Tuomo Ala-Vannesluoma
- Resolved by Tuomo Ala-Vannesluoma
- Resolved by Tuomo Ala-Vannesluoma
- Resolved by Tuomo Ala-Vannesluoma
- Resolved by Tuomo Ala-Vannesluoma
- Resolved by Tuomo Ala-Vannesluoma
- Resolved by Tuomo Ala-Vannesluoma
- Resolved by Nick Thomas
- Resolved by Tuomo Ala-Vannesluoma
- Resolved by Tuomo Ala-Vannesluoma
- Resolved by Tuomo Ala-Vannesluoma
- Resolved by Tuomo Ala-Vannesluoma
- Resolved by Tuomo Ala-Vannesluoma
- Resolved by Tuomo Ala-Vannesluoma
Thanks for transitioning this to the
project_features
table @tuomoa !I can see it's thrown up a few more issues than we expected, but it doesn't feel as if they're insurmountable. I've gone through the whole MR one more time, and noted a few other things as well. Just let me know if you've any questions/disagreements, or feel like this is at risk of not making it in time for %11.2 - I'm feeling optimistic!
We also have some (minor-looking) merge conflicts, so if you could rebase / merge latest master, that'd be a big help!
@nick.thomas thanks for all the comments! I'm gonna be busy during the weekend but I'll take a closer look at them next week and hopefully get the issues sorted out and rebase :)
added 1 commit
- 41f40631 - Always deploy pages, use pages being public to tell gitlab pages if auth is…
added 416 commits
-
41f40631...6f240d7b - 414 commits from branch
gitlab-org:master
- 2b41f62b - Add access control setting and toggle for GitLab Pages and implement changes for…
- 3bf97f5d - Always deploy pages, use pages being public to tell gitlab pages if auth is…
-
41f40631...6f240d7b - 414 commits from branch
added 190 commits
-
b567b171...ab08f998 - 188 commits from branch
gitlab-org:master
- 7679b136 - Add access control setting and toggle for GitLab Pages and implement changes for…
- 1a0508c8 - Remove toggle because without pages access control support there is no way to disable pages
-
b567b171...ab08f998 - 188 commits from branch
Great, thanks @tuomoa ! I'll do my best to give this another review and some manual testing by monday at the latest! (was hoping to get to it today, but various other things got in the way :( )
@nick.thomas Awesome! By the way, how should we handle the custom pages domains? As the pages can only set the cookie for it's own domain and its subdomains, the cookie won't work when pages is accessed with the custom domain. Also we can't add the cookie to the custom domain as it would require the OAuth2 app have all the custom domains so that it could redirect back after authorizing the app (the redirect uri parameter).
I think it could only be accomplished if the Pages would have one OAuth app per project, as then it could have all the custom domains in that project's OAuth app. But I'm not a fan of that approach as then it would require the user to authorize every single project when accessed for the first time instead of just authorizing the GitLab Pages once (and it would also increase the amount of OAuth2 apps in the GitLab instance for no reason).
I think having private pages but served in a custom domain scenario is a bit odd, so I think we could just decide not to support it (the custom domain could also just redirect to the GitLab pages domain if that is really wanted)? Therefore should we disable the custom domains support in the configuration if the pages are not public / or should we force pages to be public when there are custom domains configured?
I'm still hoping we could get this into the 11.2! :)
Edited by Tuomo Ala-Vannesluomaadded 1 commit
- c1bccbb9 - Write id and access_control flag to domain config similar to https only
I managed to resolve the custom domain problem, I described the solution to the gitlab-pages!94 (merged). Only new thing in GitLab is that now we add the project ID
access_control
flag to the domain configuration also (similar to thehttps_only
flag).Sorry for going quiet on this @tuomoa, some regressions popped up that took higher priority. New ETA is likely Monday or Tuesday :(
One more(!) use case to consider - for GitLab.com,
gitlab.io
isn't actually served by the Pages daemon at all right now, going to a static site instead. I'm guessing that this might be problematic with the current code, but I've not taken a look at your updates to handle being in the public suffix list, so perhaps it's solved.changed milestone to %11.3
@nick.thomas no worries. That actually is going to be a problem with the current code since at the moment the pages root domain is used for the authentication callback from the rails. I guess we could use some reserved namespace domain for that purpose (like
projects.gitlab.io
), and use that for the redirect url instead of pages domain.I modified the pages code to use the redirect uri for authentication flow and then updated the examples to use the
projects
prefix. This way we don't need to have the root pages domain to be pointed at the GitLab pages daemon.Edited by Tuomo Ala-VannesluomaI'm OK with
projects.gitlab.io
, sinceprojects
is a reserved group name - there's no chance of conflicts. /cc @ayufan to think about it also!added 1 commit
- f5298382 - Pages are public if project is public and pages are enabled for everyone with access
added 1 commit
- 8b63204b - For public projects there is no need to have Everyone and Everyone with access options
All right. I did a small change that pages are public if project is public and level is
Everyone with access
and also hid theEveryone
options if the projects is public, because it is the same asEveryone with access
. Now theaccess_control
won't be true, if project is public and pages level isEveryone with access
.added 1 commit
- 7ec876e8 - For public projects there is no need to have Everyone and Everyone with access options
This is a great feature and I am looking forward to use it.
Would it possible to add an "always-public" folder?Currently I am using GitLab pages to store auto-created badges of ci-jobs.
It would be great to have them public always, while not sharing the auto-generated doxygen documentation with the whole world.@j.goebbert I think that is a different issue/feature and should not be implemented in the scope of this MR (or the access control issue). I would suggest creating an issue to the GitLab-CE project and then it can be implemented later on if it's worth doing so.
mentioned in issue #50603 (closed)
Thanks, I created a new issue and added a more detailed explanation there: https://gitlab.com/gitlab-org/gitlab-ce/issues/50603
added 1340 commits
-
82a6ed55...0689900c - 1339 commits from branch
gitlab-org:master
- 027b721d - Merge branch 'master' into auth
-
82a6ed55...0689900c - 1339 commits from branch
changed milestone to %11.4
added 488 commits
-
027b721d...c56f2b96 - 487 commits from branch
gitlab-org:master
- 9a10518c - Merge branch 'master' into auth
-
027b721d...c56f2b96 - 487 commits from branch
- Resolved by Nick Thomas
mentioned in issue #30548 (closed)
added 1071 commits
-
9a10518c...ae014e18 - 1070 commits from branch
gitlab-org:master
- 2693b209 - Merge branch 'master' into auth
-
9a10518c...ae014e18 - 1070 commits from branch
added 1 commit
- 086faa1f - Ensure pages access control is always false if pages itself is disabled
mentioned in commit gitlab-pages@86ad3207
marked the checklist item Conform by the merge request performance guides as completed
marked the checklist item Conform by the style guides as completed
marked the checklist item Squashed related commits together as completed
I've just noticed that there's no admin documentation. I'm not too worried about this, since it doesn't look like the Omnibus support will make it for 11.4. I'll create a follow-up MR with documentation. We can consider whether it's worth merging in time if omnibus doesn't make it. If it does, then the admin docs are mostly unneeded anyway.
enabled an automatic merge when the pipeline for 76a1b39d succeeds
mentioned in commit 88fa9a3c
mentioned in issue gitlab-development-kit#403
mentioned in issue gitlab-org/release/tasks#473 (closed)
mentioned in issue gitlab-com/www-gitlab-com#3003 (closed)
mentioned in issue #49692 (moved)
mentioned in merge request !22146 (merged)
mentioned in issue #54142 (closed)
mentioned in merge request !23146 (merged)
mentioned in issue gitlab-pages#161 (closed)
Ah, sorry, I found the issue https://gitlab.com/gitlab-com/gl-infra/infrastructure/issues/5576.
mentioned in issue #59286 (closed)
mentioned in merge request !29881 (closed)
mentioned in merge request !25175 (closed)