DNNCommunity / Dnn.CommunityForums

Open-source forums module for DNN Platform. This is a fork and continuance of the Active Forums module.
https://dnncommunity.org
Other
13 stars 21 forks source link

Web API Implementation #53

Open WillStrohl opened 3 years ago

WillStrohl commented 3 years ago

Is your feature request related to a problem?

We need to transition the module to a more modern architecture. One of the steps required for this is to begin separating front-end and back-end logic.

As much as possible, the back-end needs to be Web API-based so we can have the option for an actual API that other extensions can use and the possibility of this module being forward compatible as much as possible for future DNN versions and/or a transition to an MVC module pattern. Doing this will also ensure the maximum amount of forward compatibility.

Describe the solution you'd like

See above.

For the first phase of this, there shouldn't be any SPA or other similar work done yet. This first step should only be to separate front-end and back-end logic and code as much as possible.

Later, we can then consider reorganizing views and/or implementing more SPA-based implementations.

Describe alternatives you've considered

None at this time.

johnhenley commented 1 year ago

@WillStrohl I'm working on this (to implement some features like #174) and wanted to get your opinion. For certain features (example "like"), DnnAuthorize() attributes won't be sufficient given the forums permissions model. Would you recommend inheriting DnnApiController and supporting security levels specific to the forums? Or duplicate the security logic that is in the UI in each web api?

skamphuis commented 1 year ago

@johnhenley Awesome that you're working on this. I know I'm not @WillStrohl but I figured I'd give you my thoughts anyway. I inherit DnnApiController often, but mainly to either do something on every api call (you can use the constructor for that) or to be able to use it for reflection.

In this case, I'd probably create an attribute and do the security level checking there. The other choice would be to begin each method with some code to check security. I'd prefer creating an attribute and use it together with DnnAuthorize. Having a look at DnnModuleAuthorize will give you a start.

johnhenley commented 1 year ago

@skamphuis thanks for your reply. Really appreciate it! I was leaning towards the inheritance option. Although I think it's a little more work initially, it probably means less work in the long run and is certainly cleaner.

skamphuis commented 1 year ago

👍 Feel free to contact me when needed. I have created a few custom attributes like this before.

johnhenley commented 1 year ago

👍 Feel free to contact me when needed. I have created a few custom attributes like this before.

Thanks. I did some work on it and ended up with a helper function as the first line in the api that handles it at least for now. The problem I ran into with the custom attribute is that the api context knows the portal, module, and user, but doesn't know which forum is being accessed.

skamphuis commented 1 year ago

👍 Feel free to contact me when needed. I have created a few custom attributes like this before.

Thanks. I did some work on it and ended up with a helper function as the first line in the api that handles it at least for now. The problem I ran into with the custom attribute is that the api context knows the portal, module, and user, but doesn't know which forum is being accessed.

Sounds like a good solution without having to jump through unnecessary hoops.

johnhenley commented 1 year ago

👍 Feel free to contact me when needed. I have created a few custom attributes like this before.

Thanks. I did some work on it and ended up with a helper function as the first line in the api that handles it at least for now. The problem I ran into with the custom attribute is that the api context knows the portal, module, and user, but doesn't know which forum is being accessed.

Sounds like a good solution without having to jump through unnecessary hoops.

I realized the attribute approach works fine as long as the forumId is part of the URL as a route data or query string.

Code implementing the attribute (if the attribute is used and forumId isn't part of the URL, throws invalidOperationException): image

Here's an API with the new attribute: image

WillStrohl commented 1 year ago

Thanks, @skamphuis ... I was leaning toward the same solution.

@johnhenley We eventually want to implement a more robust URL Provider later... Will this approach have any negative impact on that?

johnhenley commented 1 year ago

Thanks, @skamphuis ... I was leaning toward the same solution.

@johnhenley We eventually want to implement a more robust URL Provider later... Will this approach have any negative impact on that?

What do you envision for a "more robust URL Provider"?

WillStrohl commented 1 year ago

One that includes some of the original post title in it.

skamphuis commented 1 year ago

@johnhenley We eventually want to implement a more robust URL Provider later... Will this approach have any negative impact on that?

This authorizeAttribute will be used on the WebAPI for the forums and be called from the module's own client side code. A better URL Provider would be interpreting the browser URL and not the API calls, so I don't expect any issues there. Or am I missing something?

johnhenley commented 1 year ago

One that includes some of the original post title in it.

That's already included unless I'm misunderstanding. Here's a url from my site:

https://lawsonguru.com/forums/financial/s3-financials/acbrgm-in-foreign-company/

On dnncommunity.org I don't think it's configured?

johnhenley commented 1 year ago

@johnhenley We eventually want to implement a more robust URL Provider later... Will this approach have any negative impact on that?

This authorizeAttribute will be used on the WebAPI for the forums and be called from the module's own client side code. A better URL Provider would be interpreting the browser URL and not the API calls, so I don't expect any issues there. Or am I missing something?

Agreed. And the URL for webapi/serviceframework includes /API/ so shouldn't conflict.