Skip to content
Snippets Groups Projects

Make GitLab pages support access control

Merged Tuomo Ala-Vannesluoma requested to merge tuomoa/gitlab-foss:auth into master
All threads resolved!

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)

Screenshot_from_2018-09-11_14-57-02

Does this MR meet the acceptance criteria?

What are the relevant issue numbers?

#33422 (closed)

Edited by Nick Thomas

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Nick Thomas
  • 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!

  • You're welcome and that is correct :) Thanks for all the comments! I was expecting that there will be some points that needs to discussed so no surprises there. I'll take a closer look at the comments next week. I am willing to work on this until it is ready for merging!

  • added 1 commit

    • 5e2abe35 - Move conditions to own function to avoid complexity increasing

    Compare with previous version

  • @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

    Compare with previous version

  • 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

    Compare with previous version

  • @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 the sessions package. But I would like to keep the sessions package because the API is nicer than the securecookie and if we swapped it to cookiejar 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) ?

  • @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.

  • Nick Thomas changed milestone to %11.2

    changed milestone to %11.2

  • @nick.thomas yeah sounds good to me considering the changes required :)

  • Tuomo Ala-Vannesluoma changed the description

    changed the description

  • @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 of read_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-Vannesluoma
  • Tuomo Ala-Vannesluoma resolved all discussions

    resolved all discussions

  • added 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…

    Compare with previous version

  • added 1 commit

    • fc0065bb - Add access control setting and toggle for GitLab Pages and implement changes for…

    Compare with previous version

  • added 1 commit

    • 69431ce5 - Add access control setting and toggle for GitLab Pages and implement changes for…

    Compare with previous version

  • 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. :)

  • 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…

    Compare with previous version

  • 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…

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • added 1 commit

    • b708ac48 - Fix updating always when config changes

    Compare with previous version

  • added 1 commit

    • 987e6968 - Try to fix migration spec problem

    Compare with previous version

  • added 1 commit

    • 74985086 - Try to fix migration spec problem

    Compare with previous version

  • added 1 commit

    • 4646e507 - Fix static analysis error and one spec

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • 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

    Compare with previous version

  • 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-Vannesluoma
  • added 1 commit

    • c1bccbb9 - Write id and access_control flag to domain config similar to https only

    Compare with previous version

  • 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 the https_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.

  • Nick Thomas changed milestone to %11.3

    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-Vannesluoma
  • Tuomo Ala-Vannesluoma resolved all discussions

    resolved all discussions

  • I'm OK with projects.gitlab.io, since projects is a reserved group name - there's no chance of conflicts. /cc @ayufan to think about it also!

  • OK, I think this side is ready, so I've moved my attention back to the Pages MR. Once they're both ready, we should merge them together.

    Ideally, we'd get the Omnibus MR in for the same release as well.

  • added 1 commit

    • f5298382 - Pages are public if project is public and pages are enabled for everyone with access

    Compare with previous version

  • added 1 commit

    • 8b63204b - For public projects there is no need to have Everyone and Everyone with access options

    Compare with previous version

  • All right. I did a small change that pages are public if project is public and level is Everyone with access and also hid the Everyone options if the projects is public, because it is the same as Everyone with access. Now the access_control won't be true, if project is public and pages level is Everyone with access.

  • added 1 commit

    • 7ec876e8 - For public projects there is no need to have Everyone and Everyone with access options

    Compare with previous version

  • added 1 commit

    • c35800d9 - If auth is not enabled pages are public

    Compare with previous version

  • added 1 commit

    • 82a6ed55 - Set pages access control enabled in tests

    Compare with previous version

  • 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

  • Nick Thomas added 1340 commits

    added 1340 commits

    Compare with previous version

  • Nick Thomas changed milestone to %11.4

    changed milestone to %11.4

  • Nick Thomas added 488 commits

    added 488 commits

    Compare with previous version

  • Nick Thomas
  • Nick Thomas resolved all discussions

    resolved all discussions

  • Nick Thomas changed the description

    changed the description

  • mentioned in issue #30548 (closed)

  • Nick Thomas added 1071 commits

    added 1071 commits

    Compare with previous version

  • Nick Thomas added 1 commit

    added 1 commit

    Compare with previous version

  • Nick Thomas added 1 commit

    added 1 commit

    • 086faa1f - Ensure pages access control is always false if pages itself is disabled

    Compare with previous version

  • mentioned in commit gitlab-pages@86ad3207

  • Nick Thomas added 1 commit

    added 1 commit

    Compare with previous version

  • Nick Thomas added 1 commit

    added 1 commit

    Compare with previous version

  • One static analysis failure that I just pushed a fix for. The ee-compat-check is bogus.

  • Nick Thomas marked the checklist item Has been reviewed by UX as completed

    marked the checklist item Has been reviewed by UX as completed

  • Nick Thomas marked the checklist item Has been reviewed by Frontend as completed

    marked the checklist item Has been reviewed by Frontend as completed

  • Nick Thomas marked the checklist item Has been reviewed by Backend as completed

    marked the checklist item Has been reviewed by Backend as completed

  • Nick Thomas marked the checklist item Has been reviewed by Database as completed

    marked the checklist item Has been reviewed by Database as completed

  • Nick Thomas marked the checklist item Tests added for this feature/bug as completed

    marked the checklist item Tests added for this feature/bug as completed

  • Nick Thomas marked the checklist item API support added as completed

    marked the checklist item API support added as completed

  • Nick Thomas marked the checklist item Conform by the merge request performance guides as completed

    marked the checklist item Conform by the merge request performance guides as completed

  • Nick Thomas marked the checklist item Conform by the style guides as completed

    marked the checklist item Conform by the style guides as completed

  • Nick Thomas marked the checklist item Squashed related commits together as completed

    marked the checklist item Squashed related commits together as completed

  • Nick Thomas marked the checklist item Internationalization required/considered as completed

    marked the checklist item Internationalization required/considered as completed

  • Nick Thomas marked the checklist item End-to-end tests pass (package-and-qa manual pipeline job) as completed

    marked the checklist item End-to-end tests pass (package-and-qa manual pipeline job) 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.

  • Nick Thomas approved this merge request

    approved this merge request

  • Nick Thomas enabled an automatic merge when the pipeline for 76a1b39d succeeds

    enabled an automatic merge when the pipeline for 76a1b39d succeeds

  • Nick Thomas canceled the automatic merge

    canceled the automatic merge

  • merged

  • Nick Thomas mentioned in commit 88fa9a3c

    mentioned in commit 88fa9a3c

  • mentioned in issue #49692 (moved)

  • mentioned in merge request !22146 (merged)

  • mentioned in issue #54142 (closed)

  • mentioned in merge request !23146 (merged)

  • Hey guys, can you please help me with a confusion? This MR was merged for 11.4 and all its dependencies were merged too, but there's still no permissions for static pages. Is there a follow up for this one? Also, how critical is failing of ee_compat_check on this one?

  • Are you using it on prem? If so you need to enable it in config and then at the project settings level.

    IIRC it's not yet been rolled out to gitlab.com.

  • mentioned in issue #59286 (closed)

  • Vladimir Shushlin mentioned in merge request !29881 (closed)

    mentioned in merge request !29881 (closed)

  • Bob Van Landuyt mentioned in merge request !25175 (closed)

    mentioned in merge request !25175 (closed)

  • Please register or sign in to reply
    Loading