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

Bugfix: Ensure self_group name matches email during authentication #78

Merged
merged 10 commits into from
Nov 21, 2023

Conversation

breedenc
Copy link
Contributor

@breedenc breedenc commented Nov 17, 2023

Description

Artemis API keys are authorized via the group with which they are associated. This applies to individual users' API keys as well - these are assigned to a "self group" that contains only the user for whom the group was created. When created, a self group is named after the user's email address. This PR adds logic to the Artemis UI authentication which ensures that the name of the user's self group stays in-sync with the user's email address (in the case of an email address update).

This PR also adds debug-level logs to the repo lambda to facilitate debugging.

Motivation and Context

PR #50 introduced authentication via Email Aliases, in which a user account's email address could be updated automatically according to configured patterns. However, this logic did not update the name of the user's self group to the user's new email address, and some API authentication/authorization logic is dependent on the name of the self group. This PR ensures that when a user authenticates to the Artemis UI, their self group is named correctly.

How Has This Been Tested?

  • Unit testing
  • Tested in a live nonprod environment

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows conforms to the coding standards.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Pic

@breedenc breedenc changed the title Bugfix: Ensure self_group name matches email during authentication #67 Bugfix: Ensure self_group name matches email during authentication Nov 17, 2023
@@ -120,48 +150,83 @@ def test_get_nonexistent_user(self):
"""
MockUser.users = copy.deepcopy(USERS)
email = "[email protected]"
expected_userid = MockUser.users[-1].get("id") + 1
Copy link
Contributor Author

@breedenc breedenc Nov 21, 2023

Choose a reason for hiding this comment

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

Changed this to a derived value which still tests that a new user was created, but does not require the expected ID value to be hard-coded into the unit test and updated in the case that unrelated tests are added.

@breedenc breedenc marked this pull request as ready for review November 21, 2023 01:36
@breedenc breedenc requested a review from a team as a code owner November 21, 2023 01:36
Copy link
Contributor

@g-marconet g-marconet left a comment

Choose a reason for hiding this comment

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

LGTM

from typing import Tuple

from django.db import transaction
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Has black been run on the changed files? (this change may well be because black was run, but I figured I'd ask)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, either black or isort would have made this change, i did not make it manually

@breedenc breedenc merged commit 670f823 into main Nov 21, 2023
1 check passed
@breedenc breedenc deleted the breedenc/self-group-bugfix branch November 21, 2023 18:59
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