Skip to content

Conversation

battermann
Copy link
Contributor

@battermann battermann commented Mar 14, 2022

https://wearezeta.atlassian.net/browse/SQSERVICES-1375

Checklist

  • The PR Title explains the impact of the change.
  • The PR description provides context as to why the change should occur and what the code contributes to that effect. This could also be a link to a JIRA ticket or a Github issue, if there is one.
  • changelog.d contains the following bits of information (details):
    • A file with the changelog entry in one or more suitable sub-sections. The sub-sections are marked by directories inside changelog.d.

Copy link
Contributor

@fisx fisx left a comment

Choose a reason for hiding this comment

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

I would prefer using the team end-point (GET /i/teams/:tid/features/sndFactorPasswordChallenge). You could easily get the team id from the user id or bot id in the handlers; ie. checkAllowed would be something like :: UserId -> Handler r () and be called first thing inside the handler, not from outside of it.

The only arguable downside I can see is that the block is buried inside the handler logic rather than in being visible in the routing table. But the big advantage is that the block will be effective for all situations in which the feature is enabled.

@battermann battermann force-pushed the SQSERVICES-1375-block-bot-api-if-2-fa-is-enabled branch from f8ff0bc to 9076196 Compare March 15, 2022 08:15
@battermann battermann closed this Mar 17, 2022
@battermann battermann closed this Mar 17, 2022
@battermann battermann reopened this Mar 25, 2022
@battermann battermann force-pushed the SQSERVICES-1375-block-bot-api-if-2-fa-is-enabled branch from 9076196 to 0eb744c Compare March 25, 2022 11:00
@battermann battermann force-pushed the SQSERVICES-1375-block-bot-api-if-2-fa-is-enabled branch 2 times, most recently from 7ea8edc to ea23c3e Compare March 29, 2022 09:31
@battermann battermann force-pushed the SQSERVICES-1375-block-bot-api-if-2-fa-is-enabled branch from ea23c3e to 1fef757 Compare March 29, 2022 09:36
@battermann battermann requested a review from fisx March 29, 2022 09:36
@battermann battermann requested a review from fisx March 29, 2022 11:23
Copy link
Contributor

@fisx fisx 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 don't need to re-approve the upcoming integration test if you and concourse agree that it's good.)

@battermann battermann marked this pull request as ready for review March 29, 2022 13:41
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.

2 participants