-
Notifications
You must be signed in to change notification settings - Fork 226
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
B2B sample app improvements #405
B2B sample app improvements #405
Conversation
@@ -47,6 +47,8 @@ Now we need **two** applications to communicate with the **Guardio-Business-App* | |||
|
|||
Create two applications named **Guardio-Business-App** and **Guardio-Admin-App** [This should be a management application]. | |||
|
|||
Share both applications with all the organizations by clicking the share button. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was no place mentioning about application sharing. Also the applications have to be shared at the moment of creation to view the organization_switch grant.
* Also, On the User Attributes tab, click on + Add User Attributes. | ||
* Select `Email`, `First Name`, `Last Name`, and `Username` from the list of attributes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section is not related to this step and moved to the top after the admin app creation.
@@ -65,6 +67,9 @@ Select `Email`, `First Name`, `Last Name`, and `Username` from the list of attri | |||
>| Authorized redirect URLs | `http://localhost:3001/api/auth/callback/wso2isAdmin` and `http://localhost:3001` | | |||
>| Allowed origin | `http://localhost:3001` | | |||
|
|||
Also, On the User Attributes tab, click on + Add User Attributes. | |||
Select `Email`, `First Name`, `Last Name`, and `Username` from the list of attributes. | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved from bottom as mentioned in this comment
"ApplicationConfig": { | ||
"SampleOrganization": [ | ||
{ | ||
"id": "<SUB ORGANIZATION ID>", | ||
"name": "<SUB ORGANIZATION NAME>" | ||
} | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These configs are not required as user_organization claim is present in the ID token. The IS distributions with B2B feature where the user_organization claim is not presented can be affected due to removing this config. But we can suggest the previously released b2b-sample app for them to use, while keeping the latest b2b sample app up to date with latest changes without unnecessary configurations which can mislead the users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial release of the b2b app - https://github.com/wso2/samples-is/releases/download/v4.5.3/b2b-sample.zip
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather removing the configurations shall we update the readme with the proper description? Because as Sadil mentioned the sample will not work with IS distributions. This can be a problem when we release the GAs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The developers would ask why we need this configuration https://github.com/wso2/samples-is/pull/405/files#diff-34fe1b8cbee00497c7574755a2a49dc01937e2a04b08a16c2e7d3cfd24822197L88. Also I have seen some of them set the sub org id there, but it is not required. Hence why we can't remove that unnecessary config in order to not to confuse or answer back from us why that config stands for
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it was for backward compatibility, but we have previous b2b sample app releases which can be used for WSO2 PCC (The backward compatibility aid will be needed until PCC backed by IS 6.1). With these facts, still do we need that config in our latest sample. Please share the recommendations.
"ManagementAPIConfig": { | ||
"SharedApplicationName": "Guardio-Business-App", | ||
"UserStore" : "DEFAULT" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These configs are used by the admin app. Hence keeping these configs within business app can cause confusions. By moving that config to admin app config section will be helpful incase the config.json file was required to be managed as two separate files to configure for the maintainability.
}, | ||
"ManagementAPIConfig": { | ||
"SharedApplicationName": "Guardio-Business-App", | ||
"UserStore" : "DEFAULT" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved the configs to an appropriate place as discussed in the comment
* If the federated organization id is returned in the idToken (in user_organization variable) when a user is logging | ||
* into an organization, remove the `CommonConfig.ApplicationConfig.SampleOrganization` section from the <i>config.json</i> file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These settings may not required to be mentioned as normal users of the b2b sample app would be the users who try the b2b organization management feature of Asgardeo. For the users who try the b2b sample app with the PCC, we can suggest in the PCC documentation for the previously released b2b sample app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better if we have this part (with a change), mentioning that, in the PCC environment the developer needs to setup this to work with the sample application.
**_NOTE:_** With the latest B2B organization management features provided by WSO2 Identity Server, the following operations are allowed in sub organization for the organization creator. | ||
|
||
1 - onboarding enterprise IDP. | ||
2 - Update authentication options of the shared applications. | ||
3 - Delete the business users. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the latest features, above operations can be done from the Console app itself which exceeds the capabilities of the b2b sample app. In order to not to limit the functionalities which can be done at the sub organization by the less features of the b2b sample app, we can suggest to check the Console app.
ex - configure multiple authentication steps / options
- delete users
@@ -159,22 +159,12 @@ setup the config json file as mentioned above. | |||
|
|||
### Step 6: Setup the identity provider for the Guardio-Business-App | |||
|
|||
* For this we will create an external identity provider in Asgardeo. After creating an identity provider in Asgardeo. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
external identity provider in Asgardeo
-> external identity provider in sub organization
### Step 7: Create a corporate user | ||
* You need to create a new corporate user to access the **Guardio-Business-App**. | ||
|
||
* To create a corporate user for Best Auto Mart organization go to the Asgardeo console where you set up the corporate IDP | ||
and create a user named `John`. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Business user will be log in via the Enterprise IDP. Even after configuring the enterprise IDP, the basic authentication option will be removed. In such cases, there is no point of creating a business user in the sub organization itself.
@@ -187,7 +177,7 @@ setup the config json file as mentioned above. | |||
> **_NOTE:_** If `nx serve business-app` produced an error, try with `npx nx serve business-app` | |||
|
|||
* Open a **private browser** and type [http://localhost:3000](http://localhost:3000) with your browser to see the result. | |||
* Login from the created user `John` to the application. | |||
* Login via the configured enterprise IDP to the application. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -40,7 +40,7 @@ export default async function viewUsers(req: NextApiRequest, res: NextApiRespons | |||
|
|||
try { | |||
const fetchData = await fetch( | |||
`${getOrgUrl(orgId)}/scim2/Users`, | |||
`${getOrgUrl(orgId)}/scim2/Users?count=100`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix for users not listing the sample app. Without a count, the SCIM user list API response will be in below format.
{
"totalResults": 1,
"startIndex": 1,
"itemsPerPage": 0,
"schemas": [
"urn:ietf:params:scim:api:messages:2.0:ListResponse"
]
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we add domain=<userstore>
attribute if there is a defined userstore here https://github.com/wso2/samples-is/pull/405/files#diff-34fe1b8cbee00497c7574755a2a49dc01937e2a04b08a16c2e7d3cfd24822197R130
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding count is not necessary right. IMO, looks like the SCIM api is broken
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to fix the SCIM API. Also need to add the #405 (comment) improvement
} else if (token.user.org_id) { | ||
|
||
return token.user.org_id; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix for the B2C user login issue. In B2C login cases, the user_organization claim won't be in the id token. The org_id claim of the token will represent the user_organization in such cases.
@@ -47,6 +47,8 @@ Now we need **two** applications to communicate with the **Guardio-Business-App* | |||
|
|||
Create two applications named **Guardio-Business-App** and **Guardio-Admin-App** [This should be a management application]. | |||
|
|||
Share both applications with all the organizations by clicking the share button. | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why Guardio-Business-App needs a client credential grant?
I think it should be changed to Organization Switch
and Code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, We can skip client credential
grant
Fixed by - 221eee7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we also have the current version released as a version, before merging this PR ?
} else if (config.CommonConfig.ApplicationConfig.SampleOrganization[0]) { | ||
|
||
return config.CommonConfig.ApplicationConfig.SampleOrganization[0].id; | ||
} else { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we change the order of the config check rather removing them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, based on the conclusion of this discussion https://github.com/wso2/samples-is/pull/405/files#r1133118204, will attend to this
Some of the fixes were already addressed in separate PRs. |
Purpose