bcgov / business-filings-ui

BC Registry Services - Legal Entities - Business Dashboard and Filings
Apache License 2.0
9 stars 51 forks source link

feat: DBC feature is only available to self-registered owner/operators #589

Closed amanji closed 9 months ago

amanji commented 10 months ago

Issue #: /bcgov/entity#18688

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the business-filings-ui license (Apache 2.0).

severinbeauvais commented 10 months ago

Overall, isn't the check done here something the Legal API should do as part of allowable actions?

cc: @argush3

argush3 commented 10 months ago

Overall, isn't the check done here something the Legal API should do as part of allowable actions?

cc: @argush3

Yes, ideally we would have a check in allowable actions.

I'm not sure how we flag that check to occur though.

severinbeauvais commented 10 months ago

Overall, isn't the check done here something the Legal API should do as part of allowable actions? cc: @argush3

Yes, ideally we would have a check in allowable actions.

I'm not sure how we flag that check to occur though.

Well, the Legal API knows who the current user is and can compare with the current business proprietor, right?

argush3 commented 10 months ago

Well, the Legal API knows who the current user is and can compare with the current business proprietor, right?

If we are talking about blocking the registration filing, we can probably check the logged in user against the proprietor.

But I need to know when that check needs to happen.

Not all SPs will be using DBC and so shouldn't be blocked on this condition.

Probably a bit hard to talk about this in this PR.

severinbeauvais commented 10 months ago

Well, the Legal API knows who the current user is and can compare with the current business proprietor, right?

If we are talking about blocking the registration filing, we can probably check the logged in user against the proprietor.

No, this is to block ~access to a Sole Prop~ a Sole Prop's access to the DBC action/pages. The original registration was in the past. The check may need to find that registration and compare the completing party, not sure yet.

But I need to know when that check needs to happen.

I'm hoping the allowable actions from the Legal API can simply return "not allowed" for DBC if the current user isn't the proprietor (and perhaps the completing party).

Not all SPs will be using DBC and so shouldn't be blocked on this condition.

Right. The only thing that might be blocked is access to the DBC action/pages depending on the current user.

severinbeauvais commented 10 months ago

/gcbrun

bcregistry-sre commented 10 months ago

Temporary Url for review: https://business-filings-dev--pr-589-fwm9m23s.web.app

SB says, try this: https://business-filings-dev--pr-589-fwm9m23s.web.app/FM1059658?accountid=2288 (using BCREG0020)

sonarcloud[bot] commented 10 months ago

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

amanji commented 10 months ago

Im thinking about this a bit more and I do agree we should create a rule engine in the API to expose this feature. It could further be used to protect the DC API endpoints.

If this can be used as a stop-gap in the meantime we can always issue a PR to revert this out of the codebase since it's a pretty isolated change.

severinbeauvais commented 10 months ago

Im thinking about this a bit more and I do agree we should create a rule engine in the API to expose this feature. It could further be used to protect the DC API endpoints.

Yes, the "allowable actions" is the API rule engine already. We should keep investigating this approach.

If this can be used as a stop-gap in the meantime we can always issue a PR to revert this out of the codebase since it's a pretty isolated change.

I'd be ok with this. Will there be time left in your project to change this later?

severinbeauvais commented 10 months ago

Test Results

With proprietor == completing party == current user (BCREG0020): https://business-filings-dev--pr-589-fwm9m23s.web.app/FM1059658?accountid=2288 -> Digital Business Cards menu item is available [PASS]

With proprietor != completing party == current user: https://business-filings-dev--pr-589-fwm9m23s.web.app/FM1019176/?accountid=2288 -> Digital Business Cards menu item is NOT available [PASS]

With proprietor == completing party != current user (BCREGA001): https://business-filings-dev--pr-589-fwm9m23s.web.app/FM1059658?accountid=2079 -> Digital Business Cards menu item is NOT available [PASS]

I also tested the following, and was NOT able to see the DBC page: https://business-filings-dev--pr-589-fwm9m23s.web.app/FM1019176/digital-credentials/ [PASS] https://business-filings-dev--pr-589-fwm9m23s.web.app/FM1059658/digital-credentials/ [PASS]

amanji commented 10 months ago

Im thinking about this a bit more and I do agree we should create a rule engine in the API to expose this feature. It could further be used to protect the DC API endpoints.

Yes, the "allowable actions" is the API rule engine already. We should keep investigating this approach.

If this can be used as a stop-gap in the meantime we can always issue a PR to revert this out of the codebase since it's a pretty isolated change.

I'd be ok with this. Will there be time left in your project to change this later?

I think so. I have some runway to add in tests for the queue so I might as well add in a second PR for this work. I don't imagine it would take long because we know exactly what rule to implement.

severinbeauvais commented 10 months ago

Will there be time left in your project to change this later?

I think so. I have some runway to add in tests for the queue so I might as well add in a second PR for this work. I don't imagine it would take long because we know exactly what rule to implement.

OK, then make a quick plan with Argus on how to do this, and if that looks feasible then I will approve this PR for merging. Thx.

severinbeauvais commented 10 months ago

Is this feature (users check) required for MVP?

If so, and if we have an actionable plan to implement this in the API instead, then I will approve this PR. Otherwise, I'd rather not have this code in the UI for an undetermined period of time.

amanji commented 9 months ago

Closing this as logic has been moved to API. Will open a new PR