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

Activate Per Tenant Settings Feat and use ACA-Py 0.8.2 image #682

Merged
merged 6 commits into from
Jul 27, 2023

Conversation

shaangill025
Copy link
Contributor

@shaangill025 shaangill025 requested a review from loneil July 4, 2023 20:04
@loneil

This comment was marked as outdated.

@loneil loneil temporarily deployed to development July 4, 2023 20:52 — with GitHub Actions Inactive
@github-actions
Copy link

github-actions bot commented Jul 4, 2023

@loneil
Copy link
Contributor

loneil commented Jul 4, 2023

(some tenants for testing)

Issuer:
260f147e-1d98-4488-b6ec-dd47b1c6d64f
fd967634-d290-4f54-a3ec-3469a15d4ff7

Not an issuer:
a60cc559-d273-4245-b1f6-577d6eeed8eb
ac27f6df-97a5-4b7c-9dce-2fec85ef3a21

@loneil
Copy link
Contributor

loneil commented Jul 5, 2023

Altering the settings through the UI seems to be working well from what I understand from testing out.

I have some questions about how default state should be handled (sorry if already discussed and I missed it).

Using the Auto Respond message field as the example here but I think it applies to all, depending on default values...
Right now if you pull up the settings on a new Tenant you'll see there is no settings property in the response for something that hasn't been 'set' yet

Going into a new Tenant, all the settings are unset (false for the switches)
image

Because nothing comes back from the wallet settings endpoint for these extra settings unless the user has already set one of the fields
image

So it looks like Auto Respond to Messages is false, but if I send the tenant a message the tenant does auto-respond
image

This would be because that's the system default I assume?

Toggling it on and then off and saving does then set the setting for that Tenant
image

image

So I think this is a possible confusion for Tenants if these settings appear 'turned off' but then they're actually possibly 'on' if that's the default system setting?
If I have that assumption correct...

So if this is the case I could see handling it in a couple ways

  1. Purely in the Tenant UI? Use a tristate checkbox? https://primevue.org/tristatecheckbox/
    Some other UX to display that the defaults are being used and opt in to setting yourself?
  2. Maybe when a tenant is created, have the settings persistence set to the current state of the fields defaults? Not sure if possible or other knock-on effects.
  3. If the setting is not set, then the api still returns the setting value, but pull from the default.

Option 1 is probably easy, could make another ticket to address that.
Questions for @esune about how we want Tenants to deal with this case.

Tagging @shaangill025 and @usingtechnology if they had any thoughts from the aca-py PR 2233. IE should this just be the Tenant UI's problem, or does this need default situation need any more consideration in the core?

@usingtechnology
Copy link
Contributor

good work integrating that and your analysis.
the simplest thing (i think) would to ALWAYS return the full (tenant) set of effective settings.

@swcurran
Copy link
Contributor

swcurran commented Jul 6, 2023

My $0.02CDN — if I’m understanding the issues:

  • ACA-Py allows tenants to update settings, get settings. I assume there are endpoints for those already.
  • Traction instances are configured (or hardcoded?) to have default settings for a new Tenant, applies the settings on/after creation of each tenant. These might include ACA-Py settings as well as Traction settings — e.g. “Is Issuer” Traction setting, “Has Public DID".
  • The Traction UI always uses get settings when settings need to be displayed.
  • The Traction UI to update settings updates the ACA-Py settings for the tenant.

@esune
Copy link
Member

esune commented Jul 6, 2023

Could we just make the settings explicit on tenant creation? i.e.: the tenant settings are set to what the instance defaults are on creation of a new tenant.
This would cause calls to the get settings endpoint to reflect the actual state, rather than return undefined and potentially lead to confusion. This might need to be a change in the multitenancy code - @shaangill025 can you chime in?

@loneil
Copy link
Contributor

loneil commented Jul 6, 2023

Could we just make the settings explicit on tenant creation? i.e.: the tenant settings are set to what the instance defaults are on creation of a new tenant. This would cause calls to the get settings endpoint to reflect the actual state, rather than return undeined and potentially lead to confusion. This might need to be a change in the multitenancy code - @shaangill025 can you chime in?

That makes the most sense to me as an API consumer I think. I create a Tenant, I ask it for what the current settings for that Tenant is, and it tells me the settings, same as if they'd been set by the user.
Unless it's that the settings are meant to only be overrides of the defaults. IE consider what happens if you create a Tenant, don't touch settings, and then re-set the system defaults (if that's possible)
Not sure if there's downsides on the ACA-Py side or anything.

@esune
Copy link
Member

esune commented Jul 6, 2023

That makes the most sense to me as an API consumer I think. I create a Tenant, I ask it for what the current settings for that Tenant is, and it tells me the settings, same as if they'd been set by the user. Unless it's that the settings are meant to only be overrides of the defaults. IE consider what happens if you create a Tenant, don't touch settings, and then re-set the system defaults (if that's possible) Not sure if there's downsides on the ACA-Py side or anything.

I think we should make the API as straightforward as possible, which means return whichever value that setting is for the tenant - even if it is an override of the defaults, I will need to know what the default values are.

@usingtechnology
Copy link
Contributor

usingtechnology commented Jul 6, 2023

it's only a small set that the user/tenant can self-manage. i think that complete set should get returned all the time. i guess the decision would be whether what is stored in wallet are just the changes (overrides) and those get layered on top of the system settings or if you do as @esune suggests and copy the current system settings into the wallet and that is what the tenant manages. that is probably the simplest way. could add a method to reset to system defaults too (remove all your customizations). it may make it difficult to add new settings (or remove) to the tenant but you could probably figure a way to "sync" the allowable set when the wallet is queried for it's extra_settings.

But i don't think you want traction/tenant-ui to know what the set is and find out system defaults to send to the frontend. the API/acapy returns the manageable set.

@loneil

This comment was marked as resolved.

@loneil loneil closed this Jul 18, 2023
@loneil loneil deleted the pr_580_followup branch July 18, 2023 23:23
@loneil loneil restored the pr_580_followup branch July 18, 2023 23:49
@loneil loneil reopened this Jul 18, 2023
@loneil

This comment was marked as resolved.

@shaangill025 shaangill025 temporarily deployed to development July 20, 2023 23:45 — with GitHub Actions Inactive
Copy link
Contributor

@loneil loneil left a comment

Choose a reason for hiding this comment

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

Curious where the endorser role is meant to come from?

It should be author for tenants that are created right?
The deployment values have
endorser-protocol-role: author

But in /settings there isn't the endorser role being shown. Though it has the other dependent auto flags set to true

image

So on the settings page it's defaulting it to none but is it supposed to be author?

image

@loneil
Copy link
Contributor

loneil commented Jul 21, 2023

See review comments above, but another question (forgive me if I missed this earlier, was only ever looking at the initial specification for the PUT /multitenancy/wallet before) I have is about the difference between the field naming for editing these settings vs fetching them.

When a consumer calls

GET https://pr-682-traction-tenant-proxy-dev.apps.silver.devops.gov.bc.ca/tenant/wallet
to see the subwallet, all these settings come back in this form
image

But when updating with
PUT https://pr-682-traction-tenant-proxy-dev.apps.silver.devops.gov.bc.ca/tenant/wallet
it's extra_settings instead of settings and the field names are the env var style
image

Not sure if could be adjusted or if it's an ACA-Py limitation or anything, but just raising that it's confusing from a REST standpoint IMO.

@shaangill025
Copy link
Contributor Author

@loneil The settings in the response schema are settings already saved to the wallet_record. This is different than the extra_settings in the request schema, where configurable per tenant settings can be provided and saved to the wallet_record.
Re env_var, the following objects provide the association keys to configurable settings in ACA-Py:

ACAPY_LIFECYCLE_CONFIG_FLAG_MAP = {
    "ACAPY_LOG_LEVEL": "log.level",
    "ACAPY_INVITE_PUBLIC": "debug.invite_public",
    "ACAPY_PUBLIC_INVITES": "public_invites",
    "ACAPY_AUTO_ACCEPT_INVITES": "debug.auto_accept_invites",
    "ACAPY_AUTO_ACCEPT_REQUESTS": "debug.auto_accept_requests",
    "ACAPY_AUTO_PING_CONNECTION": "auto_ping_connection",
    "ACAPY_MONITOR_PING": "debug.monitor_ping",
    "ACAPY_AUTO_RESPOND_MESSAGES": "debug.auto_respond_messages",
    "ACAPY_AUTO_RESPOND_CREDENTIAL_OFFER": "debug.auto_respond_credential_offer",
    "ACAPY_AUTO_RESPOND_CREDENTIAL_REQUEST": "debug.auto_respond_credential_request",
    "ACAPY_AUTO_VERIFY_PRESENTATION": "debug.auto_verify_presentation",
    "ACAPY_NOTIFY_REVOCATION": "revocation.notify",
    "ACAPY_AUTO_REQUEST_ENDORSEMENT": "endorser.auto_request",
    "ACAPY_AUTO_WRITE_TRANSACTIONS": "endorser.auto_write",
    "ACAPY_CREATE_REVOCATION_TRANSACTIONS": "endorser.auto_create_rev_reg",
    "ACAPY_ENDORSER_ROLE": "endorser.protocol_role",
}

ACAPY_LIFECYCLE_CONFIG_FLAG_ARGS_MAP = {
    "log-level": "log.level",
    "invite-public": "debug.invite_public",
    "public-invites": "public_invites",
    "auto-accept-invites": "debug.auto_accept_invites",
    "auto-accept-requests": "debug.auto_accept_requests",
    "auto-ping-connection": "auto_ping_connection",
    "monitor-ping": "debug.monitor_ping",
    "auto-respond-messages": "debug.auto_respond_messages",
    "auto-respond-credential-offer": "debug.auto_respond_credential_offer",
    "auto-respond-credential-request": "debug.auto_respond_credential_request",
    "auto-verify-presentation": "debug.auto_verify_presentation",
    "notify-revocation": "revocation.notify",
    "auto-request-endorsement": "endorser.auto_request",
    "auto-write-transactions": "endorser.auto_write",
    "auto-create-revocation-transactions": "endorser.auto_create_rev_reg",
    "endorser-protocol-role": "endorser.protocol_role",
}

@shaangill025 shaangill025 temporarily deployed to development July 21, 2023 13:26 — with GitHub Actions Inactive
@shaangill025 shaangill025 temporarily deployed to development July 21, 2023 16:02 — with GitHub Actions Inactive
@loneil
Copy link
Contributor

loneil commented Jul 21, 2023

@loneil The settings in the response schema are settings already saved to the wallet_record. This is different than the extra_settings in the request schema, where configurable per tenant settings can be provided and saved to the wallet_record. Re env_var, the following objects provide the association keys to configurable settings in ACA-Py:

Ok, thanks for the explanation @shaangill025 . Yeah just that what results in the REST shape for the multitenancy endpoints I think could end up confusing for a Traction consumer (PUTing and GETing on the same endpoint). I'm fine with it as it is if there is a difference between the two concepts as you said, I think most usage of this would be through the Tenant UI anyways which abstracts consumers really needing to know about it.

shaangill025 and others added 3 commits July 25, 2023 13:37
Signed-off-by: Shaanjot Gill <[email protected]>
Signed-off-by: Lucas ONeil <[email protected]>
@hiteshgh
Copy link

hiteshgh commented Jul 25, 2023

moved to review/qa as per feedback from @shaangill025

Signed-off-by: Shaanjot Gill <[email protected]>
@shaangill025 shaangill025 temporarily deployed to development July 25, 2023 21:49 — with GitHub Actions Inactive
@shaangill025 shaangill025 temporarily deployed to development July 26, 2023 23:43 — with GitHub Actions Inactive
Copy link
Member

@esune esune left a comment

Choose a reason for hiding this comment

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

Looks good to me! @loneil if you want to take a last look before merging it would be great

@loneil
Copy link
Contributor

loneil commented Jul 27, 2023

I think it looks good. I don't know enough about all the settings to test them out but I tried a couple on and off and got the expected results.
Did basic regression testing on the deployed PR and could create tenants and connect and issue and all that.
👍

@esune esune merged commit 3c188a1 into main Jul 27, 2023
@esune esune deleted the pr_580_followup branch July 27, 2023 23:53
@esune esune temporarily deployed to development July 27, 2023 23:53 — with GitHub Actions Inactive
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.

6 participants