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

Update of open id recipes #7646

Closed
wants to merge 11 commits into from

Conversation

Tsjerno
Copy link
Contributor

@Tsjerno Tsjerno commented Nov 15, 2020

Update of openID application Step

  • not using viewmode
  • recipe can be run multiple times
    Update of openID scope Step
  • not using viewmode
  • recipe can be run multiple times ScopeController

Update of openID scope page

  • Give user feedback when spaces are added in the name

Fixes #6364

 - not using viewmode
 - recipe can be run multiple times
Update of openID scope Step
 - not using viewmode
 - recipe can be run multiple times ScopeController

Update of openID scope page
 - Give user feedback when spaces are added in the name
Copy link
Member

@kevinchalet kevinchalet left a comment

Choose a reason for hiding this comment

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

Thanks for your PR!

@agriffard agriffard changed the title Update of open id recipes (issue #6364) Update of open id recipes Nov 24, 2020
@agriffard
Copy link
Member

@kevinchalet Can you please review this PR?


var descriptor = new OpenIdApplicationDescriptor
var descriptor = new OpenIdApplicationDescriptor();
if (application!=null)
Copy link
Member

Choose a reason for hiding this comment

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

nit: spaces.


descriptor.Permissions.Remove(OpenIddictConstants.Permissions.GrantTypes.ClientCredentials);
Copy link
Member

Choose a reason for hiding this comment

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

It would have been better to use the same if/else logic as the one used in the controllers to make things more consistent and avoid discrepancies between recipes and controllers, but it shouldn't be blocking to merge this PR,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it better to create an OpenIdApplicationService with an Create and Edit function with an ApplicationModel?
Then use this service in the recipe and the controller?

Copy link
Member

Choose a reason for hiding this comment

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

It's an option, but the problem is that the step and view models are 2 different CLR classes, so we'd to either merge the classes or introduce some interfaces. Dunno if it's worth it or not.

If anything, it would be better to put that in IOpenIdApplicationManager.

if (model.AllowAuthorizationCodeFlow)
{
descriptor.Permissions.Add(OpenIddictConstants.Permissions.GrantTypes.AuthorizationCode);
}
if (model.AllowClientCredentialsFlow)
{
descriptor.Permissions.Add(OpenIddictConstants.Permissions.GrantTypes.ClientCredentials);

if (model.Roles!= null && model.Roles.Any())
Copy link
Member

Choose a reason for hiding this comment

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

nit: formatting

hs added 7 commits December 10, 2020 21:13
 - not using viewmode
 - recipe can be run multiple times
Update of openID scope Step
 - not using viewmode
 - recipe can be run multiple times ScopeController

Update of openID scope page
 - Give user feedback when spaces are added in the name
@deanmarcussen
Copy link
Member

I think you had a bad merge here as there are over 1200 file changes.

Can you reset the pr please?

@Piedone
Copy link
Member

Piedone commented Jan 14, 2024

The #6364 issue has since been fixed by other PRs. Thank you for your contribution, but I'm now closing this.

@Piedone Piedone closed this Jan 14, 2024
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.

OpenId Deployment and Recipes have some issues
5 participants