Fix #1783

[zg009]

This is the thread for addressing #1783

Current has some work to register hostname on server creation and apply it to AccountTemplate class as a static variable

Ran some tests with localhost webId resolution.

Integration tests probably need more granularity, also strategy to handle multiuser mapping due to prefix path rather than postfix.

[zg009]

@jeff-zucker If you'd like to pull this branch and see if it fulfills your needs, let me know. I can fix issues as they appear or don't resolve properly, but this will serve as the starting point.

[jeff-zucker]

@jeff-zucker If you'd like to pull this branch and see if it fulfills your needs, let me know. I can fix issues as they appear or don't resolve properly, but this will serve as the starting point.

The code looks right. I'll download and test tomorrow. Thanks much!

[bourgeoa]

@zg009 Thanks. I think this is correct. You may make :

• some cleaning in static registerHostName()
• minor but I would rename realWebId --> webIdPathToPod or podRelativeWebId or webIdRelToPodRoot not easy to explain. The intention is to get the WebId relative to pod Root
• your tests are OK but do not show that except external Webid in NSS the resulting relative WebId will be --> /profile/card#me

[zg009]

@bourgeoa I addressed your first and second point. I need more clarity about what I need to check in the test suite.

[bourgeoa]

Your code works but I think (to be discussed) it does not respect the actual code logic (I did not create it nor help maintain this part)

I thin k your function should be implemented here https://github.com/nodeSolidServer/node-solid-server/blob/07ee53cc3857fa050232cc7723f7a7abd440dc13/lib/models/account-manager.js#L67

To avoid

[zg009]

@bourgeoa Are you saying in the static from method, move the logic from current changes into the from method and set the 'host' variable in that way?

[bourgeoa]

Yes but I am not sure of me at all

[bourgeoa]

I was thinking that from was returning all data needed from input

[zg009]

The only places I see AccountManager and AccountTemplate interacting is here https://github.com/nodeSolidServer/node-solid-server/blob/07ee53cc3857fa050232cc7723f7a7abd440dc13/lib/models/account-manager.js#L390-L402

If you trace AccountTemplate.for(), it is the only code which uses AccountTemplate.templateSubstitutionsFor().

I could modify the AccountTemplate.for method to take hostname from AccountManager object from AccountManager.createAccountFor.

EDIT: Actually, looking at the code further, the same end goal is reached. I do not have strong feelings about where to move the host server checking logic. It seems logical to pass hostname to AccountTemplate.for and change webId path based upon that.

[bourgeoa]

To improve cleaning the logic this should be moved : https://github.com/nodeSolidServer/node-solid-server/blob/18b7cb3c0b6506554365be66bcbd1771807d2946/lib/create-app.js#L56 inside function initWebId

And may be passed via accountManager with a new parameter argv.serverUri to the API.accounts.middleware()

app.use('/', API.accounts.middleware(accountManager))

[bourgeoa]

I wonder if there is a situation where there is any difference between registerHostname() and https://github.com/nodeSolidServer/node-solid-server/blob/18b7cb3c0b6506554365be66bcbd1771807d2946/lib/models/account-manager.js#L156

What do you think ? May be with external WebId ?

[bourgeoa]

I marked it as draft because it is not working. I think the logic is more complex between

• the user form to create a userAccount
• the userAccount going to create the pod using the templates

The podUri is not stored in userAccount. May be look at rootAclUri

[zg009]

Yeah, I figured this wouldn't be so simple. I'll have some time to take a deeper dive later this week or early June, I am busy with my current work right now.

[bourgeoa]

It is the first time I go in the logic of the account manager of NSS code. Anyway thanks for looking.

[jeff-zucker]

Yeah, I figured this wouldn't be so simple. I'll have some time to take a deeper dive later this week or early June, I am busy with my current work right now.

Thanks much for looking in to it. I do not see this as in any way high priority and can deal with it in other ways, so there is no rush on my account. And I can't think that's priority for others either.

[bourgeoa]

@jeff-zucker I agree it is not high priority. But when you get in, and thinks it is quite simple and don't find the code logic you somehow are frustrated.

@zg009 I think I resolved it. Thanks again for your help. Could you :

• replace failing tests and add any needed ones
• /profile/card#me should use predefined constants

[zg009]

@jeff-zucker I agree it is not high priority. But when you get in, and thinks it is quite simple and don't find the code logic you somehow are frustrated.

@zg009 I think I resolved it. Thanks again for your help. Could you :

• replace failing tests and add any needed ones
• /profile/card#me should use predefined constants

You will need to clarify what predefined constants I should be using for these tests.

[bourgeoa]

This is not in the tests, but to improve the code to use https://github.com/nodeSolidServer/node-solid-server/blob/1034123e268365230342894db825f8f419af1b32/lib/models/account-manager.js#L50-L51

in place of /profile/card#me

[zg009]

@bourgeoa I edited your function a little bit because the webID document was including port. If this is unintended behavior I can revert. I also added one test case. If you want to see other test cases, let me know.

[jeff-zucker]

@bourgeoa I edited your function a little bit because the webID document was including port.

Yes, this is important, users ought to be able to change the port of the server without having to recreate an account.

[bourgeoa]

replaced wirh url.origin and updated tests that should have failed on relative webId and did not.

[zg009]

@bourgeoa Looks better to me, I had the wrong idea about which tests should pass

[zg009]

@bourgeoa Is this good to merge?