moov2 / Orchard.ActiveDirectoryAuthorization

Module for Orchard CMS that handles authorization for active directory users.
19 stars 15 forks source link

Question - ActiverDirectoryUser ContentItem is null - potential issue? #6

Closed hughknz closed 11 years ago

hughknz commented 11 years ago

Hi there. I'm new to Orchard module development, so I may not have all the correct information in my head when I'm asking this :).

I noticed that ActiverDirectoryUser.ContentItem is null. Since IUser extends IContent, will it be a good idea to actually return a ContentItem (if possible) ?

I first noticed this when I used the module with the Bootstrap theme. The theme used @Html.ItemDisplayText(IContent) on WorkContext.CurrentUser, which would be an ActivtyDirectoryUser (instead of an UserPart) if the user does not have admin rights. The html helper will currently throw a null reference exception because it is expecting a ContentItem.

(At the moment, I worked around this by creating an UserPart for every authenticated user. )

peterkeating commented 11 years ago

Hey,

We have made that change to the code also and it will be in the latest version of the module when we release it.

See https://github.com/moov2/Orchard.ActiveDirectoryAuthorization/commit/7fb554e42f6fb40d6ea06a107e98257a951a75f8

Feel free to download the project from Github and use the module though and let us know any other problems. The list of fixes and updates can be found in the README for the project.

hughknz commented 11 years ago

Thanks for the quick reply.

I have just downloaded the current code from GitHub. While testing I've found the following issue. It's a small issue and there are workarounds, but it might be worth while to resolve in the future.

When debugging through the authentication process for a new user (first request), the membership service is returning a null user inside ActiveDirectoryAuthenticationService.GetAuthenticatedUser()'. This is happening even thoughAuthorizer..CreateUser` has ran before it. This is in the same request, so it could be because the session/transaction has not been committed yet.

This causes a few null reference exceptions in the CommonParHandler, but since they are not fatal, they are only logged. The created user does get stored and retrieved correctly at the end of request if no fatal error occurs. This is fine for the default theme. If a theme (such as the Bootstrap one) relies on the content item, then it would throw another null reference.

peterkeating commented 11 years ago

Ah okay, thanks for bringing that to our attention. Will take a look at it when I get a moment, although feel free to get involved and submit a pull request if you think you can fix it :)

Will re-open this issue.

peterkeating commented 11 years ago

Just gone through and tested this issue with a first time user and the Bootstrap theme and it seems to be fixed. Feel free to re-open the issue if you find any other problems around the same issue.