Add improvements to media service operations. #21

dewniMW · 2020-07-10T13:51:20Z

This PR adds the following improvements:

Add the capability to override the properties defined in the media.properties file via environment variables.
Introduce a configuration to define the maximum allowed byte size of the media to be uploaded. The default value would 512 KB.
Have the 'yyyyMMdd' date format (of the media uploaded date) appended to the unique id of the media and incorporate this when creating the hierarchical folder structure.
Enforce user to manually create a directory called 'media' in the media mount location. This decision was taken as an additional safety measure to prevent files from being stored in an incorrect location.
Have user id instead of the username.
Add ETag support for media download API.

...ntity.media.core/src/main/java/org/wso2/carbon/identity/media/core/StorageSystemManager.java

emswbandara · 2020-07-14T11:01:40Z

....core/src/main/java/org/wso2/carbon/identity/media/core/file/FileBasedStorageSystemImpl.java

@@ -79,7 +78,7 @@

    @Override
    public String addMedia(List<InputStream> inputStreams, MediaMetadata mediaMetadata, String uuid,
-                           String tenantDomain) throws StorageSystemException {
+                           String tenantDomain) throws StorageSystemServerException {


why was this changed? don't we need to send a client exception in uuid already exists?

uuid is system generated. hence there is no check for uuid already exists condition.

....core/src/main/java/org/wso2/carbon/identity/media/core/file/FileBasedStorageSystemImpl.java

emswbandara · 2020-07-16T11:35:50Z

components/org.wso2.carbon.identity.media.core/pom.xml

                            org.json.simple,
-                            org.json.simple.parser
+                            org.json.simple.parser,


shall we add a version range for this

unable to provide a version range here. same approach has been followed at https://github.com/wso2-extensions/identity-inbound-auth-oauth/blob/master/components/org.wso2.carbon.identity.oauth.dcr/pom.xml#L134

...n.identity.media.core/src/main/java/org/wso2/carbon/identity/media/core/FileContentImpl.java

emswbandara · 2020-07-16T11:41:51Z

...ntity.media.core/src/main/java/org/wso2/carbon/identity/media/core/StorageSystemManager.java

-        return false;
+        StorageSystemFactory storageSystemFactory = getStorageSystemFactory(getMediaStoreType());
+        String userId = getUserIdFromUserName(username);
+        return storageSystemFactory.getInstance().isDownloadAllowedForProtectedMedia(mediaId, type, tenantDomain,


ideally getStorageSystemFactory should return an instance of a StorageSystemFactory. you need to call isDownloadAllowedForProtectedMedia on that instance. Therefore no need to call getInstance() on an instance (it getInstance is used with singleton class)

fixed with 93a5753

...ntity.media.core/src/main/java/org/wso2/carbon/identity/media/core/StorageSystemManager.java

emswbandara · 2020-07-16T11:47:15Z

...ntity.media.core/src/main/java/org/wso2/carbon/identity/media/core/StorageSystemManager.java

+            UserRealm userRealm = realmService.getTenantUserRealm(tenantID);
+            if (userRealm != null) {
+                UserStoreManager userStoreManager = (UserStoreManager) userRealm.getUserStoreManager();
+                String userIdFromUserName = ((AbstractUserStoreManager) userStoreManager)


does this work correctly for a secondary userstore user?

yes I have tested the flow for a secondary userstore user

....core/src/main/java/org/wso2/carbon/identity/media/core/file/FileBasedStorageSystemImpl.java

emswbandara · 2020-07-17T04:57:16Z

...ntity.media.core/src/main/java/org/wso2/carbon/identity/media/core/StorageSystemManager.java

+            throw new StorageSystemServerException(String.format("Unable to obtain StorageSystemFactory for " +
+                    "configured media store type: %s", storageType));
+        }
+        return storageSystemFactory.getInstance();


here also since storageSystemFactory is an instance variable, its not correct to call getInstance() on that. can we rename the getInstace method to getStorageSystem or something meaningful?

Add improvements to media service operations.

531ecf9

emswbandara reviewed Jul 14, 2020

View reviewed changes

...ntity.media.core/src/main/java/org/wso2/carbon/identity/media/core/StorageSystemManager.java Outdated Show resolved Hide resolved

emswbandara reviewed Jul 14, 2020

View reviewed changes

....core/src/main/java/org/wso2/carbon/identity/media/core/file/FileBasedStorageSystemImpl.java Outdated Show resolved Hide resolved

dewniMW added 2 commits July 15, 2020 07:49

Address PR comments.

651b43c

Add ETag support for media download API.

f9a643b

dewniMW requested review from emswbandara and ashensw July 16, 2020 01:13

emswbandara reviewed Jul 16, 2020

View reviewed changes

emswbandara reviewed Jul 17, 2020

View reviewed changes

Address PR comments.

34fabf7

dewniMW force-pushed the new-changes branch from 93a5753 to 34fabf7 Compare July 17, 2020 05:01

emswbandara approved these changes Jul 17, 2020

View reviewed changes

ashensw approved these changes Jul 17, 2020

View reviewed changes

dewniMW merged commit 62d9ebe into wso2-attic:master Jul 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add improvements to media service operations. #21

Add improvements to media service operations. #21

dewniMW commented Jul 10, 2020 •

edited

Loading

emswbandara Jul 14, 2020

dewniMW Jul 15, 2020

emswbandara Jul 16, 2020

dewniMW Jul 17, 2020

emswbandara Jul 16, 2020

dewniMW Jul 17, 2020

emswbandara Jul 16, 2020

dewniMW Jul 17, 2020

emswbandara Jul 17, 2020

dewniMW Jul 17, 2020

Add improvements to media service operations. #21

Add improvements to media service operations. #21

Conversation

dewniMW commented Jul 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dewniMW commented Jul 10, 2020 •

edited

Loading