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

Added column "Created by" to hashlist overview #778

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

dr0pd34d
Copy link

@dr0pd34d dr0pd34d commented Jun 4, 2022

As a group of people using the tool we always wondered where to see which user created the hashlist in order to provide support or give advice.
I thought I would try my best in order to add the feature through a pull request.

For now it is only shown in the "Hashlists" overview as a sortable column.
As so far the ID of the creating user has not been saved with the hashlist, I created the column in the database setup file.

I have no experience with the database migration scripts so please help me out here, thank you. :)

I hope everything is in the right spot and the solution to pass the creator username into the template is done in the right way.
Please let me know as I am always open for improvements.

Thank you for the great tool!

@s3inlc
Copy link
Member

s3inlc commented Jun 5, 2022

Thanks a lot for the contribution, it already looks pretty great.

As you mentioned, the creator is shown on the hashlist overview. I think it would be beneficial to also have it listed on the hashlist details page (e.g. below the "Member of Access Group" row), could you add this?

Regarding your question for the update script. The idea is that an atomic change in the DB should be wrapped in an if statement which checks for a unique key (to determine if it already was applied or not), e.g. v0.12.x_maxAgents_pretask_task. Inside this statement, in your case you would need to check if the column already exists and if not, create it (The changes which are done on the update with the above key should pretty much show what you would need as well).
The only challenge left then, is that you need to take care about already existing hashlists. If there is no creator set for the existing tasks, it will result in internal server errors. I would propose to just set all of the existing tasks to the oldest (i.e. lowest) user ID, as this typically would be the admin user.
Ideally the entry in the changelog (doc/changelog.md) is included in the PR itself, so please just add a short sentence about this new feature there.

Would it be possible to add these changes? I will test it more in detail afterwards and happily merge it.

@dr0pd34d
Copy link
Author

dr0pd34d commented Jun 6, 2022

Thank you for the valuable feedback.

Sure, I will try my best to integrate it into the detail template too and of course in the change log and try my best with the database migration script.

I will report back ASAP

@dr0pd34d
Copy link
Author

dr0pd34d commented Jun 6, 2022

Okay changes done, please check, I had to merge upstream commits so I hope that was done correctly :)

Copy link
Member

@zyronix zyronix left a comment

Choose a reason for hiding this comment

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

Small change, add the new field to the generator.php found in the src/dba/models/

$CONF['Hashlist'] = [
  'hashlistId',
  'hashlistName',
  'format',
  'hashTypeId',
  'hashCount',
  'saltSeparator',
  'cracked',
  'isSecret',
  'hexSalt',
  'isSalted',
  'accessGroupId',
  'notes',
  'brainId',
  'brainFeatures',
  'creatorId'
];

Bigger issue to resolve: if a user gets deleted; the creatorId should be reset of all of the issues owned by the user or else it will return an error when trying to list access a hashlist because it ID does not resolve to a username anymore.

src/install/updates/update_v0.12.x_v0.x.x.php Show resolved Hide resolved
src/dba/models/Hashlist.class.php Outdated Show resolved Hide resolved
src/dba/models/Hashlist.class.php Outdated Show resolved Hide resolved
src/templates/hashlists/detail.template.html Outdated Show resolved Hide resolved
@dr0pd34d
Copy link
Author

dr0pd34d commented Jun 7, 2022

Thank you for looking over it, you are completely right, I forgot to add and cover those cases.
I will report back once I made some progress.

@zyronix
Copy link
Member

zyronix commented Jun 29, 2022

@dr0pd34d How far are you on this? Is it ready to be reviewed again?

@zyronix zyronix added the enhancement Enhancement of existing features / Small addition label Jun 29, 2022
@dr0pd34d
Copy link
Author

dr0pd34d commented Jul 2, 2022

As you can see I did resolve a lot of the requested changes.
For one change I added a question if it is implemented in a correct way.

The biggest issue was not tackled yet, which is:
Bigger issue to resolve: if a user gets deleted; the creatorId should be reset of all of the issues owned by the user or else it will return an error when trying to list access a hashlist because it ID does not resolve to a username anymore.

@zyronix
Copy link
Member

zyronix commented Aug 2, 2022

Do you need some help with implemnting that or are you working on it?

@dr0pd34d
Copy link
Author

@zyronix Yes please some help would be nice. Currently missing the time to follow up on the issue.

@zyronix zyronix added waiting on user feedback Waiting for a user to report back or provide results ui Hashtopolis UI related labels Jun 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement of existing features / Small addition ui Hashtopolis UI related waiting on user feedback Waiting for a user to report back or provide results
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants