bcgov / entity

ServiceBC Registry Team working on Legal Entities
Apache License 2.0
23 stars 57 forks source link

Create UI - cannot use TING affiliated to login but not account #19698

Closed severinbeauvais closed 3 months ago

severinbeauvais commented 3 months ago

Updated Description

The original description is still valid. Here are the todos:

Original Description

I can add a TING that's openable by the current login (eg, BCREG0020) but not in the current account (eg, 668 - "resd"). However, when I try to file the amalgamation, the Legal API declares the TING to be "not affiliated to the current account" and rejects the filing.

In the examples below, you can see that my current login is "BCREG0020" but my current account is "resd". You can see that Create UI declares BC0871407 to be valid, and you can see that Filings UI open it just fine. However, I cannot submit this amalgamation filing as Legal API rejects it.

Sample AA: https://dev.create.business.bcregistry.gov.bc.ca/amalg-reg-review-confirm?id=Tvj5PwZZ1V&accountid=668

image.png

Sample TING: https://dev.business.bcregistry.gov.bc.ca/BC0871407

image.png

Error from Legal API:

image.png
severinbeauvais commented 3 months ago

@OlgaPotiagalova @NaveenHebbale Here is a newly found bug. Do you have a sense of whether Create UI should reject the subject TING, or whether Legal API should accept it? Do you think this is high priority or can it wait until the bug triage meeting on Thursday?

cc: @yuisotozaki

OlgaPotiagalova commented 3 months ago

@severinbeauvais We have to take a closer look at what the expected behavior should be in this case. Lets review and discuss during bug triage. Thank you.

yuisotozaki commented 3 months ago

This should fall into the same category as "Not affiliated with currently logged in account." What's the proper terminology here to distinguish the login account and the many user accounts that it can have?

severinbeauvais commented 3 months ago

@seeker25 Any thoughts on this? The Auth API endpoint to fetch entities/BCxxx is returning data for a business affiliated to a different account but for the same login.

seeker25 commented 3 months ago
        if not is_staff:
            if not _is_business_affliated(identifier, account_id):
                msg.append({
                    'error': (f'{identifier} is not affiliated with the currently '
                              'selected BC Registries account.'),
                    'path': amalgamating_businesses_path
                })
def _is_business_affliated(identifier, account_id):
    if ((account_response := AccountService.get_account_by_affiliated_identifier(identifier)) and
        (orgs := account_response.get('orgs')) and
            any(str(org.get('id')) == account_id for org in orgs)):
        return True
    return False
```
    @classmethod
def get_account_by_affiliated_identifier(cls, identifier: str):
    """Return the account affiliated to the business."""
    token = cls.get_bearer_token()
    auth_url = current_app.config.get('AUTH_SVC_URL')
    url = f'{auth_url}/orgs?affiliation={identifier}'

    res = requests.get(url,
                       headers={**cls.CONTENT_TYPE_JSON,
                                'Authorization': cls.BEARER + token})
    try:
        return res.json()
    except Exception:  # noqa B902; pylint: disable=W0703;
        current_app.logger.error('Failed to get response')
        return None
        ```
severinbeauvais commented 3 months ago

Looks like it returns data if the business is associated to any of the orgs (accounts) for the current login.

seeker25 commented 3 months ago

@vysakh-menon-aot tagging you on this, I think you wrote this bit

seeker25 commented 3 months ago

The affiliation is to a different org_id: 668 (shown in UI), 2288 shown in data

auth-db=# select org_id, name from affiliations a join entities e on e.id = a.entity_id where e.business_identifier = 'BC0871407';
 org_id |       name
--------+-------------------
   2288 | 0871407 B.C. LTD.
(1 row)

auth-db=# select distinct user_id from authorizations_view where org_id = 2288;
 user_id
---------
      12
(1 row)

auth-db=# select first_name, last_name from users where id = 12;
    first_name     | last_name
-------------------+-----------
 BCREGTEST Lucille | TWENTY
(1 row)

What's the account-ID on the request header? request.headers.get('account-id', request.headers.get('accountId', None)) Is it 2288 or 668?

668, probably wont work, 2288 will work

severinbeauvais commented 3 months ago

Travis, yes, I agree with your data. The thing is, should the entities auth info call return anything if the business is not affiliated to the current account (668 vs 2288)?

seeker25 commented 3 months ago

currently it only returns for 2288, I believe, wont show up for 668 - at least looking at the authorizations_view is called via:

                    auth = AuthorizationView.find_user_authorization_by_business_number(
                        business_identifier=business_identifier,
                        keycloak_guid=keycloak_guid,
                        org_id=user_from_context.account_id
                    )
severinbeauvais commented 3 months ago

Waiting for input from business and Patty. @NaveenHebbale and @yuisotozaki are chasing this down.

severinbeauvais commented 3 months ago

currently it only returns for 2288, I believe, wont show up for 668

The code might appear to do just this, but I am quite certain that, as account 668, I can fetch auth info (and legal api data) for a business that's affiliated to 2288 only.

Hence, this is a larger issue (not limited to amalgamations). What do you think?

Example: BC0871407 is affiliated to 2288, but I can open/use it as 668. MBR: https://dev.account.bcregistry.gov.bc.ca/account/2288/business Business dashboard: https://dev.business.bcregistry.gov.bc.ca/BC0871407?accountid=668

seeker25 commented 3 months ago

Sounds more like a bug? Your user account has both membership to 668 and 2288

I think if you're a user and your current account can't manage the business, we should show a dialog for them to switch accounts?

auth-db=# select distinct org_id, user_id from memberships where user_id = 12;
 org_id | user_id
--------+---------
      6 |      12
    132 |      12
    133 |      12
    134 |      12
    138 |      12
    668 |      12
   1904 |      12
   2288 |      12
   2956 |      12
   2973 |      12
   2983 |      12
   3041 |      12
   3077 |      12
   3134 |      12
(14 rows)
severinbeauvais commented 3 months ago

Your table above shows that login "BCREG0020" has several accounts, right?

But the question is, if I am using account 668, should I be able to access a business that's only affiliated to account 2288?

seeker25 commented 3 months ago

Yes, membership to multiple orgs.

Probably not, hence why I said it's a bug?

Each MBA is tied to an org ID. It's not combined etc.

I don't think in filings-ui or the calls after it the ?accountid=668 is considered

Because I don't see any indication of accountId passed down to the requests

severinbeauvais commented 3 months ago

Right. I think the KC token is used for authorization. But that's only the login, not the account, right?

I guess we can be explicit about passing down (and requiring) the account-id in all API calls. (We already do for some.)

seeker25 commented 3 months ago

Yup exactly, which is a bunch of work

severinbeauvais commented 3 months ago

So, do we turn this ticket into an epic with tickets under it to update all the UIs and APIs?

And what's your take (or your PO's opinion) on the priority of this?

seeker25 commented 3 months ago

Low priority, 692 users have multiple affiliations, 82993 unique user_ids in the affiliations table

severinbeauvais commented 3 months ago

Suggestion: we add a business account check in Create UI for amalgamations, which mitigates the current hiccup, and then we work on THIS ticket/epic as time allows (future).

severinbeauvais commented 3 months ago

Travis, I think the call to fetch auth info returns data without looking at the account ID: https://auth-api-dev.apps.silver.devops.gov.bc.ca/api/v1/entities/BC0871271

The response doesn't include the account ID. If it did, Create UI could reject the business from the amalgamation.

image

Is there a way I can check if the business is affiliated to the current account (authorized)?

seeker25 commented 3 months ago

Org-id was None, account_id, account_id_claim was None

was called: auth = cls.query.filter_by(keycloak_guid=keycloak_guid, business_identifier=business_identifier).first()

Probably would need it on the route to accept the account-id header and pass that into the auth check

severinbeauvais commented 3 months ago

Probably would need it on the route to accept the account-id header and pass that into the auth check

I was hoping the auth info could include a list of affiliated accounts. Are you suggesting that the UI attach the current account id and then the Auth API would conditionally return the data?

severinbeauvais commented 3 months ago

Travis, if I do this...

image

... what should I expect the response to be? Currently getting the following "CORS error":

image

seeker25 commented 3 months ago

I think the ideal response would be a 403, if you weren't authorized to that entity, if you were authorized.. probably the same payload as before

severinbeauvais commented 3 months ago

New ticket for Relationships team (probably): #19742

I think, if the above ticket is done, and Create UI attaches Account-Id to the auth info call, it will solve the issue of TINGs added to amalgamations when they are not actually affiliated to the current account.

severinbeauvais commented 3 months ago

I have updated this ticket's title and description. This is now a Create UI ticket (per the design discussed above).

Olga will discuss with Relationships team to see who will implement blocker ticket #19742.

I have created epic #19749 to look at the bigger picture of accounts accessing businesses in other accounts (with the same login).

THIS ticket is no longer blocked by requirements or design.

cc: @OlgaPotiagalova @seeker25

severinbeauvais commented 3 months ago

The work for this has been done and tested locally, and is captured in this PR: https://github.com/bcgov/business-create-ui/pull/650

The estimate for this ticket retroactively includes the work above, which was done as part of the investigation/design for this issue.

severinbeauvais commented 3 months ago

Test Notes

  1. As a regular user, try to resume an amalgamation that is affiliated to a different account. (should fail with error)
  2. As a regular user, try to resume an amalgamation that is affiliated to the current account. (should succeed)
  3. As a staff user, try to resume an amalgamation that is affiliated to a different account. (should succeed)

  4. As a regular user, try to add a business that is affiliated to a different account. (should fail with error)
  5. As a regular user, try to add a business that is affiliated to the current account. (should succeed)
  6. As a staff user, try to add a business that is affiliated to a different account. (should succeed)
  7. Try save and resume the draft. (should succeed and look the same as before saving)