Changes required for supporting metadata pulldown in admin tool

[dhakim87]

VALIDATE ACCESS CHANGED to allow admins to access -any- route accessible to a user.
Admin repo now sends template info alongside survey results.

@AmandaBirmingham please carefully check the changes I made to validate access to make sure you agree with them

[wasade]

Should this trigger some changes in test code?

[wasade]

would it make sense to pull this from the db? (could potentially remark as a TODO)

[dhakim87]

I don't follow... template_id = survey_answers_repo.find_survey_template_id(answer_id) pulls it from the database, I'm just caching the answer in a dictionary because I needed it twice, once to filter out surveys other than the requested one (if one specific one is requested), and once to mark the response object with what survey the set of answers is associated with.

[wasade]

Okay, re-looking at this, answer_id is a survey_id. I suspect we can do something better (as noted in the comments in find_survey_template_id, but let's not tackle that right now.

[wasade]

Not sure I understand the change here. k is the question ID I think, but what's new_k?

[dhakim87]

k is the question ID, new_k is the metadata column header - like DIET_TYPE or ALCOHOL_TYPES. Multiselect column headers like ALCOHOL_TYPES_BeerCider are then generated by the microsetta-admin side

[wasade]

Ah okay, thanks

[dhakim87]

New_k is the metadata column name. I needed the question id as well as the metadata column name in order to convert all possible answers to questions into column headers rather than just the column answers some barcode had responded yes to.

…

Sent from my iPhone

On May 14, 2020, at 8:29 PM, Daniel McDonald ***@***.***> wrote:  @wasade commented on this pull request. Should this trigger some changes in test code? In microsetta_private_api/repo/admin_repo.py: > @@ -419,14 +419,18 @@ def get_survey_metadata(self, sample_barcode, survey_template_id=None): answer_ids = survey_answers_repo.list_answered_surveys_by_sample( account_id, source_id, sample_id) + answer_to_template_map = {} would it make sense to pull this from the db? (could potentially remark as a TODO) In microsetta_private_api/repo/admin_repo.py: > @@ -453,9 +457,13 @@ def get_survey_metadata(self, sample_barcode, survey_template_id=None): survey_answers = {} for k in answer_model: new_k = metadata_map[int(k)] - survey_answers[new_k] = answer_model[k] + survey_answers[k] = [new_k, answer_model[k]] Not sure I understand the change here. k is the question ID I think, but what's new_k? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[wasade]

On new_k, is that being done for non-multiple questions too?

[wasade]

Test failure looks real

[dhakim87]

Yes. I changed the format on the api side to enable matching against the survey template objects. Potentially we could do the lookups on the api side instead. Since we want to be able to pull answers in the non admin section of the api with a different format, I’m not sure what’s cleanest

…

Sent from my iPhone

On May 14, 2020, at 9:00 PM, Daniel McDonald ***@***.***> wrote:  On new_k, is that being done for non-multiple questions too? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[AmandaBirmingham]

I have really only reviewed the validate account access part, as I am not familiar enough with the admin stuff to be useful there. I have a thought about the account validation that I'd like consideration of ...

[AmandaBirmingham]

I would like to suggest a slightly different structure for this functionality extension:

def _validate_account_access(token_info, account_id):
    token_authenticates_admin = False  # default assumption

    with Transaction() as t:
        account_repo = AccountRepo(t)

        # get the account linked to the token
        account = account_repo.find_linked_account(
            token_info['iss'],
            token_info['sub'])

        if account is not None:
            # determine if the account linked with the token is an admin
            token_authenticates_admin = account.account_type == 'admin'
            
            # if token is for an admin but call is NOT asking about their own account
            admin_accessing_others_acct = account.id != account_id and \
                token_authenticates_admin
            if admin_accessing_others_acct:
                # get the account by input account id rather than token
                account = account_repo.get_account(account_id)

        if account is None:
            raise NotFound(ACCT_NOT_FOUND_MSG)
        else:
            if not admin_accessing_others_acct:
                auth_match = account.account_matches_auth(
                    token_info[JWT_EMAIL_CLAIM_KEY],
                    token_info[JWT_ISS_CLAIM_KEY],
                    token_info[JWT_SUB_CLAIM_KEY])
                if auth_match == AuthorizationMatch.NO_MATCH:
                    raise Unauthorized()

        return account

What I like about this modification is that in the most common case (a user accessing their own account), only one db query to load an account is done instead of two. Also, in the case where someone is trying to access an account that ISN'T theirs, we don't even load that account's info from the db unless they have already demonstrated they are an admin. Finally, we don't do the account_matches_auth check in the case where someone is (legally) accessing another's account, because we in that case we have no use for that info.

What do you think?

[dhakim87]

I believe the general scheme you propose can be done, but I don't think this snippet is quite correct. I believe if a non admin user enters a url specifying an account_id they do not have access to, this code will return a copy of the account object that they do have access to (due to the fact that the account pulled based on the token is guaranteed to pass the account_matches_auth), rather than raising Unauthorized() as desired.

If we split apart admin_accessing_others_acct into accessing_others_acct and is_admin, I think that could be addressed without the secondary database hit to pull requested account.

That said, I don't think the cost of loading two accounts is particularly high and I've spent longer staring the version of the logic currently in the PR than the suggested version.

[AmandaBirmingham]

That said, I don't think the cost of loading two accounts is particularly high and I've spent longer staring the version of the logic currently in the PR than the suggested version.
Ok, fair enough :)