search

SharkCagey / SharkCage

1 stars 3 forks source link

Interating token library into the project #59

Closed bencikpeter closed 6 years ago

bencikpeter commented 6 years ago

Majorly rewritten token library, should fix most of the issues raised in the first review, now included in your new project structure.

bencikpeter commented 6 years ago

@DonatJR Well... to your questions:

  1. Actually not.... I´ve just separated the API into two different levels... at tokenManipulation.h are just functions directly related to token manipulation (to be precise, two variants of the same function... one takes a group name and the other group sid). The reasoning is, that for the library it is not essential to create a group the way exposed in groupManipulation.h and the user of the library can choose to do it anyway they deem appropriate. Therefore separating them into different header files allows user to import only the stuff needed. On the other hand, the library provides a way to manage groups as well, if that is needed. If you have a closer look, you will notice that tokenManipulation.h is included in groupManipulation.h therefore by including the latter, you will pull in whole functionality of the library... by just including the former, only token manipulation will be exposed

  2. Yes and no. Those two functions do not depend directly on each other, and can be called in any order. However, the sid object is representing a group in the system and if deleteLocalGroup is called before deleteSid there will be a period of time where an invalid SID object exists (is representing a system object that does not exist anymore). You can however, only call deleteSid without deleteLocalGroup . This will only deallocate a pointer, but keep a group... which is a valid state. You can see deleteSid as an alternative to CloseHandle. It will not destroy the resource, just close this particlular access method.

  3. I will have a look at what precisely that mentioned piece of code does but from what i´ve seen yes... it should be the case but it might even not be necessary since they at the glance seem to be doing exactly the same thing. In either case, I suggest that I might take care of usage of the library inside the project. Since I know it the best it might be a natural progression of my participation in the project. And besides... the library is approaching a finished state and I am assigned to this project until october, I will have to find myself something to work on anyway... 😄

  4. What I was considering before I´ve created this PR was that I would extract the library into a separate repository and include it into the solution as git submodule, but some quirks of git submodules (such as the need to use --recursive modifier in almost every command to update or download it, made me go the direct way... However, integrate it to the project any way that fits your workflow, I don´t mind at all.

SailReal commented 6 years ago

@bencikpeter our develop and master branch are protected. Only signed commits can be merged directly. Would be super cool if you could sign your commits in the future.

bencikpeter commented 6 years ago

Yeah, I’ve noticed that... sorry 😅 On 7 Jul 2018, 18:05 +0200, Julian Raufelder notifications@github.com, wrote:

@bencikpeter our develop and master branch are protected. Only signed commits can be merged directly. Would be super cool if you could sign your commits in the future. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.