Skip to content

Define and add IneligibleStatus fields for access list members and owners#31857

Merged
kimlisa merged 8 commits intomasterfrom
lisa/access-list-ineligible-reason
Sep 21, 2023
Merged

Define and add IneligibleStatus fields for access list members and owners#31857
kimlisa merged 8 commits intomasterfrom
lisa/access-list-ineligible-reason

Conversation

@kimlisa
Copy link
Copy Markdown
Contributor

@kimlisa kimlisa commented Sep 14, 2023

part of https://github.com/gravitational/teleport.e/issues/1899

For members ineligibility status is nonzero when:

  • membership expired
  • user does not exist
  • user does not meet the membership_requires fields

For owners ineligibility status is nonzero when:

  • user does not exist
  • user does not meet the ownership_requires fields

@kimlisa kimlisa requested review from jakule and mdwn and removed request for gabrielcorado and tigrato September 14, 2023 02:19
mdwn
mdwn reviewed Sep 14, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@mdwn mdwn left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I have no issue with the code per se.

I'm not sure how I feel about having this transient field as part of the underlying object -- I feel like this should be handled in the UI endpoint layer. As is this field could get stored in the backend, and it strikes me as a derivable field at run time.

What do you think?

Edit: I'm vacillating on this because, thinking more on it, there is prior art here, isn't there? @jakule, what do you think?

@kimlisa
Copy link
Copy Markdown
Contributor Author

kimlisa commented Sep 18, 2023

friendly ping @mdwn @jakule

jakule
jakule suggested changes Sep 18, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@jakule jakule left a comment

Choose a reason for hiding this comment

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

As I mentioned in the comment, INELIGIBLE_STATUS_UNKNOWN should not be used as eligible status. Apart from that the PR looks good. I missed this the last time.

Comment thread api/proto/teleport/accesslist/v1/accesslist.proto Outdated
Copy link
Copy Markdown
Contributor

@jakule jakule Sep 18, 2023

Choose a reason for hiding this comment

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

Missing comment

Comment thread api/proto/teleport/accesslist/v1/accesslist.proto Outdated
Copy link
Copy Markdown
Contributor

@jakule jakule Sep 18, 2023

Choose a reason for hiding this comment

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

Missing comment

Comment thread api/proto/teleport/accesslist/v1/accesslist.proto Outdated
Copy link
Copy Markdown
Contributor

@jakule jakule Sep 18, 2023

Choose a reason for hiding this comment

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

This doesn't sound correct. UNSPECIFY means unknown. We should not assume that when the status is unknown, the user is eligible. Please add INELIGIBLE_STATUS_ELIGIBLE to indicate that we checked the user and all requirements are met and leave the unknown as the default value for situations where we were not able to determine it (error for example)

@kimlisa kimlisa force-pushed the lisa/access-list-ineligible-reason branch from 8f5de12 to b71c5a8 Compare September 19, 2023 05:14
@kimlisa kimlisa requested review from jakule and mdwn September 19, 2023 05:33
mdwn
mdwn approved these changes Sep 19, 2023
View reviewed changes
Comment thread api/types/accesslist/convert/v1/accesslist.go Outdated
Copy link
Copy Markdown
Contributor

@mdwn mdwn Sep 19, 2023

Choose a reason for hiding this comment

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

nit: godoc.

Comment thread api/types/accesslist/convert/v1/member.go Outdated
Copy link
Copy Markdown
Contributor

@mdwn mdwn Sep 19, 2023

Choose a reason for hiding this comment

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

nit: godoc.

@kimlisa kimlisa force-pushed the lisa/access-list-ineligible-reason branch from 889f79d to a306b77 Compare September 21, 2023 02:12
@kimlisa kimlisa added this pull request to the merge queue Sep 21, 2023
Merged via the queue into master with commit 0ac6cb5 Sep 21, 2023
@kimlisa kimlisa deleted the lisa/access-list-ineligible-reason branch September 21, 2023 02:57
@public-teleport-github-review-bot
Copy link
Copy Markdown

public-teleport-github-review-bot Bot commented Sep 21, 2023

@kimlisa See the table below for backport results.

Branch Result
branch/v13 Failed
branch/v14 Create PR

@kimlisa kimlisa mentioned this pull request Sep 21, 2023
Merged
kimlisa added a commit that referenced this pull request Sep 21, 2023
@kimlisa
Define and add IneligibleStatus fields for access list members and …
7468699 
…owners (#31857)
@kimlisa kimlisa mentioned this pull request Sep 21, 2023
Merged
github-merge-queue Bot pushed a commit that referenced this pull request Sep 21, 2023
@kimlisa
[v13] Define and add IneligibleStatus fields for access list member…
9aa038c 
…s and owners (#31857) (#32279)

* Define and add `IneligibleStatus` fields for access list members and owners (#31857)

* Check for error before running option
@fheinecke fheinecke mentioned this pull request Sep 26, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@mdwn mdwn mdwn approved these changes

@jakule jakule jakule approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

size/sm

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kimlisa @mdwn @jakule