story/VOGRE-37 #71
story/VOGRE-37 #71
Conversation
|
Can one of the admins verify this patch? |
concepts/lifecycle.py
Outdated
| concept_type, | ||
| equal_to=equal_uri) | ||
| except ConceptPowerCredentialsMissingException as e: | ||
| raise e |
There was a problem hiding this comment.
since this exception is already thrown I don't think you need to rethrow it?
external_accounts/views.py
Outdated
| account.save() | ||
| messages.success(request, "Password updated successfully!") | ||
| except ConceptpowerAccount.DoesNotExist: | ||
| messages.error(request, "No ConceptPower account found.") |
There was a problem hiding this comment.
conceptpower is spelled "Conceptpower"
| ConceptpowerAccount.objects.filter(user=request.user).delete() | ||
| except Exception as e: | ||
| print(f"Error disconnecting Conceptpower account: {e}") | ||
|
|
There was a problem hiding this comment.
the user should see some message saying that either the account was disconnected successfully or that it didn't work.
concepts/conceptpower.py
Outdated
| from external_accounts.models import ConceptpowerAccount | ||
|
|
||
|
|
||
| class ConceptPowerCredentialsMissingException(Exception): |
There was a problem hiding this comment.
Conceptpower is misspelled (lowercase 'p')
| return concept_entries[0] if concept_entries else {} | ||
|
|
||
| def create(self, user, password, label, pos, conceptlist, description, | ||
| def create(self, user, label, pos, conceptlist, description, |
There was a problem hiding this comment.
just as important as throwing an exception when conceptpower credentials are missing is to throw one when the authentication is incorrect. right now it looks like it would just show a generic 'something went wrong' message to the user.
|
Resolve conflicts please |
external_accounts/models.py
Outdated
| """Get or create a Fernet instance with a key""" | ||
| if not self._encryption_key: | ||
| self._encryption_key = Fernet.generate_key().decode() | ||
| self.save(update_fields=['_encryption_key']) |
There was a problem hiding this comment.
does this mean that the key to decrypt a password is stored in the db along with the password? that does not seem like the best approach. if someone gets access to the database, then they have all they need. might as well not encrypt the password at all. a better approach is probably to store the key as a setting. at least it's separate from the database.
3 participants
Guidelines for Pull Requests
If you haven't yet read our code review guidelines, please do so, You can find them here.
Please confirm the following by adding an x for each item (turn
[ ]into[x]).Please provide a brief description of your ticket
Users should set up their own connection with Conceptpower by entering their username and password and during concept resolution, their own account would be used.
https://diging.atlassian.net/browse/VOGRE-37
Are there any other pull requests that this one depends on?
This PR depends on #36
Anything else the reviewer needs to know?
... describe here ...