Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ New probe for required MFA #4398

Closed
wants to merge 3 commits into from
Closed

Conversation

eddie-knight
Copy link
Contributor

What kind of change does this PR introduce?

New Probe to check whether an organization has MFA enabled. Requires an authorized token. Only supports GitHub in the present state.

What is the current behavior?

What is the new behavior (if this is a feature change)?**

  • Tests for the changes have been added (for bug fixes/features)

Which issue(s) this PR fixes

NONE

Special notes for your reviewer

Does this PR introduce a user-facing change?

For user-facing changes, please add a concise, human-readable release note to
the release-note

(In particular, describe what changes users might need to make in their
application as a result of this pull request.)

Added independent probe to validates that the organization requires MFA for all collaborators.

Signed-off-by: Eddie Knight <[email protected]>

linting

Signed-off-by: Eddie Knight <[email protected]>

updated docs

Signed-off-by: Eddie Knight <[email protected]>
@eddie-knight eddie-knight marked this pull request as ready for review October 31, 2024 19:42
@eddie-knight eddie-knight requested a review from a team as a code owner October 31, 2024 19:42
@eddie-knight eddie-knight requested review from spencerschrock and raghavkaul and removed request for a team October 31, 2024 19:42
@eddie-knight
Copy link
Contributor Author

Note that this follows #4391

@@ -204,6 +204,17 @@ func (client *Client) GetCreatedAt() (time.Time, error) {
return client.repo.CreatedAt.Time, nil
}

func (client *Client) GetMFARequired() (required bool, err error) {
org, _, err := client.repoClient.Organizations.Get(context.Background(), client.repourl.owner)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add e2e tests of this on:

  1. repo owned by a user
  2. repo owned by an organization

@@ -236,6 +236,11 @@ func (client *Client) GetCreatedAt() (time.Time, error) {
return client.project.getCreatedAt()
}

func (c *Client) GetMFARequired() (required bool, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Appears this is available on GitLab for group namespaces as well:
https://docs.gitlab.com/ee/api/groups.html

Comment on lines +18 to +21
motivation: >
What is the motivation for this probe?
implementation: >
How does this probe work under-the-hood?
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add these fields to the documentation?

probes/orgRequiresMFA/def.yml Outdated Show resolved Hide resolved
Co-authored-by: Raghav Kaul <[email protected]>
Signed-off-by: Eddie Knight <[email protected]>
Comment on lines 46 to +47
GetDefaultBranch() (*BranchRef, error)
GetMFARequired() (bool, error)
Copy link
Member

Choose a reason for hiding this comment

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

We may want to reach a decision on #4049

This is technically a breaking change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants