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

Add GDS::SSO::PermissionDeniedError to excluded exceptions list #366

Merged
merged 2 commits into from
Apr 25, 2024

Conversation

AlexAvlonitis
Copy link
Contributor

@AlexAvlonitis AlexAvlonitis commented Apr 25, 2024

Related to: alphagov/gds-sso#300

  • We do not need to capture these errors generated from gds-sso. They do not indicate that something went wrong with the application; rather, they indicate that a user does not have permissions to access a resource.

trello card: https://trello.com/c/qb1qpwnr/1369-additional-functionality-to-gds-sso-gem-for-intercepting-401s-and-providing-a-route-constraint

@AlexAvlonitis AlexAvlonitis force-pushed the add-excluded-exception-gds-sso-permission-denied branch from 844bfaf to 805245b Compare April 25, 2024 09:47
Copy link
Member

@kevindew kevindew left a comment

Choose a reason for hiding this comment

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

Change looks good. It would be good if the commit explained why we don't want this send to error to Sentry.

Would you be able to add an entry to the changelog too?

- We do not need to capture these errors generated from gds-sso. They do not indicate that something went wrong with the application; rather, they indicate that a user does not have permissions to access a resource.
@AlexAvlonitis AlexAvlonitis force-pushed the add-excluded-exception-gds-sso-permission-denied branch from 805245b to 1795912 Compare April 25, 2024 12:45
@AlexAvlonitis
Copy link
Contributor Author

Change looks good. It would be good if the commit explained why we don't want this send to error to Sentry.

Would you be able to add an entry to the changelog too?

Thanks for the review @kevindew , I updated the things you suggested, I followed the pattern for the changelog message and version (I noticed it does not have unreleased section), should I update the gem's version as part of this PR?

@kevindew
Copy link
Member

Thanks for the review @kevindew , I updated the things you suggested, I followed the pattern for the changelog message and version (I noticed it does not have unreleased section), should I update the gem's version as part of this PR?

We normally just add an unreleased section each time a new one is added.

It's no biggie though to update the gem version as part of this PR since this is a relatively slow moving gem.

CHANGELOG.md Outdated Show resolved Hide resolved
@AlexAvlonitis AlexAvlonitis force-pushed the add-excluded-exception-gds-sso-permission-denied branch from 1795912 to 95fd7a5 Compare April 25, 2024 13:17
Copy link
Member

@kevindew kevindew left a comment

Choose a reason for hiding this comment

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

Looks good to me. Lets just check with the owning team: Platform Security Reliability team before merging. Would you mind dropping them a message on Slack (#govuk-platform-security-reliability-team)

@AlexAvlonitis AlexAvlonitis merged commit 200c614 into main Apr 25, 2024
12 checks passed
@AlexAvlonitis AlexAvlonitis deleted the add-excluded-exception-gds-sso-permission-denied branch April 25, 2024 13:28
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.

None yet

3 participants