Skip to content

Conversation

@pjlast
Copy link
Contributor

@pjlast pjlast commented Oct 4, 2024

Pull Request approval

Although pull request approval is not enforced for this repository in order to reduce friction, merging without a review will generate a ticket for the docs team to review your changes. So if possible, have your pull request approved before merging.

@vercel
Copy link

vercel bot commented Oct 4, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
sourcegraph-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 18, 2024 8:33am

@pjlast pjlast marked this pull request as ready for review October 8, 2024 12:20
@pjlast pjlast requested review from ggilmore and mmanela October 8, 2024 12:20
{
"authorization": {
"subRepoPermissions": true,
"ignoreRulesWithHost": true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a little confused on exactly what ignoreRulesWithHost does that is different than just having enforceIPRestrictions. Does this rule mean we don't just treat it as a * but we ignore the whole rule?

What is the default behavior if you don't set ignoreRulesWithHost?
Should we call out that these settings are mutually exclusive?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So here I describe the default behaviour: https://github.com/sourcegraph/docs/pull/691/files#diff-265332799615f15ca1718cf47161c2845000523dfdea9385bc4363dddc0dfc0cR163-R170

We can mention that they're mutually exclusive. The site admin config should complain about it as well since it's encoded as mutually exclusive in the schema

}
```

When `enforceIPRestrictions` is set to `true`, Sourcegraph will use the user's IP address to apply Perforce permissions at the user level. It uses the final `X-Forwarded-For` header in the request to identify the user's IP. Note that this header can be easily spoofed, so ensure your load balancer or proxy handles `X-Forwarded-For` headers securely.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have this last part in a !!! alert box saying we require you ensure via your load balancer that the last ip in x-forwarded-for is validated

@MaedahBatool
Copy link
Contributor

Hey @pjlast is this scheduled to go live with Sourcegraph 5.9?

@pjlast
Copy link
Contributor Author

pjlast commented Oct 9, 2024

@MaedahBatool we're not certain when exactly this is going live yet

@MaedahBatool MaedahBatool marked this pull request as draft October 9, 2024 17:21
@MaedahBatool
Copy link
Contributor

@MaedahBatool we're not certain when exactly this is going live yet

Gotcha, I am then converting it to a draft PR till we get a decision to avoid it from getting merged with the main. I am already prepping docs for SG 5.9, so if a decision is made, please ping me. :)

@pjlast pjlast marked this pull request as ready for review October 17, 2024 11:46
@pjlast pjlast requested a review from evict October 17, 2024 11:46
@pjlast
Copy link
Contributor Author

pjlast commented Oct 17, 2024

@MaedahBatool seems like we're going ahead with merging this feature for the 5.9 release!

Copy link
Contributor

@evict evict left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I do agree with @mmanela's point about the double exclam alert, if that's possible that would be a good addition.

@pjlast
Copy link
Contributor Author

pjlast commented Oct 18, 2024

Cool, I changed it to a warning instead of a note

@ggilmore ggilmore requested a review from MaedahBatool October 21, 2024 20:16
@MaedahBatool MaedahBatool changed the base branch from main to sg-next-oct30-2024 October 21, 2024 20:21
@MaedahBatool
Copy link
Contributor

@pjlast is the PR complete? Can I merge with the 5.9 release branch?

@pjlast pjlast merged commit d827e33 into sg-next-oct30-2024 Oct 22, 2024
7 checks passed
@pjlast pjlast deleted the pjlast/perforce-ip-permissions branch October 22, 2024 06:53
@pjlast
Copy link
Contributor Author

pjlast commented Oct 22, 2024

@MaedahBatool yes I've hit merge 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants